As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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] (Exited; not receiving bugmail, email if necessary)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 614146 786987
  Show dependency treegraph
 
Reported: 2012-02-09 15:30 PST by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
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] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-09 15:30:16 PST
Created attachment 595884 [details] [diff] [review]
Patch
Comment 1 User image 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-09 21:29:04 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e20dbdb7db
Comment 4 User image Ed Morley [:emorley] 2012-02-10 04:57:34 PST
https://hg.mozilla.org/mozilla-central/rev/a5e20dbdb7db
Comment 5 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-14 14:37:53 PST
A follow up:

https://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079
Comment 10 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-15 17:48:33 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ad3b142fe47
Comment 11 User image 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.