Closed Bug 643325 Opened 13 years ago Closed 11 years ago

Review Web Workers draft and implement SharedWorkers

Categories

(Core :: DOM: Core & HTML, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
relnote-firefox --- -

People

(Reporter: smaug, Assigned: bent.mozilla)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 7 obsolete files)

There seems to be still some small problems in the spec like
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12340

But we should still implemented SharedWorkers, IMO.
They give nice cross-same-origin-windows communication.
IIUC, this would also involve implementing MessagePorts, which we don't seem to have now (http://www.whatwg.org/specs/web-apps/current-work/#messageport).  Bug 677638, which is asking for the MessageChannel API also needs ports.  Should we create a new bug for MessagePort and have it block both this and bug 677638?  ISTM we could probably refactor the queue stuff from dom/workers into a MessagePort and have both dedicated and shared workers use them.
I think another bug for implementing MessagePort (including your remarks) is beneficial. Could you please open one and add it here?
I'm pretty sure there was a separate bug for implementing SharedWorkers but I can't find it. Has this bug been renamed?
(In reply to Florian Bender from comment #3)
> I'm pretty sure there was a separate bug for implementing SharedWorkers but
> I can't find it. Has this bug been renamed?

As far as I'm aware, this was the only bug for shared workers and is why I marked bug 741618 (message ports) as blocking this one.
Attached patch Patch, v1 (obsolete) — Splinter Review
This is a first stab at implementing SharedWorker. It implements a simplified version equivalent to what Chrome has (only supported on the main thread). It uses the new DOM bindings and can be disabled with a pref.

There is no support yet for JS components or JSMs, I'll add those in a separate bug. I also haven't added support for ChromeSharedWorker yet since I need bug 880367 for that.

This only includes one test. I'll obviously add a lot more (in the next day or so) but I wanted to get this patch up for review first since it's going to take a while to digest it.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #759988 - Flags: review?(khuey)
Keywords: dev-doc-needed
Attached patch Patch, v1 (obsolete) — Splinter Review
Now with several more tests.
Attachment #759988 - Attachment is obsolete: true
Attachment #759988 - Flags: review?(khuey)
Attachment #762833 - Flags: review?(khuey)
It seems that this patch can be applied to neither m-c (REV c6bca8768874) nor m-i (REV 50e5eb0c4008) now.
Ben, can you estimate when this will be done, i. e. are you blocked and for how long, and how much is there left to do here?
(In reply to Florian Bender from comment #8)
> Ben, can you estimate when this will be done, i. e. are you blocked and for
> how long, and how much is there left to do here?

It's just waiting on review.
Kyle, any estimation when you will get to this? 

I'm rooting for SharedWorkers to land in Fx 25 though I understand that we're late in the cycle …
Flags: needinfo?(khuey)
I am reviewing it now, but it's a big patch.
Flags: needinfo?(khuey)
Comment on attachment 762833 [details] [diff] [review]
Patch, v1

Review of attachment 762833 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, but I want to see it again.

::: dom/bindings/BindingDeclarations.h
@@ +63,5 @@
>    {
>      return !Get();
>    }
>  
> +  JSObject* GetJSObject() const

Should this be just JSObject()?  Can mGlobalJSObject be null?  Seems like no since this is a stack class created for entering code from JS.

::: dom/bindings/Bindings.conf
@@ +639,5 @@
>  },
>  
> +'MessagePort': [{
> +    'nativeType': 'mozilla::dom::workers::MessagePort',
> +    'nativeOwnership': 'refcounted',

'nativeOwnership': 'refcounted' is obsolete, please remove it.

@@ +817,5 @@
>  },
>  
> +'SharedWorker': {
> +    'nativeType': 'mozilla::dom::workers::SharedWorker',
> +    'nativeOwnership': 'refcounted',

here too

::: dom/interfaces/events/nsIDOMErrorEvent.idl
@@ +5,5 @@
> +
> +#include "nsIDOMEvent.idl"
> +
> +[scriptable, builtinclass, uuid(1acd8560-a496-470f-a29f-bd13d9ed0ead)]
> +interface nsIDOMErrorEvent : nsIDOMEvent

There's an 'error' property in the spec, why isn't it here?

::: dom/workers/MessagePort.h
@@ +80,5 @@
> +
> +    SetEventHandler(nsGkAtoms::onmessage, aCallback, aRv);
> +    if (!aRv.Failed()) {
> +      Start(aRv);
> +    }

OMG this is terrible.  At the very least the setter needs to swallow the "can't start twice" error so that you can set onmessage multiple times.  That needs a test too.  But having this work for the onmessage handler but not message listeners is nuts.

::: dom/workers/RuntimeService.cpp
@@ +1762,5 @@
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT(aData.get());
> +  MOZ_ASSERT(aUserArg);
> +
> +  WorkerPrivate* workerPrivate = static_cast<WorkerPrivate*>(aUserArg);

auto!

@@ +1783,5 @@
> +RuntimeService::FindSharedWorkerInfo(const nsACString& aKey,
> +                                     SharedWorkerInfo* aData,
> +                                     void* aUserArg)
> +{
> +  MatchSharedWorkerInfo* match = static_cast<MatchSharedWorkerInfo*>(aUserArg);

auto!

@@ +1831,4 @@
>      for (uint32_t index = 0; index < workers.Length(); index++) {
> +      WorkerPrivate*& worker = workers[index];
> +
> +      if (worker->IsSharedWorker()) {

Seems like everything in this if block should be a function on WorkerPrivate(Parent?).  CloseSharedWorkersForWindow, perhaps.

@@ +1935,5 @@
> +    SharedWorkerInfo* sharedWorkerInfo;
> +
> +    if (mDomainMap.Get(loadInfo.mDomain, &domainInfo) &&
> +        domainInfo->mSharedWorkerInfos.Get(scriptSpec, &sharedWorkerInfo) &&
> +        sharedWorkerInfo->mName == aName) {

We need to deal with CSP here.  I'm going to send mail to the working group.  I don't think it needs to block landing this but we probably shouldn't ship to release until it's sorted out :-/

::: dom/workers/RuntimeService.h
@@ +44,5 @@
> +    SharedWorkerInfo(WorkerPrivate* aWorkerPrivate,
> +                     const nsACString& aScriptSpec,
> +                     const nsAString& aName)
> +    : mWorkerPrivate(aWorkerPrivate), mScriptSpec(aScriptSpec), mName(aName)
> +    { }

MOZ_COUNT_CTOR/DTOR

@@ +59,5 @@
> +    WorkerDomainInfo()
> +    : mActiveWorkers(1), mChildWorkerCount(0)
> +    {
> +      mSharedWorkerInfos.Init();
> +    }

This too.

@@ +81,5 @@
> +    SharedWorkerInfo* mSharedWorkerInfo;
> +
> +    MatchSharedWorkerInfo(WorkerPrivate* aWorkerPrivate)
> +    : mWorkerPrivate(aWorkerPrivate), mSharedWorkerInfo(nullptr)
> +    { }

And this.

@@ +120,5 @@
>    // True when the observer service holds a reference to this object.
>    bool mObserved;
>    bool mShuttingDown;
>    bool mNavigatorStringsLoaded;
> +  bool mHoldingJSObjects;

This does not appear to be used ...

::: dom/workers/SharedWorker.cpp
@@ +141,5 @@
> +    NS_NewRunnableMethod(this, &SharedWorker::ResumeNow);
> +  if (NS_SUCCEEDED(NS_DispatchToCurrentThread(runnable))) {
> +    mResumeRunnable = runnable;
> +  } else {
> +    NS_WARNING("Failed to dispatch runnable, worker won't resume!");

The same comment I made below applies here too, I think.

@@ +169,5 @@
> +    nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewRunnableMethod(this, &SharedWorker::ReleaseWorker);
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Unable to defer release of SharedWorker!");
> +    }

Why is deferring the call to UnregisterWorker (and thus potentially WorkerPrivate::Cancel) ok here?  It means that CancelWorkersForWindow is going to cancel dedicated workers immediately and shared workers only after going through the event loop.

@@ +219,5 @@
> +
> +  if (!mSuspendedEvents.IsEmpty()) {
> +    for (uint32_t index = 0; index < mSuspendedEvents.Length(); index++) {
> +      nsCOMPtr<nsIRunnable> runnable =
> +        new SuspendedEventRunnable(this, mSuspendedEvents[index].forget());

You should have a single runnable that takes all of the suspended events.

::: dom/workers/Worker.cpp
@@ +138,5 @@
> +    nsString voidString;
> +    voidString.SetIsVoid(true);
> +
> +    JSObject* obj =
> +      Construct(aCx, parent, scriptURL, aIsChromeWorker, voidString);

So you have a function called Construct that calls ConstructInternal which calls a different function named Construct?  This needs some renaming.

::: dom/workers/WorkerPrivate.cpp
@@ +140,5 @@
>    dest->swap(rawSupports);
>  }
>  
> +void
> +LogErrorToConsole(const nsAString& aMessage,

This probably belongs in nsJSUtils or something.

@@ +1606,1 @@
>  }

Huzzah!

@@ +2068,5 @@
> +        AssertIsOnMainThread();
> +        MOZ_ASSERT(aSharedWorker);
> +        MOZ_ASSERT(aClosure);
> +
> +        Closure* closure = static_cast<Closure*>(aClosure);

auto closure = ...

\o/

@@ +2070,5 @@
> +        MOZ_ASSERT(aClosure);
> +
> +        Closure* closure = static_cast<Closure*>(aClosure);
> +
> +        nsRefPtr<SharedWorker> kungFuDeathGrip = aSharedWorker;

why is this necessary?

@@ +2086,5 @@
> +        return PL_DHASH_NEXT;
> +      }
> +    };
> +
> +    Closure closure = { aWindow, true };

Use a ctor instead of struct initialization please.

@@ +2125,5 @@
> +  MOZ_ASSERT(aCx);
> +  MOZ_ASSERT_IF(!IsSharedWorker(), mParentSuspended);
> +
> +  // Shared workers are resumed if any of their owning documents are
> +  // suspended.

are resumed?

@@ +2144,5 @@
> +        AssertIsOnMainThread();
> +        MOZ_ASSERT(aSharedWorker);
> +        MOZ_ASSERT(aClosure);
> +
> +        Closure* closure = static_cast<Closure*>(aClosure);

same

@@ +2146,5 @@
> +        MOZ_ASSERT(aClosure);
> +
> +        Closure* closure = static_cast<Closure*>(aClosure);
> +
> +        nsRefPtr<SharedWorker> kungFuDeathGrip = aSharedWorker;

same

@@ +2163,5 @@
> +        return PL_DHASH_NEXT;
> +      }
> +    };
> +
> +    Closure closure = { aWindow, false };

same

@@ +2218,2 @@
>  {
>    AssertIsOnParentThread();

This should really be AssertIsOnMainThread, no?

@@ +2513,5 @@
> +
> +  JS::Rooted<JS::Value> data(cx);
> +  if (!buffer.read(cx, data.address(), WorkerStructuredCloneCallbacks(true))) {
> +    return false;
> +  }

no exception?

::: dom/workers/WorkerPrivate.h
@@ +276,5 @@
> +
> +    LoadInfo()
> +    : mEvalAllowed(false), mReportCSPViolations(false),
> +      mXHRParamsAllowed(false), mPrincipalIsSystem(false)
> +    { }

This class needs MOZ_COUNT_CTOR/DTOR macros.

@@ +319,5 @@
>    // Protected by mMutex.
>    JSSettings mJSSettings;
>  
> +  // Only touched on the parent thread.
> +  nsDataHashtable<nsUint64HashKey, SharedWorker*> mSharedWorkers;

which is only ever the main thread.  worth mentioning imo.

@@ +682,5 @@
> +  bool
> +  IsSharedWorker() const
> +  {
> +    return !mSharedWorkerName.IsVoid();
> +  }

I know you think you're being clever here, but no.  Store a separate boolean for this.

::: dom/workers/moz.build
@@ +9,5 @@
>  MODULE = 'dom'
>  
>  # Public stuff.
>  EXPORTS.mozilla.dom.workers += [
> +    'Workers.h'

Don't remove the trailing comma.

@@ +29,3 @@
>      'XMLHttpRequest.h',
>      'XMLHttpRequestEventTarget.h',
> +    'XMLHttpRequestUpload.h'

Here too.
Attachment #762833 - Flags: review?(khuey) → review-
> ::: dom/workers/MessagePort.h
> @@ +80,5 @@
> > +
> > +    SetEventHandler(nsGkAtoms::onmessage, aCallback, aRv);
> > +    if (!aRv.Failed()) {
> > +      Start(aRv);
> > +    }
> 
> OMG this is terrible.  At the very least the setter needs to swallow the
> "can't start twice" error so that you can set onmessage multiple times. 
> That needs a test too.  But having this work for the onmessage handler but
> not message listeners is nuts.

I can't find any justification in the spec for making start throw when called multiple times, and it's not what webkit or IE do ...
(In reply to Florian Bender from comment #10)
> I'm rooting for SharedWorkers to land in Fx 25 though I understand that
> we're late in the cycle …

I think that's unlikely.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #14)
> I think that's unlikely.

Yeah, I figured that. Thanks anyway, good to see some progress here! :)
Will this bug also provide shared ChromeWorkers?
What is the benefit of a shared ChromeWorker?  You can just wrap your ChromeWorker in a jsm that lets people talk to it.
(In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> Will this bug also provide shared ChromeWorkers?

This patch in particular does not. It should be trivial to add though.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> You can just wrap your ChromeWorker in a jsm that lets people talk to it.

True, but the cost of enabling the same syntax for chrome and non-chrome is marginal. I don't see any reason why we shouldn't do it.
Sure, but it doesn't enable anything you can't do already, so I don't know why it's blocking bug 853460 or bug 881652.
Consider a hypothetical (but planned) reimplementation of Telemetry as a shared worker. Ideally, using this implementation of Telemetry from a chrome worker should be as simple as

let {StopWatch} = require("resource://gre/modules/workers/telemetry.js");
StopWatch.start(...);
StopWatch.stop(...);

If my understanding of shared workers is correct, this is trivial to do with a shared chrome worker.

By comparison, without shared chrome workers, we would need every single worker that needs to access Telemetry to explicitly implement a protocol for sending Telemetry messages from the worker to the main thread before routing these messages to the Telemetry worker. This is anti-modular, error-prone and inefficient.
I guess this bug is now blocked by Bug 677638?
Attached patch Patch, v1 (obsolete) — Splinter Review
This is just un-bitrotted for those flowing along. I haven't had a chance to look at khuey's comments yet.
Attachment #762833 - Attachment is obsolete: true
Attached patch Patch, v2 (obsolete) — Splinter Review
Merged and comments addressed (quibbles to follow!)
Attachment #796970 - Attachment is obsolete: true
Attachment #808066 - Flags: review?(khuey)
Attached patch Interdiff, v1 to v2 (obsolete) — Splinter Review
Changes from v1 to v2 (not including ridiculous merge gymnastics).
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> Should this be just JSObject()?

JSObject is a type and the compiler thinks you want to make a new one if you try.

> 'nativeOwnership': 'refcounted' is obsolete, please remove it.

Gone.

> There's an 'error' property in the spec, why isn't it here?

It was added after this work... And apparently we don't support it in DOM bindings yet. It'll need a followup I guess.

> OMG this is terrible.  At the very least the setter needs to swallow the
> "can't start twice" error so that you can set onmessage multiple times.

I can't tell if the spec want an exception for calling start() twice anyway (and Chrome doesn't...) so I just removed it. IMO it's better to throw for obvious programmer errors but meh.

> But having this work for the onmessage handler but
> not message listeners is nuts.

Let me know what you hear back from the mailing lists ;)

> auto!

/me is nervous that this is going to break someone... But ok.

> Seems like everything in this if block should be a function on
> WorkerPrivate(Parent?).  CloseSharedWorkersForWindow, perhaps.

Ok.

> We need to deal with CSP here.  I'm going to send mail to the working group.

Heard anything back?

> MOZ_COUNT_CTOR/DTOR

These are all private classes that are stored as members of classes with already-logged destructors so I don't think this really buys us anything... It's technically safer but in practice it just makes for uglier and more bug-prone code. I went ahead and added these so you can see which you like better. 

> This does not appear to be used ...

Bizarre, it's not in my merged patch. Some remnant from long ago I guess.

> The same comment I made below applies here too, I think.

No, Resume is special. We have to wait for the full event-queue to process otherwise some previously-suspended events may run out of order.

> Why is deferring the call to UnregisterWorker (and thus potentially
> WorkerPrivate::Cancel) ok here?

It was originally to avoid recursive hashtable munging but I reworked things so I think it's safe now.

> You should have a single runnable that takes all of the suspended events.

Fixed.

> So you have a function called Construct that calls ConstructInternal which
> calls a different function named Construct?  This needs some renaming.

Construct is a JS-ism, Create is now our C++-ism here.

> This probably belongs in nsJSUtils or something.

This doesn't really have anything to do with JS, it could go somewhere in XPCOM even. But there are lots of places in our code where messages are logged in the console... If they all need ot be unified somehow then it should be a followup.

> > +        nsRefPtr<SharedWorker> kungFuDeathGrip = aSharedWorker;
> 
> why is this necessary?

I'm only holding weak pointers to SharedWorker objects and each method I call could muck with the refcount.

> Use a ctor instead of struct initialization please.

Meh. Fine.

> are resumed?

Yep.

> This should really be AssertIsOnMainThread, no?

Fixed.

> > +  if (!buffer.read(cx, data.address(), WorkerStructuredCloneCallbacks(true))) {
> > +    return false;
>
> no exception?

The read() call sets exceptions if it fails.

> which is only ever the main thread.  worth mentioning imo.

Ok.

> I know you think you're being clever here, but no.  Store a separate boolean
> for this.

Just to be clear: you're saying it's somehow more clever to redundantly store the same information? I figured you'd probably argue and I don't care so much so I went ahead and changed this to the more un-clever approach ;)

> Don't remove the trailing comma.

Ok.
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #25)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> > Should this be just JSObject()?
> 
> JSObject is a type and the compiler thinks you want to make a new one if you
> try.

Bah.

> > OMG this is terrible.  At the very least the setter needs to swallow the
> > "can't start twice" error so that you can set onmessage multiple times.
> 
> I can't tell if the spec want an exception for calling start() twice anyway
> (and Chrome doesn't...) so I just removed it. IMO it's better to throw for
> obvious programmer errors but meh.

It's the web.  The web lets you fuck up as badly as you want.

> > But having this work for the onmessage handler but
> > not message listeners is nuts.
> 
> Let me know what you hear back from the mailing lists ;)

:-P

> > auto!
> 
> /me is nervous that this is going to break someone... But ok.

We're using it already.  It'll be fine.

> > We need to deal with CSP here.  I'm going to send mail to the working group.
> 
> Heard anything back?

I actually just sent the email today.  We can land without sorting this out but I think we need to figure it out before we ship.

> > MOZ_COUNT_CTOR/DTOR
> 
> These are all private classes that are stored as members of classes with
> already-logged destructors so I don't think this really buys us anything...
> It's technically safer but in practice it just makes for uglier and more
> bug-prone code. I went ahead and added these so you can see which you like
> better. 

Yeah, you win.  That shit is ugly.

> > The same comment I made below applies here too, I think.
> 
> No, Resume is special. We have to wait for the full event-queue to process
> otherwise some previously-suspended events may run out of order.

I don't understand this.  We call WorkerPrivateParent<Derived>::Resume after allowing the full event queue to process (this is what SynchronizeAndResume defers).  Why would we want to defer resuming SharedWorkers for another trip through the event loop?

> > I know you think you're being clever here, but no.  Store a separate boolean
> > for this.
> 
> Just to be clear: you're saying it's somehow more clever to redundantly
> store the same information? I figured you'd probably argue and I don't care
> so much so I went ahead and changed this to the more un-clever approach ;)

Haha, nice try only going half way.  Don't think I didn't spot that.
Comment on attachment 808066 [details] [diff] [review]
Patch, v2

Review of attachment 808066 [details] [diff] [review]:
-----------------------------------------------------------------

There are some irrelevant IndexedDB changes in here.  Please remove them.

r=me with comments addressed.  I know you like to think my review comments are optional, so I will back this out if you skip them.

::: dom/interfaces/events/nsIDOMErrorEvent.idl
@@ +1,1 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

You should not need this to generate an ErrorEvent impl.  The WebIDL file is sufficient.

::: dom/webidl/WorkerMessagePort.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +// XXX Remove me soon!
> +[PrefControlled]

Just use Func, PrefControlled is going away.

::: dom/webidl/moz.build
@@ +84,5 @@
>      'DragEvent.webidl',
>      'DummyBinding.webidl',
>      'DynamicsCompressorNode.webidl',
>      'Element.webidl',
> +    'ErrorEvent.webidl',

Stick this in GENERATED_EVENTS_WEBIDL_FILES or whatever it's called and magic will happen.

::: dom/workers/MessagePort.cpp
@@ +182,5 @@
> +      QueueEvent(event);
> +      preventDispatch = true;
> +    } else {
> +      preventDispatch = false;
> +    }

Just do = false in the decl and drop the else.

::: dom/workers/MessagePort.h
@@ +8,5 @@
> +
> +#include "Workers.h"
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/WorkerMessagePortBinding.h"

You should not need the FooBinding.h in the header.

::: dom/workers/SharedWorker.cpp
@@ +141,5 @@
> +
> +  nsRefPtr<nsRunnableMethod<SharedWorker> > runnable =
> +    NS_NewRunnableMethod(this, &SharedWorker::ResumeNow);
> +  if (NS_SUCCEEDED(NS_DispatchToCurrentThread(runnable))) {
> +    mResumeRunnable = runnable;

Like we talked about this morning, this is wrong.  WorkerPrivate::SynchronizeAndResume handles this for us.

@@ +211,5 @@
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Failed to dispatch suspended event!");
> +      mSuspendedEvents.Clear();
> +    }
> +  }

And this is double-extra wrong ;-)  A third trip through the event loop is completely unnecessary.

::: dom/workers/SharedWorker.h
@@ +8,5 @@
> +
> +#include "Workers.h"
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/SharedWorkerBinding.h"

Do you really need SharedWorkerBinding.h in the header here?  I doubt it.

@@ +10,5 @@
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/SharedWorkerBinding.h"
> +#include "nsDOMEventTargetHelper.h"
> +#include "nsThreadUtils.h"

I also doubt you need nsThreadUtils.h in this header.

::: dom/workers/WorkerPrivate.cpp
@@ +1983,2 @@
>  {
>    MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivateParent);

Please add MOZ_ASSERT_IF(IsSharedWorker(), !aObject).

@@ +2008,5 @@
> +
> +  if (!IsSharedWorker()) {
> +    SetIsDOMBinding();
> +    SetWrapper(aObject);
> +  }

I think you should override WrapObject and make it MOZ_CRASH.

@@ +2076,1 @@
>                 "Shouldn't have anything queued!");

Why are you relanding bug 823822?

@@ +2507,5 @@
> +  AssertIsOnMainThread();
> +
> +  JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());
> +  if (aTransferable.WasPassed()) {
> +    const Sequence<JS::Value >& realTransferable = aTransferable.Value();

nit: no space after Value.

@@ +2577,5 @@
> +  nsresult rv =
> +    event->InitMessageEvent(NS_LITERAL_STRING("message"), false, false, data,
> +                            EmptyString(), EmptyString(), nullptr);
> +  if (NS_FAILED(rv)) {
> +    xpc::Throw(cx, rv);

use mozilla::dom::Throw (in mozilla/dom/Exceptions.h)

@@ +2590,5 @@
> +
> +  bool ignored;
> +  rv = port->DispatchEvent(domEvent, &ignored);
> +  if (NS_FAILED(rv)) {
> +    xpc::Throw(cx, rv);

same here

@@ +2847,5 @@
> +    }
> +
> +    MOZ_ASSERT(cx);
> +
> +    AutoPushJSContext autoPush(cx);

bholley should probably look at this.

@@ +2851,5 @@
> +    AutoPushJSContext autoPush(cx);
> +
> +    if (NS_FAILED(rv)) {
> +      xpc::Throw(cx, rv);
> +      JS_ReportPendingException(cx);

again, mozilla::dom::Throw.  And idk enough about JSAPI to know if these JS_ReportPendingException calls are necessary.

@@ +2899,5 @@
> +    }
> +
> +    if (event->IsDispatchStopped()) {
> +      break;
> +    }

This is probably wrong.  Why would one SharedWorker be able to stop an error event from firing on SharedWorkers in a different window?  Where is this in the spec?

@@ +2903,5 @@
> +    }
> +  }
> +
> +  // Prune windows where preventDefault() was called.
> +  for (uint32_t index = 0; index < windowActions.Length(); /* no increment */) {

Why loop over this twice?  Just continue below if !mDefaultAction?

@@ +2907,5 @@
> +  for (uint32_t index = 0; index < windowActions.Length(); /* no increment */) {
> +    if (windowActions[index].mDefaultAction) {
> +      index++;
> +    } else {
> +      windowActions.RemoveElementAt(index);

I generally think you should avoid removing windows from the array since that could make this function O(N^2) in number of windows.

@@ +2944,5 @@
> +    if (NS_FAILED(rv)) {
> +      xpc::Throw(cx, rv);
> +      JS_ReportPendingException(cx);
> +
> +      status = nsEventStatus_eIgnore;

Why do you need to reset the status?  This is the default.

@@ +2948,5 @@
> +      status = nsEventStatus_eIgnore;
> +    }
> +
> +    if (status == nsEventStatus_eConsumeNoDefault) {
> +      windowActions.RemoveElementAt(index);

Here too.  Rather than remove elements from the array and resize it lets just set a boolean the first time we get something other than eConsumeNoDefault that tells us to LogErrorToConsole.

@@ +2955,5 @@
> +      index++;
> +    }
> +  }
> +
> +  // If any window declined default behavior then we're done.

s/any/every/ ...

@@ +3161,1 @@
>      }

nit: fix up indentation

@@ +3270,5 @@
> +    // still alive.  We may have received control messages initiating shutdown.
> +    {
> +      MutexAutoLock lock(aParent->mMutex);
> +      parentStatus = aParent->mStatus;
> +  }

nit: fix up indentation

@@ +3374,1 @@
>              }

nit: more wonky indentation

@@ +3379,2 @@
>            }
>          }

here too

@@ +3435,1 @@
>      }

here too

@@ +4980,5 @@
> +
> +WorkerCrossThreadDispatcher*
> +WorkerPrivate::GetCrossThreadDispatcher()
> +{
> +  mozilla::MutexAutoLock lock(mMutex);

Do you really need the mozilla:: qualification here?

@@ +4992,5 @@
> +WorkerPrivate::BeginCTypesCall()
> +{
> +  AssertIsOnWorkerThread();
> +
> +  MutexAutoLock lock(mMutex);

Cause you apparently don't here ...

@@ +5126,1 @@
>      bool current;

more weird indentation

::: dom/workers/WorkerPrivate.h
@@ +336,5 @@
>  
>  protected:
> +  WorkerPrivateParent(JSContext* aCx, JS::Handle<JSObject*> aObject,
> +                      WorkerPrivate* aParent, const nsAString& aScriptURL,
> +                      bool aIsChromeWorker, const nsAString& aSharedWorkerName,

Nice try, pass in bool aIsSharedWorker too.

::: dom/workers/test/multi_sharedWorker_frame.html
@@ +8,5 @@
> +    <title>Test for SharedWorker</title>
> +  </head>
> +  <body>
> +    <script type="text/javascript;version=1.7">
> +"use strict";

Can you indent this (and the rest of your test files)?

::: js/xpconnect/src/event_impl_gen.conf.in
@@ +51,5 @@
>      'RecordErrorEvent',
>  #ifdef MOZ_WEBSPEECH
>      'SpeechRecognitionEvent',
>  #endif
> +    'ErrorEvent',

This can go too.
Attachment #808066 - Flags: review?(khuey) → review+
> r=me with comments addressed.  I know you like to think my review comments
> are optional, so I will back this out if you skip them.

Haha!
Attached patch Patch, v3 (obsolete) — Splinter Review
With most comments addressed.
Attachment #808066 - Attachment is obsolete: true
Attachment #808067 - Attachment is obsolete: true
Attachment #812295 - Flags: review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #27)
> Just use Func, PrefControlled is going away.

It causes build bustage due to the fact that worker headers are exported into a different location than the func parser expects. I'll file a followup on it.

> bholley should probably look at this.

I'll flag him for feedback but he's travelling (according to the DOM  calendar). Since this is preffed off by default I don't think we risk anything by landing now and letting him look over it after.

> And idk enough about JSAPI to know if these
> JS_ReportPendingException calls are necessary.

They are because there can only be one pending exception at a time and I want all of them to be reported.

> nit: fix up indentation

hg rebase is soooooo bad...
Attached patch Patch, v3Splinter Review
Bobby, can you look over the JSContext usage in WorkerPrivate.cpp, specifically BroadcastErrorToSharedWorkers()?
Attachment #812295 - Attachment is obsolete: true
Attachment #812305 - Flags: review+
Attachment #812305 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 812305 [details] [diff] [review]
Patch, v3

Review of attachment 812305 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +2853,5 @@
> +    }
> +
> +    MOZ_ASSERT(cx);
> +
> +    AutoPushJSContext autoPush(cx);

You should probably use AutoCxPusher here. AutoPushJSContext means "push this cx if it isn't already stack-top". This is fine if you just want to make sure that the cx you're using is on the stack, and have a pretty good reason to believe that it will be. But it doesn't give you any guarantees about what compartment you'll be in afterwards (since AutoPushJSContext doesn't promise to land you in the current compartment of the outer window the way AutoCxPusher does).

That being said, this screwiness is within existing noise levels of this stuff in Gecko, so I'm not massively concerned. It's just possible that you may end up with opaque exceptions in certain weird call paths.

@@ +2921,5 @@
> +      continue;
> +    }
> +
> +    AutoPushJSContext cx(windowAction.mJSContext);
> +    MOZ_ASSERT(cx);

Same here.
Attachment #812305 - Flags: feedback?(bobbyholley+bmo) → feedback+
Ok, here we go.

https://hg.mozilla.org/integration/mozilla-inbound/rev/160f313bccab

I'm pretty sure I fixed bug 890841, those changes I'll post there, but they have r=sicking.
https://hg.mozilla.org/mozilla-central/rev/160f313bccab
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
>pref("dom.workers.sharedWorkers.enabled", false);

When will shared workers be enabled by default?
(In reply to Mayhem~ from comment #39)
> When will shared workers be enabled by default?

Bug 924089.
Depends on: 925070
Depends on: 923649
Blocks: 1183894
Depends on: 1313762
Alias: SharedWorkers
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.