Bug 26932 – Add ENABLE(SHARED_WORKERS) flag and define SharedWorker APIs

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

[Andrew Wilson]

To support shared workers, we need to define an ENABLE(SHARED_WORKERS) flag and start refactoring the worker code to enable code sharing between the different flavors of workers.

[Andrew Wilson]

Created attachment 32196 [details]
proposed patch

[Andrew Wilson]

Created attachment 32199 [details]
proposed patch

Removed unnecessary braces from V8SharedWorkerCustom

[David Levin]

Comment on attachment 32199 [details]
proposed patch

This looks good in general.  There are a few things to address at the moment, but it is looking good.


> diff --git a/LayoutTests/fast/workers/shared-worker-constructor.html-disabled b/LayoutTests/fast/workers/shared-worker-constructor.html-disabled
> +try {
> +    new SharedWorker({toString:function(){throw "exception"}}, "name")
> +    log("FAIL: toString exception not propagated.");

According to http://www.whatwg.org/specs/web-workers/current-work/#runtime-script-errors,

"Whenever a runtime script error occurs in one of the worker's scripts, if the error did not occur while handling a previous script error, the user agent must report the error using the WorkerGlobalScope object's onerror attribute.

For shared workers, if the error is still not handled afterwards, or if the error occurred while handling a previous script error, the error should be reported to the user."

So it sounds this the exception should not be propogated.


> +try {
> +    var foo = {toString:function(){new Worker(foo)}}
> +    new SharedWorker(foo, name);
> +    log("FAIL: no exception when trying to create workers recursively");

Same comment as above.


> diff --git a/WebCore/DerivedSources.make b/WebCore/DerivedSources.make
> +webcore_built_sources += \
> +	WebCore/bindings/js/JSAbstractWorkerCustom.cpp \
> +	WebCore/bindings/js/JSSharedWorkerConstructor.cpp \
> +	WebCore/bindings/js/JSSharedWorkerCustom.cpp \
> +	WebCore/workers/AbstractWorker.cpp \
> +	WebCore/workers/AbstractWorker.h \
> +	WebCore/workers/SharedWorker.cpp \
> +	WebCore/workers/SharedWorker.h

You're missing JSSharedWorkerConstructor.h in the above list.

> diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro

This seems to be missing a line like this one:
    !contains(DEFINES, ENABLE_WORKERS=.): DEFINES += ENABLE_WORKERS=1

> +    SOURCES += \
> +        bindings/js/JSAbstractWorkerCustom.cpp \
> +        bindings/js/JSSharedWorkerConstructor.cpp \
> +        bindings/js/JSSharedWorkerCustom.cpp \
> +        workers/AbstractWorker.cpp \
> +        workers/AbstractWorker.h \
> +        workers/SharedWorker.cpp \
> +        workers/SharedWorker.h

As you noted (well) before I did, the *.h need to be removed for this file.

> +
> +

It looks like there are typically not blank lines at this point in other sections like this.
> +}


> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj
>  			isa = PBXGroup;
>  			children = (
> +				2E4346350F546A8200B0F1BA /* Worker.idl */,

"Worker.idl" is already in this list, so this addition seems like a mistake.

>  				2E4346330F546A8200B0F1BA /* Worker.cpp */,
>  				2E4346340F546A8200B0F1BA /* Worker.h */,
>  				2E4346350F546A8200B0F1BA /* Worker.idl */,


> diff --git a/WebCore/bindings/js/JSAbstractWorkerCustom.cpp b/WebCore/bindings/js/JSAbstractWorkerCustom.cpp
> +#include "JSDOMGlobalObject.h"
> +#include "JSEventListener.h"
> +#include "AbstractWorker.h"

Sort these alphabetically.

Of course this file looks exceedingly similar to bindings/js/JSWorkerCustom.cpp, but it sounds like that is on your roadmap to clean that up.

Why not file a bug now about this so it is tracked?


> diff --git a/WebCore/bindings/js/JSEventTarget.cpp b/WebCore/bindings/js/JSEventTarget.cpp

> +#if ENABLE(SHARED_WORKERS)
> +    if (AbstractWorker* abstractWorker = target->toAbstractWorker())
> +        return toJS(exec, abstractWorker);
> +#endif
...
> +#if ENABLE(SHARED_WORKERS)
> +    CONVERT_TO_EVENT_TARGET(AbstractWorker)
> +#endif

Why aren't these SharedWorker instead of AbstractWorker?


> diff --git a/WebCore/bindings/js/JSSharedWorkerConstructor.cpp b/WebCore/bindings/js/JSSharedWorkerConstructor.cpp
> +#include "JSDOMWindowCustom.h"
> +#include "JSSharedWorker.h"
> +#include "SharedWorker.h"

Sort these headers.

> +JSSharedWorkerConstructor::JSSharedWorkerConstructor(ExecState* exec, JSDOMGlobalObject* globalObject)
> +    : DOMObject(JSSharedWorkerConstructor::createStructure(exec->lexicalGlobalObject()->objectPrototype()))
> +{
> +    putDirect(exec->propertyNames().prototype, JSSharedWorkerPrototype::self(exec, globalObject), None);

This file looks ok, but I'm weak on JSC bindings, so it would be great to get this file glanced at by someone who knows it better (for example ap, olliej, weinig, ...).

That being said the JSWorkerConstructor has the following line which you omitted:

    putDirect(exec->propertyNames().length, jsNumber(exec, 1), ReadOnly|DontDelete|DontEnum);

Why?  (To be honest I don't know why this line is necessary in that other file)


> diff --git a/WebCore/dom/EventTarget.h b/WebCore/dom/EventTarget.h
> +    class SharedWorker;

Sorting is done case sensitive so SharedWorker comes after ScriptExecutionContext. (ScriptExecutionContext comes after SVGElementInstance because ascii('c') > ascii('V')).

>      class SVGElementInstance;
>      class ScriptExecutionContext;


> diff --git a/WebCore/workers/AbstractWorker.cpp b/WebCore/workers/AbstractWorker.cpp
> +void AbstractWorker::dispatchScriptErrorEvent(const String&, const String&, int)
> +{
> +    //FIXME(atwilson): Generate an ErrorEvent instead of a simple event

FIXME's in WebKit don't have names in them.  If people want to know who added it, they can use the revision history.


> diff --git a/WebCore/workers/SharedWorker.h b/WebCore/workers/SharedWorker.h
> +        SharedWorker(const String&, const String&, ScriptExecutionContext*, ExceptionCode&);

I would keep the parameter names for the two strings here.

[Andrew Wilson]

(In reply to comment #3)
> 
> For shared workers, if the error is still not handled afterwards, or if the
> error occurred while handling a previous script error, the error should be
> reported to the user."
> 
> So it sounds this the exception should not be propogated.
> 

That part of the spec is dealing with exceptions thrown by worker script. In
this test, the exception is thrown by the page script (in the "toString()"
method invoked to generate the params passed to the Worker constructor. This
test is correct.

> 
> > +try {
> > +    var foo = {toString:function(){new Worker(foo)}}
> > +    new SharedWorker(foo, name);
> > +    log("FAIL: no exception when trying to create workers recursively");
> 
> Same comment as above.

Same thing as above - this is page script throwing an exception, so it should
be propagated up.


> 
> You're missing JSSharedWorkerConstructor.h in the above list.

Fixed.

> 
> > diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> 
> This seems to be missing a line like this one:
>     !contains(DEFINES, ENABLE_WORKERS=.): DEFINES += ENABLE_WORKERS=1
> 

Good catch. I totally missed that.


> > diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj
> >  			isa = PBXGroup;
> >  			children = (
> > +				2E4346350F546A8200B0F1BA /* Worker.idl */,
> 
> "Worker.idl" is already in this list, so this addition seems like a mistake.

Yeah, it's weird. I used xcode itself to make these modifications, so I'm
confused about why this was added. I've removed it.


> 
> Why not file a bug now about this so it is tracked?
> 

Done: https://bugs.webkit.org/show_bug.cgi?id=26948

> Why aren't these SharedWorker instead of AbstractWorker?

They are intentionally AbstractWorker. Long-term we'll get rid of toWorker()
also and just have toAbstractWorker() since that's the common base class. 

> 
> 
> > diff --git a/WebCore/bindings/js/JSSharedWorkerConstructor.cpp b/WebCore/bindings/js/JSSharedWorkerConstructor.cpp
> > +#include "JSDOMWindowCustom.h"
> > +#include "JSSharedWorker.h"
> > +#include "SharedWorker.h"
> 
> Sort these headers.

Aren't they already sorted?

> 
> 
> This file looks ok, but I'm weak on JSC bindings, so it would be great to get
> this file glanced at by someone who knows it better (for example ap, olliej,
> weinig, ...).

It's using Worker as a template, which ap wrote, so I'm assuming it's correct,
but who knows? :) The Constructor code is kinda dense.

> 
> That being said the JSWorkerConstructor has the following line which you
> omitted:
> 
>     putDirect(exec->propertyNames().length, jsNumber(exec, 1),
> ReadOnly|DontDelete|DontEnum);
> 

I thought that was a copy/paste bug in the Worker constructor, but maybe I'm
wrong? I don't see that in JSXMLHttpRequestConstructor.cpp. I'll ping the folks
on webkit-dev about it.

I addressed the other comments as well.

[Andrew Wilson]

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

Also added the "length" property to the constructor with appropriate comments, per maciej's email on the webkit-dev mailing-list.

[David Levin]

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

This looks great.  This could be landed as is (if possible to address the nits it would be great -- but it could be landed as is).


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        * bindings/js/JSEventTarget.cpp:
> +        (WebCore::toJS):
> +        (WebCore::toEventTarget):
> +        Added support for converting to/from AbstractWorkers.
> +        * bindings/js/JSSharedWorkerConstructor.cpp: Added.
> +       (WebCore::JSSharedWorkerConstructor::JSSharedWorkerConstructor):

This one got out of place by one space.

> +        (WebCore::CALLBACK_FUNC_DECL):
> +        Custom constructor for SharedWorker.
> +       * dom/EventTarget.cpp:

This one got out of place by one space.

> diff --git a/WebCore/bindings/js/JSEventTarget.cpp b/WebCore/bindings/js/JSEventTarget.cpp

> +#if ENABLE(SHARED_WORKERS)
> +#include "JSAbstractWorker.h"
> +#include "AbstractWorker.h"

Nice to sort these.

> diff --git a/WebCore/workers/AbstractWorker.cpp b/WebCore/workers/AbstractWorker.cpp
> +#endif  // ENABLE(SHARED_WORKERS)

Only one space before end of line comments.

[Andrew Wilson]

Created attachment 32242 [details]
Patch with final style/whitespace tweaks

OK, addressed the whitespace/include sorting tweaks.

[Andrew Wilson]

There's one more issue I need to deal with on this patch - I need a custom toJS handler on AbstractWorker, to make sure I generate the correct wrapper object based on the type of m_impl.

[Eric Seidel (no email)]

bugzilla-tool isn't smart enough yet to land a patch other than the one which is r+'d :(

[Andrew Wilson]

Created attachment 32403 [details]
proposed patch with a fix to AbstractWorker::toJS()

While working on a patch based on this patch, I found a bug in how AbstractWorker::toJS() worked, so I added a custom toJS() function in AbstractWorker.

Also fixed JSSharedWorkerConstructor.cpp to use the proper global object when constructing the prototype chain.

[Andrew Wilson]

Created attachment 32592 [details]
Patch including changes to newly-upstreamed gypi files, and a couple of layout test "expected.txt" files I missed.

Code base keeps changing underneath me - updated patch to reflect latest revision.

[Andrew Wilson]

Created attachment 32593 [details]
More style fixups per levin's review.

[Andrew Wilson]

Created attachment 32614 [details]
Removed test expectations that were incorrectly generated with non-default flags.

[David Levin]

Comment on attachment 32614 [details]
Removed test expectations that were incorrectly generated with non-default flags.

> While working on a patch based on this patch, I found a bug in how
> AbstractWorker::toJS() worked, so I added a custom toJS() function in
> AbstractWorker.

It would have been nice to fix this issue while adding a layout tests that caught the issue, but it sounds like your later patch will have a layout test which catches this.

This looks fine.  I have a few very minor nits which I'll address while landing.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        (WebCore::AbstractWorker::AbstractWorker):
> +        Common baseclass for SharedWorker and (soon) Worker. The functions below were copied from Worker.cpp.

s/baseclass/base class/


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

Typically a blank line is after the header for the cpp file.



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

> +
> +#if ENABLE(SHARED_WORKERS)
> +
> +#include "AtomicStringHash.h"
> +#include "ActiveDOMObject.h"

Out of order.

[David Levin]

Committed as http://trac.webkit.org/changeset/45795