Worker hang at shutdown with XHR

RESOLVED FIXED in Firefox 12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

({hang, regression})

Trunk
mozilla13
hang, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa?])

Attachments

(1 attachment)

Created attachment 595884 [details] [diff] [review]
Patch
Attachment #595884 - Flags: review?(bent.mozilla)
Keywords: hang, regression
Blocks: 614146
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.
Attachment #595884 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e20dbdb7db
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a5e20dbdb7db
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
Attachment #595884 - Flags: approval-mozilla-beta?
Attachment #595884 - Flags: approval-mozilla-aurora?

Comment 6

5 years ago
(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.

Updated

5 years ago
Attachment #595884 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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

5 years ago
(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.

Updated

5 years ago
Attachment #595884 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
A follow up:

https://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ad3b142fe47
status-firefox12: --- → fixed
Is there something QA can do to verify this fix?
Whiteboard: [qa?]

Updated

5 years ago
Blocks: 786987
You need to log in before you can comment on or make changes to this bug.