Comment on attachment 595884 [details] [diff] [review] Patch Review of attachment 595884 [details] [diff] [review]: ----------------------------------------------------------------- Weren't you also going to change the order in StopAcceptingEvents? Also, you never use WantsToRunDuringClear... I think this is an incorrectly refreshed patch. ::: dom/workers/WorkerPrivate.cpp @@ +702,5 @@ > NS_ASSERTION(runtime, "This should never be null!"); > > runtime->UnregisterWorker(aCx, mFinishedWorker); > > + aWorkerPrivate->DoomChildWorker(mFinishedWorker); This is not relevant @@ +2650,5 @@ > // Keep track of whether or not this is the idle GC event. > eventIsNotIdleGCEvent = event != idleGCEvent; > > + nsIRunnable* runnable = event; > + runnable->Run(); static_cast<nsIRunnable*>(event)->Run(); ::: dom/workers/WorkerPrivate.h @@ +83,5 @@ > protected: > WorkerPrivate* mWorkerPrivate; > Target mTarget; > bool mBusyBehavior; > + bool mClearingBehavior; Uh, oops, can you change these to not be bool but BusyBehavior and ClearingBehavior? @@ +156,5 @@ > + ClearingBehavior aClearingBehavior, > + PRUint32 aSyncQueueKey, > + bool aBypassSyncQueue = false) > + : WorkerRunnable(aWorkerPrivate, WorkerThread, UnchangedBusyCount, > + aClearingBehavior), Hm, this is gross. Let's just keep one constructor and add the skip flag last (and optional)

Comment on attachment 595884 [details] [diff] [review] Patch Aha, Splinter review got confused. Anyway, r=me with those comments addressed.

Comment on attachment 595884 [details] [diff] [review] Patch [Approval Request Comment] Regression caused by (bug #): Worker rewrite User impact if declined: Sporadic shutdown hangs when using workers. Testing completed (on m-c, etc.): On m-c, have seen a substantial decrease in the frequency of our shutdown hangs when running the worker tests. Risk to taking this patch (and alternatives if risky): Low risk. The patch is a little big, but it's mostly mechanical. The actual complexity here is low. String changes made by this patch: N/A

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > Comment on attachment 595884 [details] [diff] [review] > Patch > > [Approval Request Comment] > Regression caused by (bug #): Worker rewrite I'm not aware of when the worker rewrite first landed. If it's not in 11 and we haven't gotten any duplicates of this for 10, I'd rather not land on mozilla-beta. Would you mind clarifying? Approving for Aurora 12 though.

(In reply to Alex Keybl [:akeybl] from comment #6) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > > Comment on attachment 595884 [details] [diff] [review] > > Patch > > > > [Approval Request Comment] > > Regression caused by (bug #): Worker rewrite > > I'm not aware of when the worker rewrite first landed. If it's not in 11 and > we haven't gotten any duplicates of this for 10, I'd rather not land on > mozilla-beta. Would you mind clarifying? > > Approving for Aurora 12 though. The worker rewrite landed before 10. That said, we see hangs at shutdown related to workers on tinderbox all the time. It's one of our most frequent random oranges. See the blocked bug.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > The worker rewrite landed before 10. That said, we see hangs at shutdown > related to workers on tinderbox all the time. It's one of our most frequent > random oranges. See the blocked bug. Given that, I'd rather see this fixed on 12 and up for purposes of risk mitigation.

A follow up: https://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079

Is there something QA can do to verify this fix?