Bug 28170 – SharedWorkers do not exit when the last parent document exits

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

The SharedWorker lifecycle is tied to the lifecycle of its associated documents.

When the last document associated with a SharedWorker exits, the worker needs to be shut down.

[Andrew Wilson]

Created attachment 34539 [details]
proposed patch

[David Levin]

Comment on attachment 34539 [details]
proposed patch

Just some style nits that I saw while looking this over quickly.  More comments tomorrow.

> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.cpp b/WebCore/workers/DefaultSharedWorkerRepository.cpp
> -SharedWorkerProxy::SharedWorkerProxy(const String& name, const KURL& url)
> +SharedWorkerProxy::SharedWorkerProxy(const String& name, const KURL& url, PassRefPtr<SecurityOrigin> origin)
>      : m_closing(false)
>      , m_name(name.copy())
>      , m_url(url.copy())
> +    , m_origin(origin)
> +{

This should assert that m_origin only has one ref.


> +    for (SharedWorkerProxyRepository::iterator iter = m_cache.begin() ; iter != m_cache.end() ; ++iter) {

There are lots of places where there are spaces before ";" in for loops.  There shouldn't be.

[David Levin]

Comment on attachment 34539 [details]
proposed patch

Final comments for this version of the patch.


> diff --git a/LayoutTests/fast/workers/shared-worker-shared.html-disabled b/LayoutTests/fast/workers/shared-worker-shared.html-disabled

How is this testing that previous incarnations of the shared worker are closed?  Maybe you can add a comment in here?


> diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
> +#if ENABLE(SHARED_WORKERS)
> +        && !SharedWorkerRepository::hasSharedWorkers(m_frame->document())

Why can't the page be cache when it has shared workers? Oh nice, you put this in the changelog!
Nevermind.


> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.cpp b/WebCore/workers/DefaultSharedWorkerRepository.cpp

> +    bool isDocumentInWorkerDocuments(Document* document) { return m_documentSet.contains(document); }

Suggest: isInWorkerDocuments or inWorkerDocuments


About "m_documentSet", it seems like you're using "workerDocuments" consistently for this, so why not name the variable that as well?


> +void SharedWorkerProxy::close()
> +{

ASSERT(!m_closing);

> +    m_closing = true;
> +    // Stop the worker thread - the proxy will stay around until we get workerThreadExited() notification.
> +    if (m_thread)
> +        m_thread->stop();
>  }


> @@ -203,7 +237,7 @@ DefaultSharedWorkerRepository& DefaultSharedWorkerRepository::instance()
>  void DefaultSharedWorkerRepository::workerScriptLoaded(SharedWorkerProxy& proxy, const String& userAgent, const String& workerScript, PassOwnPtr<MessagePortChannel> port)
>  {
>      MutexLocker lock(m_lock);
> -    if (proxy.closing())
> +    if (proxy.isClosing())
>          return;
>  
>      // Another loader may have already started up a thread for this proxy - if so, just send a connect to the pre-existing thread.
> @@ -220,6 +254,44 @@ void SharedWorkerRepository::connect(PassRefPtr<SharedWorker> worker, PassOwnPtr
>      DefaultSharedWorkerRepository::instance().connectToWorker(worker, port, url, name, ec);
>  }
>  
> +void SharedWorkerRepository::documentDetached(Document* document)
> +{
> +    DefaultSharedWorkerRepository::instance().documentDetached(document);
> +}
> +
> +bool SharedWorkerRepository::hasSharedWorkers(Document* document)
> +{
> +    return DefaultSharedWorkerRepository::instance().hasSharedWorkers(document);
> +}
> +
> +bool DefaultSharedWorkerRepository::hasSharedWorkers(Document* document)
> +{
> +    MutexLocker lock(m_lock);
> +    for (SharedWorkerProxyRepository::iterator iter = m_cache.begin() ; iter != m_cache.end() ; ++iter) {

Prefer indexes over iterators for vector



> @@ -247,17 +319,13 @@ PassRefPtr<SharedWorkerProxy> DefaultSharedWorkerRepository::getProxy(const Stri

> +    for (SharedWorkerProxyRepository::iterator iter = m_cache.begin() ; iter < m_cache.end() ; ++iter) {

Prefer indexes over iterators for vectors.


> +        if (!(*iter)->isClosing() && (*iter)->matches(name, origin))
> +            return *iter;
>      }
> +    // Proxy is not in the repository currently - create a new one.
> +    RefPtr<SharedWorkerProxy> proxy = SharedWorkerProxy::create(name, url, origin.release());
> +    m_cache.append(proxy);
>      return proxy;

Slightly better to use release() here. (Avoids unnecessary ref count fuzzing.)



> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.h b/WebCore/workers/DefaultSharedWorkerRepository.h
> @@ -59,6 +61,14 @@ namespace WebCore {
> +        // Notification that a document has been detached, so we can shutdown any associated SharedWorkers.

Suggest removing "so..." see explanation for similar suggestions (near the end of this review).

> +        void documentDetached(Document*);

> +        // Invoked when a thread has exited, so we can remove the SharedWorkerProxy from the repository.
Suggest removing "so..." see explanation for similar suggestions (near the end of this review).
> +        void removeProxy(SharedWorkerProxy*);


> +        SharedWorkerProxyRepository m_cache;

Consider fixing the name "cache" since this isn't a cache.


> diff --git a/WebCore/workers/SharedWorkerRepository.h b/WebCore/workers/SharedWorkerRepository.h
> +        // Invoked when a document has been detached, so we can shut down any associated workers.

"so we can shut down any associated workers."
This part of the comment can be easily become obsolete in lots of ways. I'd suggest removing it.


> +        static void documentDetached(Document*);
> +
> +        // Returns true if the passed document is associated with any SharedWorkers, to allow FrameLoader to determine if the Document can be cached in the page cache.

This part of the comment can be easily become obsolete in lots of ways.
  " to allow FrameLoader to determine if the Document can be cached in the page cache."
I'd suggest removing it.

[Andrew Wilson]

Created attachment 34616 [details]
Patch addressing Levin's comments

[David Levin]

Comment on attachment 34616 [details]
Patch addressing Levin's comments

You have a duplicate changelog entry in WebCore/ChangeLog. Please fix on landing.

[Andrew Wilson]

Committed as r47078.