Workers can trigger slow script dialogs if no other script runs while a worker is posting events to the main thread

RESOLVED FIXED in Firefox 14

Status

()

Core
DOM: Workers
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox13 wontfix, firefox14 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

See bug 736152 comment 10.  We need to be calling ScriptEvaluated on the right nsJSContext somehow.

Ben, do you want to take this, or do you want me to do it?
I'm a bit swamped, but this doesn't sound too difficult. For main thread workers we already hold the nsIScriptContext pointer (mScriptContext), so maybe we just need to expose ScriptEvaluated there?
Blocks: 736152
Created attachment 620584 [details] [diff] [review]
Don't let DOM workers confuse the slow-script time accounting and trigger slow script dialogs.
Attachment #620584 - Flags: review?(bent.mozilla)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment on attachment 620584 [details] [diff] [review]
Don't let DOM workers confuse the slow-script time accounting and trigger slow script dialogs.

>+  // Hold a strong ref, since we plan to run script and who knows what it'll do.
>+  nsCOMPtr<nsIXPCScriptNotify> notify = aWorkerPrivate->GetScriptNotify();

This will run on all threads, not just the main thread.

You can check GetParent(), and if it returns null then you're a top-level worker. However, that won't tell you if you're running on the main thread (worker script sending message to main thread) or on the worker thread (main thread sending message to worker)...

I think you should probably make DispatchEventToTarget take an nsIXPCScriptNotify*, not a WorkerPrivate*, and then grab it from the two callers or pass null. The callers (WorkerRunable) will know which thread they're on by testing GetParent() == null and mTarget == ParentThread. I think you only have to do this for message and error events. Nothing else (like close events) is fired on the main thread.

>   aDoomed.SetCapacity(6);
> 
>   SwapToISupportsArray(mWindow, aDoomed);
>   SwapToISupportsArray(mScriptContext, aDoomed);
>+  SwapToISupportsArray(mScriptNotify, aDoomed);

Change that SetCapacity to 7 :)
Attachment #620584 - Flags: review?(bent.mozilla) → review-
> This will run on all threads, not just the main thread.

Ah, ok.  That was not obvious.  Good thing we have this code review stuff.  ;)

I'll fix this up tomorrow.
Yeah, sorry about that. I usually assert which thread we should be on at the start of every method, but the event stuff is generic. I should make a no-op AssertIsOnAnyThread function for this kind of thing!
(In reply to ben turner [:bent] from comment #3)
> I think you should probably make DispatchEventToTarget take an
> nsIXPCScriptNotify*

Actually, it's much simpler if you just add the nsIXPCScriptNotify stuff to the MessageEventRunnable/ReportErrorRunnable instead of messing with DispatchEventToTarget.
Created attachment 620834 [details] [diff] [review]
This should be better
Attachment #620834 - Flags: review?(bent.mozilla)
Attachment #620584 - Attachment is obsolete: true
Comment on attachment 620834 [details] [diff] [review]
This should be better

>+#include "nsIXPCScriptNotify.h"

Nit: Please stick this in the other interface include block, alphabetized.

>+    // Notify before WorkerRunnable::PostRun, since that can kill aWorkerPrivate

Is that longer than 80 chars?

>+class nsIXPCScriptNotify;

Nit: Alphabetize please.

>+  void NotifyScriptExecutedIfNeeded();

Nit: This can be const
Attachment #620834 - Flags: review?(bent.mozilla) → review+
> Is that longer than 80 chars?

It's 79 chars.  ;)

Fixed the nits.

Waiting on https://tbpl.mozilla.org/?tree=Try&rev=342c5975da48 before pushing.
Whiteboard: [need review] → [need landing]
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a901a6e733
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla15
Comment on attachment 620834 [details] [diff] [review]
This should be better

I think we should consider taking this on aurora.  I'm not sure it's worth it for beta, necessarily.

[Approval Request Comment]
Regression caused by (bug #): Not a regression.
User impact if declined: Pages that use web workers can end up with random slow
   script dialogs even when there is no slow script involved.
Testing completed (on m-c, etc.): QA ran a build with a similar patch when
   investigating bug 736152 and reported that it made the slow script dialogs
   go away.  Also manual testing.
Risk to taking this patch (and alternatives if risky): There's a slight risk, I
   think.  Ben would be a better source for a risk evaluation.
String changes made by this patch: None
Attachment #620834 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/40a901a6e733
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Boris Zbarsky (:bz) from comment #11)
> Risk to taking this patch (and alternatives if risky): There's a slight
> risk, I
>    think.  Ben would be a better source for a risk evaluation.

Ben, would you mind helping us with the risk evaluation here?
I think this is very safe for aurora (due to stack-based refcounting). It's a little more risky on beta (no stack-based refcounting), so I'd probably skip there unless we have a heavy volume of slow-script reports of which I'm currently unaware.
Comment on attachment 620834 [details] [diff] [review]
This should be better

[Triage Comment]
Given that, approved for Aurora 14 but we'll let this fix ride the trains.
Attachment #620834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
status-firefox13: --- → wontfix
http://hg.mozilla.org/releases/mozilla-aurora/rev/61af70e94213
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.