Last Comment Bug 643325 - (SharedWorkers) Review Web Workers draft and implement SharedWorkers
(SharedWorkers)
: Review Web Workers draft and implement SharedWorkers
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 All
: -- normal with 14 votes (vote)
: mozilla27
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
http://dev.w3.org/html5/workers/
Depends on: 741618 880367 922528 923649 924089 925070 990725
Blocks: html5test WorkForTheWorkers 881652 ServiceWorkers 904306 907060 890841 898706 1183894
  Show dependency treegraph
 
Reported: 2011-03-20 14:55 PDT by Olli Pettay [:smaug]
Modified: 2015-07-14 15:52 PDT (History)
46 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch, v1 (205.21 KB, patch)
2013-06-07 15:36 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v1 (222.06 KB, patch)
2013-06-14 12:02 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review-
Details | Diff | Review
Patch, v1 (239.99 KB, patch)
2013-08-28 17:16 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v2 (247.10 KB, patch)
2013-09-20 18:46 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Interdiff, v1 to v2 (43.71 KB, patch)
2013-09-20 18:47 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v3 (242.91 KB, patch)
2013-09-30 15:12 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Review
Interdiff, v2 to v3 (79.55 KB, patch)
2013-09-30 15:12 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v3 (240.63 KB, patch)
2013-09-30 15:22 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
bobbyholley: feedback+
Details | Diff | Review
Interdiff, v2 to v3 (84.09 KB, patch)
2013-09-30 15:23 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] 2011-03-20 14:55:25 PDT
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.
Comment 1 Mark Hammond [:markh] 2012-02-08 22:52:16 PST
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.
Comment 2 Florian Bender 2012-04-02 09:19:10 PDT
I think another bug for implementing MessagePort (including your remarks) is beneficial. Could you please open one and add it here?
Comment 3 Florian Bender 2012-10-17 17:33:27 PDT
I'm pretty sure there was a separate bug for implementing SharedWorkers but I can't find it. Has this bug been renamed?
Comment 4 Mark Hammond [:markh] 2012-10-17 17:40:06 PDT
(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.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-07 15:36:48 PDT
Created attachment 759988 [details] [diff] [review]
Patch, v1

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.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-14 12:02:23 PDT
Created attachment 762833 [details] [diff] [review]
Patch, v1

Now with several more tests.
Comment 7 SUN Haitao 2013-06-22 09:25:42 PDT
It seems that this patch can be applied to neither m-c (REV c6bca8768874) nor m-i (REV 50e5eb0c4008) now.
Comment 8 Florian Bender 2013-07-28 14:52:50 PDT
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?
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-28 17:50:53 PDT
(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.
Comment 10 Florian Bender 2013-07-30 10:30:44 PDT
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 …
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-07-30 11:10:33 PDT
I am reviewing it now, but it's a big patch.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-07-31 15:58:39 PDT
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.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-08-01 09:02:32 PDT
> ::: 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 ...
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-08-01 09:02:56 PDT
(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.
Comment 15 Florian Bender 2013-08-01 09:10:45 PDT
(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! :)
Comment 16 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-08-06 11:13:05 PDT
Will this bug also provide shared ChromeWorkers?
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-08-06 11:15:58 PDT
What is the benefit of a shared ChromeWorker?  You can just wrap your ChromeWorker in a jsm that lets people talk to it.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-06 11:25:01 PDT
(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.
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-08-06 11:28:51 PDT
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.
Comment 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-08-06 12:21:03 PDT
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.
Comment 21 Florian Bender 2013-08-28 05:10:36 PDT
I guess this bug is now blocked by Bug 677638?
Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-28 17:16:27 PDT
Created attachment 796970 [details] [diff] [review]
Patch, v1

This is just un-bitrotted for those flowing along. I haven't had a chance to look at khuey's comments yet.
Comment 23 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-20 18:46:12 PDT
Created attachment 808066 [details] [diff] [review]
Patch, v2

Merged and comments addressed (quibbles to follow!)
Comment 24 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-20 18:47:44 PDT
Created attachment 808067 [details] [diff] [review]
Interdiff, v1 to v2

Changes from v1 to v2 (not including ridiculous merge gymnastics).
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-20 19:18:55 PDT
(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.
Comment 26 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-09-26 01:49:13 PDT
(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 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-09-27 03:13:13 PDT
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.
Comment 28 Jonas Sicking (:sicking) 2013-09-28 16:44:10 PDT
> 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!
Comment 29 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 15:12:12 PDT
Created attachment 812295 [details] [diff] [review]
Patch, v3

With most comments addressed.
Comment 30 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 15:12:52 PDT
Created attachment 812296 [details] [diff] [review]
Interdiff, v2 to v3
Comment 31 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 15:17:59 PDT
(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...
Comment 32 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 15:22:30 PDT
Created attachment 812305 [details] [diff] [review]
Patch, v3

Bobby, can you look over the JSContext usage in WorkerPrivate.cpp, specifically BroadcastErrorToSharedWorkers()?
Comment 33 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 15:23:05 PDT
Created attachment 812306 [details] [diff] [review]
Interdiff, v2 to v3
Comment 34 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-30 23:10:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eedf61cab3fa
Comment 35 Bobby Holley (busy) 2013-10-01 00:08:43 PDT
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.
Comment 36 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-10-01 01:11:52 PDT
Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/af4605d0cc6b

Two different test failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=28600714&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28600385&tree=Mozilla-Inbound

And bug 890841 seemed to happen a lot more frequently.
Comment 37 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-10-02 12:46:07 PDT
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.
Comment 38 Phil Ringnalda (:philor) 2013-10-03 08:10:18 PDT
https://hg.mozilla.org/mozilla-central/rev/160f313bccab
Comment 39 Mayhem~ 2013-10-07 03:09:02 PDT
>pref("dom.workers.sharedWorkers.enabled", false);

When will shared workers be enabled by default?
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-10-07 07:07:15 PDT
(In reply to Mayhem~ from comment #39)
> When will shared workers be enabled by default?

Bug 924089.
Comment 41 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-01-28 12:23:01 PST
Shared workers APIs now documented, see:

https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker
https://developer.mozilla.org/en-US/docs/Web/API/SharedWorkerGlobalScope
etc.

Note You need to log in before you can comment on or make changes to this bug.