Last Comment Bug 725848 - Worker hang at shutdown with XHR
: Worker hang at shutdown with XHR
Status: RESOLVED FIXED
[qa?]
: hang, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: 614146 786987
  Show dependency treegraph
 
Reported: 2012-02-09 15:30 PST by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-08-30 03:25 PDT (History)
3 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (17.19 KB, patch)
2012-02-09 15:30 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-09 15:30:16 PST
Created attachment 595884 [details] [diff] [review]
Patch
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-02-09 17:04:05 PST
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 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-02-09 17:10:50 PST
Comment on attachment 595884 [details] [diff] [review]
Patch

Aha, Splinter review got confused. Anyway, r=me with those comments addressed.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-09 21:29:04 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e20dbdb7db
Comment 4 Ed Morley [:emorley] 2012-02-10 04:57:34 PST
https://hg.mozilla.org/mozilla-central/rev/a5e20dbdb7db
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-11 16:38:17 PST
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
Comment 6 Alex Keybl [:akeybl] 2012-02-14 11:43:18 PST
(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.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-14 11:47:14 PST
(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.
Comment 8 Alex Keybl [:akeybl] 2012-02-14 12:10:50 PST
(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.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-14 14:37:53 PST
A follow up:

https://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-15 17:48:33 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ad3b142fe47
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-03 15:06:05 PDT
Is there something QA can do to verify this fix?

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