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?
Created attachment 620584 [details] [diff] [review] Don't let DOM workers confuse the slow-script time accounting and trigger slow script dialogs.
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 :)
> 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
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
> 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 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
(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.