Bug 27927 – SharedWorkers should instantiate a thread and a SharedWorkerContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

SharedWorkers need to create a worker thread and context per the spec.

[Andrew Wilson]

Created attachment 33955 [details]
proposed patch

In a future patch I will refactor some of the existing dedicated worker tests to cover shared workers also (I've done a proof of concept for one of the worker files in this patch).

There are no V8 bindings yet, as Chromium does not support cross-process MessagePorts, and I wanted to keep this patch reasonably sized. I will add them in a future patch - in the meantime, Chromium will no longer compile with SHARED_WORKERS=true.

[Andrew Wilson]

Created attachment 34118 [details]
Patch addressing levin's comments

OK, this addresses the first batch of comments.

[David Levin]

Here were my initial comments that I had emailed Drew directly.  I'll paste his responses next:

I think you need to fix up the blk build file as well.

Ed. Note: Drew pointed out that the other worker files were not in there either.

> diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi
> +	    'bindings/js/JSSharedWorkerConstructor.cpp',
> +            'bindings/js/JSSharedWorkerConstructor.h',
> +	    'bindings/js/JSSharedWorkerContextCustom.cpp',
> +	    'bindings/js/JSSharedWorkerCustom.cpp',

TABs -- my eyes hurt.

> diff --git a/WebCore/bindings/js/JSSharedWorkerConstructor.cpp b/WebCore/bindings/js/JSSharedWorkerConstructor.cpp
...
> -    // FIXME: toJS() creates an object whose prototype is derived from lexicalGlobalScope, which is incorrect (Bug 27088)
> -    return asObject(toJS(exec, worker.release()));
> +    return asObject(toJS(exec, jsConstructor->globalObject(), worker.release()));

Do any of the  tests that you added cover this? (It seems subtle so it seems easy to break and not notice without a layout test.)




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

> +KURL AbstractWorker::resolveURL(const String& url, ExceptionCode& ec)
> +{
> +    if (url.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return KURL();
> +    }
> +
> +    // FIXME: This should use the dynamic global scope (bug #27887)
> +    KURL scriptURL = scriptExecutionContext()->completeURL(url);
> +    if (!scriptURL.isValid()) {
> +        ec = SYNTAX_ERR;
> +        return scriptURL;

I would prefer that in case of error it just "return KURL();" so that the url isn't used accidentally.  (It doesn't appear that the url is ever used in this case right now so this should be fine.)

> +    }
> +
> +    if (!scriptExecutionContext()->securityOrigin()->canAccess(SecurityOrigin::create(scriptURL).get())) {
> +        ec = SECURITY_ERR;

Same error comment about doing "return KURL();"  here.


> diff --git a/WebCore/workers/AbstractWorker.h b/WebCore/workers/AbstractWorker.h
>  #include "EventListener.h"
>  #include "EventTarget.h"
> +#include "KURL.h"

It looks like a forward declaraction would suffice here.


> diff --git a/WebCore/workers/DedicatedWorkerContext.cpp b/WebCore/workers/DedicatedWorkerContext.cpp
> +void DedicatedWorkerContext::logException(const String& errorMessage, int lineNumber, const String& sourceURL)
>  {
> +    thread()->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);

Doesn't postExceptionToWorkerObject actually do more than log the exception now?  (I think it postes an exception to the WorkerObject.)

> diff --git a/WebCore/workers/SharedWorkerContext.cpp b/WebCore/workers/SharedWorkerContext.cpp
> +void SharedWorkerContext::dispatchConnect(PassRefPtr<MessagePort> port)
> +{
> +    // Since close() stops the thread event loop, this should not ever get called while closing.
> +    ASSERT(!isClosing());

> +    // The connect event uses the MessageEvent interface, but has the name "connect".

Why not just an Event?

> +    RefPtr<Event> evt = MessageEvent::create("", "", "", 0, port);

Use event instead of evt.


> diff --git a/WebCore/workers/SharedWorkerRepository.h b/WebCore/workers/SharedWorkerRepository.h

> +    typedef int ExceptionCode;

This works but I would really just #include "ExceptionCode.h" instead of having the typedef here again as it makes changing this type more painful.

> +        // Static factory for getting the browser-specific repository implementation.
> +        static SharedWorkerRepository* instance();
Is this always non-0? If so, consider returning a SharedWorkerRepository&.

> +        ~SharedWorkerRepository() {}

Nice to add space { }


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

> +class SharedWorkerLoader : public RefCounted<SharedWorkerLoader>, public ActiveDOMObject, private WorkerScriptLoaderClient {
> +public:
> +    SharedWorkerLoader(PassRefPtr<SharedWorker>, PassOwnPtr<MessagePortChannel>, const String&);
> +    void load(const KURL& url);

No need to param name "url" here.

> diff --git a/WebCore/workers/SharedWorkerRepositoryImpl.h b/WebCore/workers/SharedWorkerRepositoryImpl.h

> +#include <wtf/Vector.h>
"<wtf/Vector.h>" doesn't appear needed in here.

> +    class SharedWorkerProxy;

SharedWorkerProxy doesn't appear needed in here.

> +
> +    // Platform-specific implementation of the SharedWorkerRepository singleton.
> +    class SharedWorkerRepositoryImpl : public SharedWorkerRepository, Noncopyable {

Consider DefaultSharedWorkerRepository or PlatformSharedWorkerRepository or SingleProcessSharedWorkerRepository.
That last one is a bit long but I think also the most precise.

> +        static SharedWorkerRepositoryImpl* instance();

Is it ever 0? If not,...

> +}

"} // namespace WebCore"

> +
> +#endif // ENABLE(WORKERS)

Should be #endif // ENABLE(SHARED_WORKERS)


> diff --git a/WebCore/workers/SharedWorkerThread.h b/WebCore/workers/SharedWorkerThread.h
> +    protected:
> +        virtual PassRefPtr<WorkerContext> createWorkerContext(const KURL& url, const String& userAgent);

"url" name not needed.

> +#endif // ENABLE(WORKERS)

Should be #endif // ENABLE(SHARED_WORKERS)


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

I think you can remove the "#include "SecurityOrigin.h"" from this file now.

> diff --git a/WebCore/workers/WorkerContext.cpp b/WebCore/workers/WorkerContext.cpp
> +void WorkerContext::reportException(const String& errorMessage, int lineNumber, const String& sourceURL)
...
> +    if (!errorHandled)
> +        logException(errorMessage, lineNumber, sourceURL);

This should be

      if (!errorHandled)
          thread()->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);

which actually fires the exception on the workerobject.

> +}
> +
> +

Two returns?... Whitespace is money! :)

>  } // namespace WebCore


> diff --git a/WebCore/workers/WorkerContext.h b/WebCore/workers/WorkerContext.h
>  namespace WebCore {
>  
>      class ScheduledAction;
> +    class SharedWorkerContext;
> +    class DedicatedWorkerContext;
I don't think these need to be here (probably a left over from another change).  If they do belong here, then sort them :).

[David Levin]

> -    // FIXME: toJS() creates an object whose prototype is derived from lexicalGlobalScope, which is incorrect (Bug 27088)
> -    return asObject(toJS(exec, worker.release()));
> +    return asObject(toJS(exec, jsConstructor->globalObject(), worker.release()));

| Do any of the  tests that you added cover this? (It seems subtle so it seems easy to break and not notice without a layout test.)
| There is test code in the regular global constructor tests, although not enabled for SharedWorkers since they are disabled by default.

I haven't added more tests because the current behavior for both Workers and SharedWorkers is *still* broken (albeit in a different, subtle way) - I logged bug #27887 to track that issue (we need to do URL resolution with a different scope than we use to do the security checks). I figured I'd add extensive tests at that time.
 

> diff --git a/WebCore/workers/DedicatedWorkerContext.cpp b/WebCore/workers/DedicatedWorkerContext.cpp
> +void DedicatedWorkerContext::logException(const String& errorMessage, int lineNumber, const String& sourceURL)
>  {
> +    thread()->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);

| Doesn't postExceptionToWorkerObject actually do more than log the exception now?  (I think it postes an exception to the WorkerObject.)

That is correct. I've renamed logException to forwardException, as I'm assuming that's what you're getting at here.
 

> diff --git a/WebCore/workers/SharedWorkerContext.cpp b/WebCore/workers/SharedWorkerContext.cpp
> +void SharedWorkerContext::dispatchConnect(PassRefPtr<MessagePort> port)
> +{
> +    // Since close() stops the thread event loop, this should not ever get called while closing.
> +    ASSERT(!isClosing());

> +    // The connect event uses the MessageEvent interface, but has the name "connect".

| Why not just an Event?

Because that's what the spec says:
"Create an event that uses the MessageEvent interface, with the name connect, "
 

> +    typedef int ExceptionCode;

| This works but I would really just #include "ExceptionCode.h" instead of having the typedef here again as it makes changing this type more painful.

OK, I've changed this, but note that there are 39 instances of "typedef int ExceptionCode;" in header files, while only 12 instances of "#include ExceptionCode.h" - I was just following the more popular pattern.
 

> +        // Static factory for getting the browser-specific repository implementation.
> +        static SharedWorkerRepository* instance();

| Is this always non-0? If so, consider returning a SharedWorkerRepository&.

instance() is going away in my next patch, so if it's OK I'd like to leave it as-is.
 

> +    // Platform-specific implementation of the SharedWorkerRepository singleton.
> +    class SharedWorkerRepositoryImpl : public SharedWorkerRepository, Noncopyable {

| Consider DefaultSharedWorkerRepository or PlatformSharedWorkerRepository or SingleProcessSharedWorkerRepository.
| That last one is a bit long but I think also the most precise.

Good point. I'd like to go with "Default" as a nice balance between length and precision (and also matches what we did with MessagePort).
 

> +        static SharedWorkerRepositoryImpl* instance();

| Is it ever 0? If not,...

This is being refactored in my next patch so I'd like to change it there instead, if that's OK.

...
| This should be
|     if (!errorHandled)
|          thread()->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);
| which actually fires the exception on the workerobject.


I think the code was correct, although I've renamed logException -> forwardException to make it clearer that the exception is still going to the parent context.

[David Levin]

Comment on attachment 34118 [details]
Patch addressing levin's comments

Just a few last things to fix up....

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added disabled tests for SharedWorker minimal functionality.
> +
> +        Began refactoring of dedicated worker tests to allow sharing test cases between shared and dedicated worker (will continue in another patch).
> +
> +        SharedWorkers should instantiate a thread and a SharedWorkerContext
> +        https://bugs.webkit.org/show_bug.cgi?id=27927

The format du jour is to put these two lines after the Reviewed By line and then the description.


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

I see an extra </html> here but I think that you removed it in the next patch.

> +++ b/LayoutTests/fast/workers/shared-worker-replace-global-constructor.html-disabled
> +</html>
I see an extra </html> here but I think that you removed it in the next patch.


> diff --git a/LayoutTests/fast/workers/shared-worker-simple.html-disabled b/LayoutTests/fast/workers/shared-worker-simple.html-disabled
> +</html>
Extra </html> here but I think that you removed it in the next patch.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Drew Wilson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Created first working implementation of SharedWorkers (execution only, no sharing).
> +        Added initial implementations of SharedWorkerThread and SharedWorkerContext.
> +        No v8 bindings yet.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27927


The current preferred format is to put the bug title and then bg link right after the review by line.

prepare-ChangeLog --bug #

will do this for you.

> diff --git a/WebCore/bindings/js/JSEventTarget.cpp b/WebCore/bindings/js/JSEventTarget.cpp
>  #if ENABLE(SHARED_WORKERS)
>  #include "JSSharedWorker.h"
> +#include "JSSharedWorkerContext.h"
>  #include "SharedWorker.h"
> +#include "SharedWorkerContext.h"
>  #endif

Since you've ifdef'ed the headers, can you remove the ifdef's here (and then sort them appropriately)?  (Less ifdef cruft is nice.)

> diff --git a/WebCore/bindings/js/WorkerScriptController.cpp b/WebCore/bindings/js/WorkerScriptController.cpp
> +#if ENABLE(SHARED_WORKERS)
> +#include "JSSharedWorkerContext.h"
> +#endif

Since you've ifdef'ed the headers, can you remove the ifdef's here?

> diff --git a/WebCore/bindings/v8/DOMObjectsInclude.h b/WebCore/bindings/v8/DOMObjectsInclude.h
> index 32354b3..ddfd629 100644
> --- a/WebCore/bindings/v8/DOMObjectsInclude.h
> +++ b/WebCore/bindings/v8/DOMObjectsInclude.h
> @@ -209,6 +209,7 @@
>  #if ENABLE(SHARED_WORKERS)
>  #include "AbstractWorker.h"
>  #include "SharedWorker.h"
> +#include "SharedWorkerContext.h"
>  #endif // SHARED_WORKERS

Since you've ifdef'ed the headers, can you remove the ifdef's here?

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

> +void DefaultSharedWorkerRepository::workerScriptLoaded(const String& name, const KURL& url, const String& userAgent, const String& workerScript, PassOwnPtr<MessagePortChannel> port)
> +{
> +    // FIXME: Cache the proxy to allow sharing and to allow cleanup after the thread exits.
> +    SharedWorkerProxy *proxy = new SharedWorkerProxy();

SharedWorkerProxy*  (* on the wrong side...You may be glad to know that there is a patch in a bug to check-webkit-style to catch but it is s\
till undergoing some fixes.)

Would you add a comment about what owns the SharedWorkerProxy (because it looks like it leaks in this code)?


> +void SharedWorkerRepository::connect(PassRefPtr<SharedWorker> worker, PassOwnPtr<MessagePortChannel> port, const KURL& url, const String& name, ExceptionCode&)
> +{
> +    // Create a loader to load/initialize the requested worker
Add "."

> +    RefPtr<SharedWorkerLoader> loader = adoptRef(new SharedWorkerLoader(worker, port.release(), name));

I think "port" alone should suffice here.


> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.h b/WebCore/workers/DefaultSharedWorkerRepository.h
> +        // Invoked once the worker script has been loaded to fire up the worker thread.
> +        void workerScriptLoaded(const String& name, const KURL& url, const String& userAgent, const String& workerScript, PassOwnPtr<MessagePortChannel>);

No need to param name "url" here.

> diff --git a/WebCore/workers/SharedWorkerContext.idl b/WebCore/workers/SharedWorkerContext.idl
> +    ] SharedWorkerContext : WorkerContext {
> +
> +        readonly attribute DOMString name;
> +        attribute EventListener onconnect;

The typical format is to indent attribute to leave spaces for "readonly" when it isn't there.

[Andrew Wilson]

(In reply to comment #5)
> (From update of attachment 34118 [details])
> Just a few last things to fix up....
> 
> > diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Added disabled tests for SharedWorker minimal functionality.
> > +
> > +        Began refactoring of dedicated worker tests to allow sharing test cases between shared and dedicated worker (will continue in another patch).
> > +
> > +        SharedWorkers should instantiate a thread and a SharedWorkerContext
> > +        https://bugs.webkit.org/show_bug.cgi?id=27927
> 
> The format du jour is to put these two lines after the Reviewed By line and
> then the description.
> 

Tragically, the git version of prepare-ChangeLogs still puts all of the commit descriptions above the bug summary. But I'll keep this in mind going forward.

> 
> > diff --git a/LayoutTests/fast/workers/shared-worker-gc.html-disabled b/LayoutTests/fast/workers/shared-worker-gc.html-disabled
> > +</html>
> 
> I see an extra </html> here but I think that you removed it in the next patch.

Yes, inexplicably there are a ton of extra </html> tags in the worker tests (yay, copy and paste). I'm cleaning the others up as we go along, but I'll fix these up now since I'm making a new patch anyway.

>
> 
> > diff --git a/WebCore/bindings/js/JSEventTarget.cpp b/WebCore/bindings/js/JSEventTarget.cpp
> >  #if ENABLE(SHARED_WORKERS)
> >  #include "JSSharedWorker.h"
> > +#include "JSSharedWorkerContext.h"
> >  #include "SharedWorker.h"
> > +#include "SharedWorkerContext.h"
> >  #endif
> 
> Since you've ifdef'ed the headers, can you remove the ifdef's here (and then
> sort them appropriately)?  (Less ifdef cruft is nice.)

Done. BTW, nobody else does this for any of the other ifdefs in that file (ENABLE_WORKERS, ENABLE_SVG, ENABLE_OFFLINE).


> Would you add a comment about what owns the SharedWorkerProxy (because it looks
> like it leaks in this code)?
> 
It does leak. Hence the FIXME comment on line 146.

> The typical format is to indent attribute to leave spaces for "readonly" when
> it isn't there.
Awesome. I always wondered why there were seemingly random indentations for some of the attributes.

[Andrew Wilson]

Created attachment 34144 [details]
Hopefully final patch addressing remaining comments

[David Levin]

Comment on attachment 34144 [details]
Hopefully final patch addressing remaining comments

In WebCore/bindings/v8/DOMObjectsInclude.h, there is an empty ifdef.  Please fix this on landing.

[Andrew Wilson]

Created attachment 34156 [details]
Final patch ready for landing

[David Levin]

Comment on attachment 34156 [details]
Final patch ready for landing

Removed commit-queue+. I think Drew wants to land this himself.

[Andrew Wilson]

Committed as r46845