725848 - Worker hang at shutdown with XHR

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

Comment on attachment 595884 [details] [diff] [review]
Patch

Aha, Splinter review got confused. Anyway, r=me with those comments addressed.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5e20dbdb7db

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

(In reply to Alex Keybl [:akeybl] from comment #6)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

A follow up:

https://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ad3b142fe47

Comment 11

[u279076]

Is there something QA can do to verify this fix?