Attachment 180978 Details for Bug 105367 – Patch

[patch] Patch

bug-105367-20121231122914.patch (text/plain), 33.81 KB, created by Kenneth Russell on 2012-12-31 09:32:02 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Kenneth Russell

Created: 2012-12-31 09:32:02 PST

Size: 33.81 KB

patch

obsolete

>Subversion Revision: 138595
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 611968f1958f98370901e24b3c3039b5ed842e14..7eac4765204f811dc62a1d982789968493968bbe 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2012-12-31  Kenneth Russell  <kbr@google.com>
>+
>+        [chromium] Creation of dedicated workers (isolates) leaks reference to HTMLDocument
>+        https://bugs.webkit.org/show_bug.cgi?id=105367
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Eliminated a Chromium-specific object wrapping WorkerMessagingProxy in order to fix a
>+        lifetime management bug, which leaked every Document which started a dedicated worker.
>+
>+        Test: fast/workers/worker-document-leak.html
>+
>+        * workers/WorkerLoaderProxy.h:
>+        (WorkerLoaderProxy):
>+            Added Chromium-specific casting method to bridge two now-distinct class hierarchies.
>+        * workers/WorkerMessagingProxy.h:
>+        (WorkerMessagingProxy):
>+            Made destructor protected instead of private to allow subclassing.
>+
> 2012-12-30  Kondapally Kalyan  <kalyan.kondapally@intel.com>
> 
>         [EFL] [WebGL] Rename EGLConfigHelper as EGLConfigSelector.
>diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog
>index 954355ac5f7fc91be70e33abc2aa947c2101637c..53f5c46fbd3db1f3ff7b0dfd4460c0cbdc537ab6 100644
>--- a/Source/WebKit/chromium/ChangeLog
>+++ b/Source/WebKit/chromium/ChangeLog
>@@ -1,3 +1,72 @@
>+2012-12-31  Kenneth Russell  <kbr@google.com>
>+
>+        [chromium] Creation of dedicated workers (isolates) leaks reference to HTMLDocument
>+        https://bugs.webkit.org/show_bug.cgi?id=105367
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Made WebWorkerClientImpl a subclass of WorkerMessagingProxy rather than an object wrapping
>+        WorkerMessagingProxy. WorkerMessagingProxy manages its own lifetime and it is impossible to
>+        properly synchronize the lifetime of WebWorkerClientImpl separately.
>+
>+        This allowed most of WebWorkerClientImpl to be deleted, but forced a divergence in the class
>+        hierarchies of WebWorkerClientImpl and WebSharedWorkerImpl. Conversion methods were added to
>+        WorkerLoaderProxy and WebWorkerBase to bridge the hierarchies of in-process and
>+        out-of-process workers.
>+
>+        * src/DatabaseObserver.cpp:
>+        (WebCore::DatabaseObserver::canEstablishDatabase):
>+            Adjusted how WebWorkerBase is obtained from WorkerLoaderProxy.
>+        * src/IDBFactoryBackendProxy.cpp:
>+        (WebKit::AllowIndexedDBMainThreadBridge::signalCompleted):
>+            Adjusted how WorkerLoaderProxy's methods are called.
>+        (WebKit::IDBFactoryBackendProxy::allowIndexedDB):
>+            Adjusted how WebWorkerBase is obtained from WorkerLoaderProxy.
>+        * src/LocalFileSystemChromium.cpp:
>+        (WebCore::openFileSystemHelper):
>+            Adjusted how WebWorkerBase is obtained from WorkerLoaderProxy.
>+        * src/WebSharedWorkerImpl.cpp:
>+        (WebKit::WebSharedWorkerImpl::toWebWorkerBase):
>+            Implemented new conversion method.
>+        * src/WebSharedWorkerImpl.h:
>+        (WebSharedWorkerImpl):
>+            Explicitly derive from WorkerLoaderProxy now that WebWorkerBase no longer does.
>+        (WebKit::WebSharedWorkerImpl::workerLoaderProxy):
>+            Added new conversion method.
>+        * src/WebWorkerBase.h:
>+        (WebWorkerBase):
>+            Removed derivation from WorkerLoaderProxy. Added method to convert to WorkerLoaderProxy.
>+        * src/WebWorkerClientImpl.cpp:
>+        (WebKit):
>+            Adjusted comment.
>+        (WebKit::WebWorkerClientImpl::createWorkerContextProxy):
>+            Adjusted whitespace.
>+        (WebKit::WebWorkerClientImpl::terminateWorkerContext):
>+            Eliminated delegation to separate object.
>+        (WebKit::WebWorkerClientImpl::toWebWorkerBase):
>+            Implemented new conversion method.
>+        (WebKit::WebWorkerClientImpl::view):
>+        (WebKit::WebWorkerClientImpl::allowDatabase):
>+        (WebKit::WebWorkerClientImpl::allowFileSystem):
>+        (WebKit::WebWorkerClientImpl::openFileSystem):
>+        (WebKit::WebWorkerClientImpl::allowIndexedDB):
>+            Eliminated delegation to separate object.
>+        (WebKit::WebWorkerClientImpl::WebWorkerClientImpl):
>+        * src/WebWorkerClientImpl.h:
>+        (WebKit):
>+            Changed to inherit from WorkerMessagingProxy directly.
>+        (WebWorkerClientImpl):
>+            Deleted most methods previously overridden from WorkerContextProxy, etc.
>+        * src/WorkerAsyncFileSystemChromium.cpp:
>+        (WebCore::WorkerAsyncFileSystemChromium::WorkerAsyncFileSystemChromium):
>+        (WebCore::WorkerAsyncFileSystemChromium::createWorkerFileSystemCallbacksBridge):
>+            Hold on to, and use, WorkerLoaderProxy rather than WebWorkerBase.
>+        * src/WorkerAsyncFileSystemChromium.h:
>+        (WebKit):
>+        (WebCore):
>+        (WorkerAsyncFileSystemChromium):
>+            Hold on to WorkerLoaderProxy rather than WebWorkerBase.
>+
> 2012-12-28  Fady Samuel  <fsamuel@chromium.org>
> 
>         Roll Chromium DEPS to r174739
>diff --git a/Source/WebCore/workers/WorkerLoaderProxy.h b/Source/WebCore/workers/WorkerLoaderProxy.h
>index d2fcf962e8bf1d4465aff133304ec7512c1a66c1..81dd2194c3a8e4994b280007a16f108d1055aebd 100644
>--- a/Source/WebCore/workers/WorkerLoaderProxy.h
>+++ b/Source/WebCore/workers/WorkerLoaderProxy.h
>@@ -55,6 +55,12 @@ namespace WebCore {
>         // specific synchronous loading requests so they can be 'nested', per spec.
>         // Returns true if the task was posted successfully.
>         virtual bool postTaskForModeToWorkerContext(PassOwnPtr<ScriptExecutionContext::Task>, const String& mode) = 0;
>+
>+#if PLATFORM(CHROMIUM)
>+        // Spans divergent class hierarchies for dedicated and shared workers. Implementation must
>+        // static_cast to WebWorkerBase*.
>+        virtual void* toWebWorkerBase() = 0;
>+#endif
>     };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/workers/WorkerMessagingProxy.h b/Source/WebCore/workers/WorkerMessagingProxy.h
>index e86924e712584deab0c44083d8fe6b874b27cc94..d8fb699532c260d091d5633ef77698ab94637e8f 100644
>--- a/Source/WebCore/workers/WorkerMessagingProxy.h
>+++ b/Source/WebCore/workers/WorkerMessagingProxy.h
>@@ -89,6 +89,9 @@ namespace WebCore {
>         // Only use this method on the worker object thread.
>         bool askedToTerminate() const { return m_askedToTerminate; }
> 
>+    protected:
>+        virtual ~WorkerMessagingProxy();
>+
>     private:
>         friend class MessageWorkerTask;
>         friend class PostMessageToPageInspectorTask;
>@@ -96,8 +99,6 @@ namespace WebCore {
>         friend class WorkerExceptionTask;
>         friend class WorkerThreadActivityReportTask;
> 
>-        virtual ~WorkerMessagingProxy();
>-
>         void workerContextDestroyedInternal();
>         static void workerObjectDestroyedInternal(ScriptExecutionContext*, WorkerMessagingProxy*);
>         void reportPendingActivityInternal(bool confirmingMessage, bool hasPendingActivity);
>diff --git a/Source/WebKit/chromium/src/DatabaseObserver.cpp b/Source/WebKit/chromium/src/DatabaseObserver.cpp
>index 322e800ec2164cb1f07f4d486c2867a6b7be2199..65c585826482ef6dc2330c9881cd3d354e60c33a 100644
>--- a/Source/WebKit/chromium/src/DatabaseObserver.cpp
>+++ b/Source/WebKit/chromium/src/DatabaseObserver.cpp
>@@ -164,8 +164,7 @@ bool DatabaseObserver::canEstablishDatabase(ScriptExecutionContext* scriptExecut
>     } else {
> #if ENABLE(WORKERS)
>         WorkerContext* workerContext = static_cast<WorkerContext*>(scriptExecutionContext);
>-        WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy();
>-        WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerLoaderProxy);
>+        WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerContext->thread()->workerLoaderProxy().toWebWorkerBase());
>         WebView* view = webWorker->view();
>         if (!view)
>             return false;
>diff --git a/Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp b/Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp
>index 0e65971974930d3483e629f495e67bd5038246e4..c60a3cba94e138be80af312bbd8016daf7d59feb 100755
>--- a/Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp
>+++ b/Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp
>@@ -111,7 +111,7 @@ public:
>     {
>         MutexLocker locker(m_mutex);
>         if (m_webWorkerBase)
>-            m_webWorkerBase->postTaskForModeToWorkerContext(createCallbackTask(&didComplete, this, result), mode);
>+            m_webWorkerBase->workerLoaderProxy()->postTaskForModeToWorkerContext(createCallbackTask(&didComplete, this, result), mode);
>     }
> 
> private:
>@@ -172,7 +172,7 @@ bool IDBFactoryBackendProxy::allowIndexedDB(ScriptExecutionContext* context, con
>         allowed = !webView->permissionClient() || webView->permissionClient()->allowIndexedDB(webFrame, name, origin);
>     } else {
>         WorkerContext* workerContext = static_cast<WorkerContext*>(context);
>-        WebWorkerBase* webWorkerBase = static_cast<WebWorkerBase*>(&workerContext->thread()->workerLoaderProxy());
>+        WebWorkerBase* webWorkerBase = static_cast<WebWorkerBase*>(workerContext->thread()->workerLoaderProxy().toWebWorkerBase());
>         WorkerRunLoop& runLoop = workerContext->thread()->runLoop();
> 
>         String mode = allowIndexedDBMode;
>diff --git a/Source/WebKit/chromium/src/LocalFileSystemChromium.cpp b/Source/WebKit/chromium/src/LocalFileSystemChromium.cpp
>index 5fa07ddadd24d1430795fa4b9e476dc3389f4aa5..7fe545d16434499e3935fb96230c0c7660988c32 100644
>--- a/Source/WebKit/chromium/src/LocalFileSystemChromium.cpp
>+++ b/Source/WebKit/chromium/src/LocalFileSystemChromium.cpp
>@@ -203,8 +203,7 @@ static void openFileSystemHelper(ScriptExecutionContext* context, FileSystemType
>     } else {
> #if ENABLE(WORKERS)
>         WorkerContext* workerContext = static_cast<WorkerContext*>(context);
>-        WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy();
>-        WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerLoaderProxy);
>+        WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerContext->thread()->workerLoaderProxy().toWebWorkerBase());
>         if (!allowFileSystemForWorker(webWorker->commonClient()))
>             allowed = false;
>         else
>diff --git a/Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp b/Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp
>index 3109c75d3fb4a7a841ab46de30dc9f1a68e5d94f..ecdeabaae11490e904f84da2ad9efffd41924c6a 100644
>--- a/Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp
>+++ b/Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp
>@@ -324,6 +324,10 @@ bool WebSharedWorkerImpl::postTaskForModeToWorkerContext(
>     return true;
> }
> 
>+void* WebSharedWorkerImpl::toWebWorkerBase()
>+{
>+    return static_cast<WebWorkerBase*>(this);
>+}
> 
> 
> bool WebSharedWorkerImpl::isStarted()
>diff --git a/Source/WebKit/chromium/src/WebSharedWorkerImpl.h b/Source/WebKit/chromium/src/WebSharedWorkerImpl.h
>index 491211e54d6f3f04de9e794ce185201d37f5e274..030ea6a33191a06de34ae5fb4285dfa0a9fdbb95 100644
>--- a/Source/WebKit/chromium/src/WebSharedWorkerImpl.h
>+++ b/Source/WebKit/chromium/src/WebSharedWorkerImpl.h
>@@ -61,10 +61,12 @@ class WebSharedWorkerClient;
> // It can't use it directly since it uses WebKit types, so this class converts the data types.
> // When the WebCore::SharedWorker object wants to call WebCore::WorkerReportingProxy, this class will
> // convert to Chrome data types first and then call the supplied WebCommonWorkerClient.
>-class WebSharedWorkerImpl : public WebCore::WorkerObjectProxy
>-                          , public WebWorkerBase
>-                          , public WebFrameClient
>-                          , public WebSharedWorker {
>+class WebSharedWorkerImpl
>+    : public WebCore::WorkerObjectProxy
>+    , public WebCore::WorkerLoaderProxy
>+    , public WebWorkerBase
>+    , public WebFrameClient
>+    , public WebSharedWorker {
> public:
>     explicit WebSharedWorkerImpl(WebSharedWorkerClient*);
> 
>@@ -88,6 +90,7 @@ public:
>     virtual void postTaskToLoader(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
>     virtual bool postTaskForModeToWorkerContext(
>         PassOwnPtr<WebCore::ScriptExecutionContext::Task>, const WTF::String& mode);
>+    virtual void* toWebWorkerBase() OVERRIDE;
> 
>     // WebFrameClient methods to support resource loading thru the 'shadow page'.
>     virtual void didCreateDataSource(WebFrame*, WebDataSource*);
>@@ -111,7 +114,8 @@ public:
>     virtual void dispatchDevToolsMessage(const WebString&);
> 
> 
>-    // NewWebWorkerBase methods:
>+    // WebWorkerBase methods:
>+    WebCore::WorkerLoaderProxy* workerLoaderProxy() { return this; }
>     WebCommonWorkerClient* commonClient() { return m_client; }
> 
> private:
>diff --git a/Source/WebKit/chromium/src/WebWorkerBase.h b/Source/WebKit/chromium/src/WebWorkerBase.h
>index c43c63931b041850647a5e522cde4ce019135cbb..f8315ac1f70b48fc497c2c5fe005b2061190b391 100644
>--- a/Source/WebKit/chromium/src/WebWorkerBase.h
>+++ b/Source/WebKit/chromium/src/WebWorkerBase.h
>@@ -48,8 +48,9 @@ class WebView;
> // containing common interface for shared workers and dedicated in-proc workers implementation.
> //
> // FIXME: Rename this class into WebWorkerBase, merge existing WebWorkerBase and WebSharedWorker.
>-class WebWorkerBase : public WebCore::WorkerLoaderProxy {
>+class WebWorkerBase {
> public:
>+    virtual WebCore::WorkerLoaderProxy* workerLoaderProxy() = 0;
>     virtual WebCommonWorkerClient* commonClient() = 0;
>     virtual WebView* view() const = 0;
> 
>diff --git a/Source/WebKit/chromium/src/WebWorkerClientImpl.cpp b/Source/WebKit/chromium/src/WebWorkerClientImpl.cpp
>index cf7e0d2f09dc0b969fe63c46e10e233d4667429d..fa64b5375efa0f8362c35d94316a6fd9a1755edb 100644
>--- a/Source/WebKit/chromium/src/WebWorkerClientImpl.cpp
>+++ b/Source/WebKit/chromium/src/WebWorkerClientImpl.cpp
>@@ -69,7 +69,7 @@ using namespace WebCore;
> 
> namespace WebKit {
> 
>-// Chromium-specific wrapper over WorkerMessagingProxy.
>+// Chromium-specific decorator of WorkerMessagingProxy.
> // Delegates implementation of Worker{Loader,Context,Object}Proxy to WorkerMessagingProxy.
> 
> // static
>@@ -79,127 +79,43 @@ WorkerContextProxy* WebWorkerClientImpl::createWorkerContextProxy(Worker* worker
>         Document* document = static_cast<Document*>(worker->scriptExecutionContext());
>         WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
>         WebWorkerClientImpl* proxy = new WebWorkerClientImpl(worker, webFrame);
>-        return proxy; 
>-    } 
>+        return proxy;
>+    }
>     ASSERT_NOT_REACHED();
>     return 0;
> }
> 
>-void WebWorkerClientImpl::startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode, WorkerThreadStartMode startMode)
>-{
>-    ASSERT(m_scriptExecutionContext->isDocument());
>-    Document* document = static_cast<Document*>(m_scriptExecutionContext.get());
>-    GroupSettings* settings = 0;
>-    if (document->page())
>-        settings = document->page()->group().groupSettings();
>-    RefPtr<DedicatedWorkerThread> thread = DedicatedWorkerThread::create(scriptURL, userAgent, settings, sourceCode, *this, *this, startMode,
>-                                                                         document->contentSecurityPolicy()->deprecatedHeader(),
>-                                                                         document->contentSecurityPolicy()->deprecatedHeaderType(),
>-                                                                         document->topDocument()->securityOrigin());
>-    m_proxy->workerThreadCreated(thread);
>-    thread->start();
>-    InspectorInstrumentation::didStartWorkerContext(m_scriptExecutionContext.get(), m_proxy, scriptURL);
>-}
>-
> void WebWorkerClientImpl::terminateWorkerContext()
> {
>     m_webFrame = 0;
>-    m_proxy->terminateWorkerContext();
>-}
>-
>-void WebWorkerClientImpl::postMessageToWorkerContext(
>-    PassRefPtr<SerializedScriptValue> value, 
>-    PassOwnPtr<MessagePortChannelArray> ports)
>-{
>-    m_proxy->postMessageToWorkerContext(value, ports);
>-}
>-
>-bool WebWorkerClientImpl::hasPendingActivity() const
>-{
>-    return m_proxy->hasPendingActivity();
>-}
>-
>-void WebWorkerClientImpl::workerObjectDestroyed()
>-{
>-    m_proxy->workerObjectDestroyed();
>-}
>-
>-#if ENABLE(INSPECTOR)
>-void WebWorkerClientImpl::connectToInspector(PageInspector* inspector)
>-{
>-    m_proxy->connectToInspector(inspector);
>-}
>-
>-void WebWorkerClientImpl::disconnectFromInspector()
>-{
>-    m_proxy->disconnectFromInspector();
>-}
>-
>-void WebWorkerClientImpl::sendMessageToInspector(const String& message)
>-{
>-    m_proxy->sendMessageToInspector(message);
>-}
>-
>-void WebWorkerClientImpl::postMessageToPageInspector(const String& message)
>-{
>-    m_proxy->postMessageToPageInspector(message);
>-}
>-
>-void WebWorkerClientImpl::updateInspectorStateCookie(const String& cookie)
>-{
>-    m_proxy->updateInspectorStateCookie(cookie);
>-}
>-#endif // ENABLE(INSPECTOR)
>-
>-
>-void WebWorkerClientImpl::postTaskToLoader(PassOwnPtr<ScriptExecutionContext::Task> task)
>-{
>-    m_proxy->postTaskToLoader(task);
>+    WebCore::WorkerMessagingProxy::terminateWorkerContext();
> }
> 
>-bool WebWorkerClientImpl::postTaskForModeToWorkerContext(PassOwnPtr<ScriptExecutionContext::Task> task, const String& mode)
>+void* WebWorkerClientImpl::toWebWorkerBase()
> {
>-    return m_proxy->postTaskForModeToWorkerContext(task, mode);
>+    return static_cast<WebWorkerBase*>(this);
> }
> 
>-void WebWorkerClientImpl::postMessageToWorkerObject(PassRefPtr<SerializedScriptValue> value, PassOwnPtr<MessagePortChannelArray> ports)
>+WebView* WebWorkerClientImpl::view() const
> {
>-    m_proxy->postMessageToWorkerObject(value, ports);
>-}
>-
>-void WebWorkerClientImpl::confirmMessageFromWorkerObject(bool hasPendingActivity)
>-{
>-    m_proxy->confirmMessageFromWorkerObject(hasPendingActivity);
>-}
>-
>-void WebWorkerClientImpl::reportPendingActivity(bool hasPendingActivity)
>-{
>-    m_proxy->reportPendingActivity(hasPendingActivity);
>-}
>-
>-void WebWorkerClientImpl::workerContextClosed()
>-{
>-    m_proxy->workerContextClosed();
>-}
>-
>-void WebWorkerClientImpl::postExceptionToWorkerObject(const String& errorMessage, int lineNumber, const String& sourceURL)
>-{
>-    m_proxy->postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);
>-}
>-
>-void WebWorkerClientImpl::postConsoleMessageToWorkerObject(MessageSource source, MessageLevel level, const String& message, int lineNumber, const String& sourceURL)
>-{
>-    m_proxy->postConsoleMessageToWorkerObject(source, level, message, lineNumber, sourceURL);
>+    if (askedToTerminate())
>+        return 0;
>+    return m_webFrame->view();
> }
> 
>-void WebWorkerClientImpl::workerContextDestroyed()
>+bool WebWorkerClientImpl::allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize)
> {
>-    m_proxy->workerContextDestroyed();
>+    if (askedToTerminate())
>+        return false;
>+    WebKit::WebViewImpl* webView = m_webFrame->viewImpl();
>+    if (!webView)
>+        return false;
>+    return !webView->permissionClient() || webView->permissionClient()->allowDatabase(m_webFrame, name, displayName, estimatedSize);
> }
> 
> bool WebWorkerClientImpl::allowFileSystem()
> {
>-    if (m_proxy->askedToTerminate())
>+    if (askedToTerminate())
>         return false;
>     WebKit::WebViewImpl* webView = m_webFrame->viewImpl();
>     if (!webView)
>@@ -207,47 +123,29 @@ bool WebWorkerClientImpl::allowFileSystem()
>     return !webView->permissionClient() || webView->permissionClient()->allowFileSystem(m_webFrame);
> }
> 
>-void WebWorkerClientImpl::openFileSystem(WebFileSystem::Type type, long long size, bool create, 
>+void WebWorkerClientImpl::openFileSystem(WebFileSystem::Type type, long long size, bool create,
>                                          WebFileSystemCallbacks* callbacks)
> {
>-    if (m_proxy->askedToTerminate()) {
>+    if (askedToTerminate()) {
>         callbacks->didFail(WebFileErrorAbort);
>         return;
>     }
>     m_webFrame->client()->openFileSystem(m_webFrame, type, size, create, callbacks);
> }
> 
>-bool WebWorkerClientImpl::allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) 
>-{
>-    if (m_proxy->askedToTerminate())
>-        return false;
>-    WebKit::WebViewImpl* webView = m_webFrame->viewImpl();
>-    if (!webView)
>-        return false;
>-    return !webView->permissionClient() || webView->permissionClient()->allowDatabase(m_webFrame, name, displayName, estimatedSize);
>-}
>-
> bool WebWorkerClientImpl::allowIndexedDB(const WebString& name)
> {
>-    if (m_proxy->askedToTerminate())
>+    if (askedToTerminate())
>         return false;
>     WebKit::WebViewImpl* webView = m_webFrame->viewImpl();
>     if (!webView)
>         return false;
>     return !webView->permissionClient() || webView->permissionClient()->allowIndexedDB(m_webFrame, name, WebSecurityOrigin());
> }
>- 
>-WebView* WebWorkerClientImpl::view() const 
>-{
>-    if (m_proxy->askedToTerminate())
>-        return 0;
>-    return m_webFrame->view(); 
>-}
> 
> WebWorkerClientImpl::WebWorkerClientImpl(Worker* worker, WebFrameImpl* webFrame)
>-    : m_proxy(new WorkerMessagingProxy(worker))
>-    , m_scriptExecutionContext(worker->scriptExecutionContext())
>-    , m_webFrame(webFrame)    
>+    : WebCore::WorkerMessagingProxy(worker)
>+    , m_webFrame(webFrame)
> {
> }
> 
>diff --git a/Source/WebKit/chromium/src/WebWorkerClientImpl.h b/Source/WebKit/chromium/src/WebWorkerClientImpl.h
>index 381a495baf4f6af84a11420cc545c24e30203a89..c677432266869df962444ac38aac2bf30e55a7bb 100644
>--- a/Source/WebKit/chromium/src/WebWorkerClientImpl.h
>+++ b/Source/WebKit/chromium/src/WebWorkerClientImpl.h
>@@ -54,9 +54,13 @@ class WebFrameImpl;
> // for in-proc dedicated workers. It also acts as a bridge for workers to chromium implementation of file systems,
> // databases and other related functionality.
> //
>-// In essence, this class wraps WorkerMessagingProxy.
>-class WebWorkerClientImpl : public WebCore::WorkerContextProxy
>-                          , public WebCore::WorkerObjectProxy
>+// In essence, this class decorates WorkerMessagingProxy.
>+//
>+// It is imperative that this class inherit from WorkerMessagingProxy rather than delegate to an instance of
>+// WorkerMessagingProxy, because that class tracks and reports its activity to outside callers, and manages
>+// its own lifetime, via calls to workerObjectDestroyed, workerContextDestroyed, workerContextClosed, etc. It
>+// is basically impossible to correctly manage the lifetime of this class separately from WorkerMessagingProxy.
>+class WebWorkerClientImpl : public WebCore::WorkerMessagingProxy
>                           , public WebWorkerBase
>                           , public WebCommonWorkerClient {
> public:
>@@ -66,57 +70,27 @@ public:
>     // WebCore::WorkerContextProxy methods:
>     // These are called on the thread that created the worker.  In the renderer
>     // process, this will be the main WebKit thread.
>-    virtual void startWorkerContext(const WebCore::KURL&,
>-                                    const WTF::String&,
>-                                    const WTF::String&,
>-                                    WebCore::WorkerThreadStartMode) OVERRIDE;
>     virtual void terminateWorkerContext() OVERRIDE;
>-    virtual void postMessageToWorkerContext(
>-        PassRefPtr<WebCore::SerializedScriptValue> message,
>-        PassOwnPtr<WebCore::MessagePortChannelArray> channels) OVERRIDE;
>-    virtual bool hasPendingActivity() const OVERRIDE;
>-    virtual void workerObjectDestroyed() OVERRIDE;
>-
>-#if ENABLE(INSPECTOR)
>-    virtual void connectToInspector(WebCore::WorkerContextProxy::PageInspector*) OVERRIDE;
>-    virtual void disconnectFromInspector() OVERRIDE;
>-    virtual void sendMessageToInspector(const String&) OVERRIDE;
>-    virtual void postMessageToPageInspector(const String&) OVERRIDE;
>-    virtual void updateInspectorStateCookie(const String&) OVERRIDE;
>-#endif
>-    // WebCore::WorkerLoaderProxy methods:
>-    virtual void postTaskToLoader(PassOwnPtr<WebCore::ScriptExecutionContext::Task>) OVERRIDE;
>-    virtual bool postTaskForModeToWorkerContext(PassOwnPtr<WebCore::ScriptExecutionContext::Task>, const String& mode) OVERRIDE;
>-
>-    // WebCore::WorkerObjectProxy methods:
>-    virtual void postMessageToWorkerObject(PassRefPtr<WebCore::SerializedScriptValue>, PassOwnPtr<WebCore::MessagePortChannelArray>) OVERRIDE;
>-    virtual void postExceptionToWorkerObject(const String& errorMessage, int lineNumber, const String& sourceURL) OVERRIDE;
>-
>-    virtual void postConsoleMessageToWorkerObject(WebCore::MessageSource, WebCore::MessageLevel,
>-                                                  const String& message, int lineNumber, const String& sourceURL) OVERRIDE;
>-    virtual void confirmMessageFromWorkerObject(bool) OVERRIDE;
>-    virtual void reportPendingActivity(bool) OVERRIDE;
>-    virtual void workerContextClosed() OVERRIDE;
>-    virtual void workerContextDestroyed() OVERRIDE;
>-
>-    // WebWorkerClientBase methods:
>+
>+    // WebCore::WorkerLoaderProxy methods
>+    virtual void* toWebWorkerBase() OVERRIDE;
>+
>+    // WebWorkerBase methods:
>+    virtual WebCore::WorkerLoaderProxy* workerLoaderProxy() OVERRIDE { return this; }
>+    virtual WebCommonWorkerClient* commonClient() OVERRIDE { return this; }
>+    virtual WebView* view() const OVERRIDE;
>+
>+    // WebCommonWorkerClient methods:
>     virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) OVERRIDE;
>     virtual bool allowFileSystem();
>     virtual void openFileSystem(WebFileSystem::Type, long long size, bool create,
>                                 WebFileSystemCallbacks*) OVERRIDE;
>     virtual bool allowIndexedDB(const WebString& name) OVERRIDE;
> 
>-    // WebCommentWorkerBase methods:
>-    virtual WebCommonWorkerClient* commonClient() OVERRIDE { return this; }
>-    virtual WebView* view() const OVERRIDE;
>-
> private:
>     WebWorkerClientImpl(WebCore::Worker*, WebFrameImpl*);
>     virtual ~WebWorkerClientImpl();
> 
>-    WebCore::WorkerMessagingProxy* m_proxy;
>-    // Guard against context from being destroyed before a worker exits.
>-    RefPtr<WebCore::ScriptExecutionContext> m_scriptExecutionContext;
>     WebFrameImpl* m_webFrame;
> };
> 
>diff --git a/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp b/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp
>index 4a5f69dc2489902b1e5c3f279604e3004a4c5d8f..2259537edf3091cddd9f563345ca6769ed856ccb 100644
>--- a/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp
>+++ b/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp
>@@ -40,7 +40,6 @@
> #include "NotImplemented.h"
> #include "WebFileSystemCallbacksImpl.h"
> #include "WebFileWriter.h"
>-#include "WebWorkerBase.h"
> #include "WorkerAsyncFileWriterChromium.h"
> #include "WorkerContext.h"
> #include "WorkerFileSystemCallbacksBridge.h"
>@@ -62,8 +61,7 @@ WorkerAsyncFileSystemChromium::WorkerAsyncFileSystemChromium(ScriptExecutionCont
> {
>     ASSERT(m_scriptExecutionContext->isWorkerContext());
> 
>-    WorkerLoaderProxy* workerLoaderProxy = &m_workerContext->thread()->workerLoaderProxy();
>-    m_worker = static_cast<WebWorkerBase*>(workerLoaderProxy);
>+    m_workerLoaderProxy = &m_workerContext->thread()->workerLoaderProxy();
> }
> 
> WorkerAsyncFileSystemChromium::~WorkerAsyncFileSystemChromium()
>@@ -194,7 +192,7 @@ PassRefPtr<WorkerFileSystemCallbacksBridge> WorkerAsyncFileSystemChromium::creat
>     m_modeForCurrentOperation = fileSystemOperationsMode;
>     m_modeForCurrentOperation.append(String::number(m_workerContext->thread()->runLoop().createUniqueId()));
> 
>-    m_bridgeForCurrentOperation = WorkerFileSystemCallbacksBridge::create(m_worker, m_scriptExecutionContext, new WebKit::WebFileSystemCallbacksImpl(callbacks));
>+    m_bridgeForCurrentOperation = WorkerFileSystemCallbacksBridge::create(m_workerLoaderProxy, m_scriptExecutionContext, new WebKit::WebFileSystemCallbacksImpl(callbacks));
>     return m_bridgeForCurrentOperation;
> }
> 
>diff --git a/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.h b/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.h
>index 6727d1e474811428d3042bb88cb6fec74f5cc471..fa4572629082c0e081f1fedfea44eed9c87c2348 100644
>--- a/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.h
>+++ b/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.h
>@@ -41,7 +41,6 @@
> namespace WebKit {
> class WebFileSystem;
> class WebURL;
>-class WebWorkerBase;
> class WorkerFileSystemCallbacksBridge;
> }
> 
>@@ -50,6 +49,7 @@ namespace WebCore {
> class AsyncFileSystemCallbacks;
> class ScriptExecutionContext;
> class WorkerContext;
>+class WorkerLoaderProxy;
> 
> class WorkerAsyncFileSystemChromium : public AsyncFileSystemChromium {
> public:
>@@ -82,7 +82,7 @@ private:
>     PassRefPtr<WebKit::WorkerFileSystemCallbacksBridge> createWorkerFileSystemCallbacksBridge(PassOwnPtr<AsyncFileSystemCallbacks>);
> 
>     ScriptExecutionContext* m_scriptExecutionContext;
>-    WebKit::WebWorkerBase* m_worker;
>+    WorkerLoaderProxy* m_workerLoaderProxy;
>     WorkerContext* m_workerContext;
>     RefPtr<WebKit::WorkerFileSystemCallbacksBridge> m_bridgeForCurrentOperation;
>     String m_modeForCurrentOperation;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5b3696d365c8c0c39504e6e1142574583f1bd982..2df6106ca33a03f9901fc7608253436807dd28ff 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2012-12-31  Kenneth Russell  <kbr@google.com>
>+
>+        [chromium] Creation of dedicated workers (isolates) leaks reference to HTMLDocument
>+        https://bugs.webkit.org/show_bug.cgi?id=105367
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/workers/resources/empty-worker.js: Added.
>+        * fast/workers/resources/worker-document-leak-iframe.html: Added.
>+        * fast/workers/worker-document-leak-expected.txt: Added.
>+        * fast/workers/worker-document-leak.html: Added.
>+
> 2012-12-31  Kangil Han  <kangil.han@samsung.com>
> 
>         [EFL] css3/masking/clip-path-circle-relative-overflow.html needs expected results update after 97217
>diff --git a/LayoutTests/fast/workers/resources/empty-worker.js b/LayoutTests/fast/workers/resources/empty-worker.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..e0db48437bbff0d856f1bb4274e38daf69edbd0d
>--- /dev/null
>+++ b/LayoutTests/fast/workers/resources/empty-worker.js
>@@ -0,0 +1,2 @@
>+postMessage("closing");
>+close();
>diff --git a/LayoutTests/fast/workers/resources/worker-document-leak-iframe.html b/LayoutTests/fast/workers/resources/worker-document-leak-iframe.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..592bcfefaa9a494bf37aea43a036153e17083dec
>--- /dev/null
>+++ b/LayoutTests/fast/workers/resources/worker-document-leak-iframe.html
>@@ -0,0 +1,14 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="worker-util.js"></script>
>+<script>
>+var worker = new Worker('empty-worker.js');
>+worker.onmessage = function(event) {
>+    waitUntilWorkerThreadsExit(function() {
>+        parent.postMessage("done", "*");
>+    });
>+};
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/workers/worker-document-leak-expected.txt b/LayoutTests/fast/workers/worker-document-leak-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..9005a9af7ea98baf79bd1f7bb81cf5f67178f41d
>--- /dev/null
>+++ b/LayoutTests/fast/workers/worker-document-leak-expected.txt
>@@ -0,0 +1,4 @@
>+Verify that creation of a worker does not leak its creating document.
>+
>+PASS: did not leak documents during test run
>+
>diff --git a/LayoutTests/fast/workers/worker-document-leak.html b/LayoutTests/fast/workers/worker-document-leak.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..06756fe874262a65587f81c652f3c2102a4bbf6c
>--- /dev/null
>+++ b/LayoutTests/fast/workers/worker-document-leak.html
>@@ -0,0 +1,89 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<p>Verify that creation of a worker does not leak its creating document.</p>
>+<div id='console'></div>
>+<script src='resources/worker-util.js'></script>
>+<script>
>+function log(message)
>+{
>+    document.getElementById("console").innerHTML += message + "<br>";
>+}
>+
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    testRunner.waitUntilDone();
>+}
>+
>+// Set this number as high as possible without introducing timeouts in debug builds.
>+// Reducing it does not require rebaselines.
>+var numIterations = 6;
>+
>+var currentIteration = 0;
>+var iframe = null;
>+var numLiveAtStart = 0;
>+var numLiveAtEnd = 0;
>+
>+window.onmessage = function(event) {
>+    if (event.data == "done") {
>+        runOneIteration();
>+    }
>+};
>+
>+function startTest()
>+{
>+    gc();
>+    if (window.internals && window.internals.numberOfLiveDocuments) {
>+        numLiveAtStart = window.internals.numberOfLiveDocuments();
>+        // Depending on which tests ran before this one in DumpRenderTree,
>+        // their Document instances may not have been fully cleaned up yet.
>+        // When this test is run in isolation, there should be only one
>+        // live document at this point.
>+        runOneIteration();
>+    } else {
>+        log("window.internals.numberOfLiveDocuments not available -- no point in running test");
>+        finishTest();
>+    }
>+}
>+
>+function runOneIteration() {
>+    if (currentIteration < numIterations) {
>+        ++currentIteration;
>+
>+        var createdIframe = false;
>+        if (!iframe) {
>+            iframe = document.createElement("iframe");
>+            createdIframe = true;
>+        }
>+        iframe.setAttribute("src", "resources/worker-document-leak-iframe.html");
>+        if (createdIframe)
>+            document.body.appendChild(iframe);
>+    } else {
>+        finishTest();
>+    }
>+}
>+
>+function finishTest()
>+{
>+    gc();
>+
>+    if (window.internals && window.internals.numberOfLiveDocuments) {
>+        numLiveAtEnd = window.internals.numberOfLiveDocuments();
>+        // Under no circumstances should the number of live documents
>+        // at the end be more than 1 greater than the number at the
>+        // beginning (because of the iframe).
>+        if (numLiveAtEnd > numLiveAtStart + 1) {
>+            log("FAIL: leaked documents during test run (started with " + numLiveAtStart + ", ended with " + numLiveAtEnd);
>+        } else {
>+            log("PASS: did not leak documents during test run");
>+        }
>+    }
>+
>+    if (window.testRunner)
>+        testRunner.notifyDone();
>+}
>+
>+window.onload = startTest;
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 105367: 180056 | 180817 | 180818 | 180978 | 181115 | 181129