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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

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 [:bzbarsky]]

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

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 [:bzbarsky]]

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

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

(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 [:bzbarsky]]

Attachment: This should be better

Comment 8

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

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 [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a901a6e733

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/40a901a6e733

Comment 13

[Alex Keybl [:akeybl]]

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

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

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/61af70e94213