Last Comment Bug 751458 - Workers can trigger slow script dialogs if no other script runs while a worker is posting events to the main thread
: Workers can trigger slow script dialogs if no other script runs while a worke...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: unspecified
: x86 Mac OS X
: -- major (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 736152
  Show dependency treegraph
 
Reported: 2012-05-02 20:43 PDT by Boris Zbarsky [:bz]
Modified: 2012-05-14 19:52 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed


Attachments
Don't let DOM workers confuse the slow-script time accounting and trigger slow script dialogs. (8.98 KB, patch)
2012-05-02 22:10 PDT, Boris Zbarsky [:bz]
bent.mozilla: review-
Details | Diff | Review
This should be better (6.18 KB, patch)
2012-05-03 13:30 PDT, Boris Zbarsky [:bz]
bent.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-05-02 20:43:00 PDT
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?
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-02 21:03:13 PDT
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?
Comment 2 Boris Zbarsky [:bz] 2012-05-02 22:10:41 PDT
Created attachment 620584 [details] [diff] [review]
Don't let DOM workers confuse the slow-script time accounting and trigger slow script dialogs.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-02 22:25:11 PDT
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 :)
Comment 4 Boris Zbarsky [:bz] 2012-05-02 22:27:43 PDT
> 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.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-02 22:31:51 PDT
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!
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-02 22:34:30 PDT
(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.
Comment 7 Boris Zbarsky [:bz] 2012-05-03 13:30:36 PDT
Created attachment 620834 [details] [diff] [review]
This should be better
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-03 13:38:37 PDT
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
Comment 9 Boris Zbarsky [:bz] 2012-05-03 14:04:58 PDT
> 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.
Comment 11 Boris Zbarsky [:bz] 2012-05-04 00:26:02 PDT
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
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-04 13:35:45 PDT
https://hg.mozilla.org/mozilla-central/rev/40a901a6e733
Comment 13 Alex Keybl [:akeybl] 2012-05-09 16:08:29 PDT
(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?
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-09 22:34:19 PDT
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 15 Alex Keybl [:akeybl] 2012-05-11 16:11:06 PDT
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.

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