If you walk through what RunExpiredTimeouts used to when called from here do
carefully, it used to do the following:
1) If mRunningExpiredTimeouts, no-op.
2) Not run anything, because everything is canceled.
3) Remove everything from mTimeouts, since everything is canceled.
4) Since mTimeouts is now empty, modify the busy count and set mTimerRunning to false.
None of this could report a JS exception, so the JS_ReportPendingException call
in CancelAllTimouts was dead code. Note that the return value of
RunExpiredTimeouts only affected whether JS_ReportPendingException is called, so
we don't even need to worry about ModifyBusyCountFromWorker failing: that
failure used to be silently swallowed.
The only reason NotifyRunnable::Dispatch needs a JSContext is so that it can call
ModifyBusyCount in Pre/PostDispatch. The only reason that needs a JSContext is
to call Cancel(), which only needs it to call Notify(), which only needs it to
call NotifyPrivate, which only needs it to dispatch a NotifyRunnable.
The only reason NotifyRunnable::Dispatch needs a JSContext is so that it can call
ModifyBusyCount in Pre/PostDispatch. The only reason that needs a JSContext is
to call Cancel(), which only needs it to call Notify(), which only needs it to
call NotifyPrivate, which only needs it to dispatch a NotifyRunnable.
There is a bit of subtlety here with NS_BINDING_ABORTED. Before these changes,
we would land in ReportLoadError, not do anything with NS_BINDING_ABORTED, and
just return. If called from WorkerPrivate::Constructor we'd then go ahead and
throw it on the ErrorResult, but I'm pretty sure we never ended up with
NS_BINDING_ABORTED there. If called from ScriptExecutorRunnable::WorkerRun, we
would proceed on to ScriptExecutorRunnable::PostRun and hence
ShutdownScriptLoader where we would throw on the ErrorResult but NOT on the
JSContext. Then we would unwind to our consumer and if that consumer was a
toplevel script load we would suppress the exception on the ErrorResult.
Otherwise we'd go ahead and throw the exception we ended up with to the caller.
The upshot is that we used to not fire error events on a worker whose main
script load was canceled with NS_BINDING_ABORTED. So we try to preserve that
behavior explicitly for toplevel scripts.
Specifically we make the following changes:
1) Remove WorkerSameThreadRunnable::PostRun, because it does exactly the same
things as WorkerRunnable::PostRun.
2) Always treat ModifyBusyCountFromWorker as infallible in terms of throwing
JS exceptions.
3) Change ExtendableFunctionalEventWorkerRunnable::PostRun to properly call
its superclass PostRun so we will correctly decrement the busy count our
PreDispatch incremented.
4) Document why some overrides of PreDispatch/PostDispatch are needed and
don't call into the superclass
RunExpiredTimeouts has "fudging" code to always ensure that we execute at least one timeout. This is intended to cover cases where an nsITimer fires slightly early, but it means we must be careful not to fire a timer more times than we intend to or we'll execute a timeout prematurely.
Consider a sequences of setTimeout calls alternating in delay between 0ms and 1000ms. When the 1000ms timeout fires, it schedules a 0ms timeout. The setTimeout call itself calls RescheduleTimeoutTimer, which schedules the timer for a 0 ms delay. And once we unwind the 1000ms timeout RunExpiredTimeouts will also schedule the timer for a 0 ms delay. If the timer has fired (remember, it's processed on a completely different thread) in the meantime, we ultimately will get two callbacks from nsITimer for our 0 ms timeout. The first will run the 0 ms timeout and schedule a 1000 ms timeout, and the second will run the 1000 ms timeout (remember, RunExpiredTimeouts always runs at least one timeout!) ~999 ms ahead of schedule.
The solution is to cancel the timer in RescheduleTimeoutTimer, so that when we call it the second time it will cause any pending events from the first scheduling to be canceled. But this actually doesn't work at all, because of how we use nsITimer. Before worker threads were capable of accepting arbitrary runnables we created TimerThreadEventTarget, which translates the timer firing to the special worker event queue when the timer thread attempts to *dispatch* a runnable to the worker. We still need this for some of the other types of timers (which use control runnables that interrupt JS, and not the regular event queue). But setTimeout can simply run like a normal nsITimer callback now. We need that here, or calling nsITimer::Cancel won't actually do anything, because the timer's event was ignored and TimerThreadEventTarget created its own event.
Boris Zbarsky
Eddy Bruel
Wes Kocher
Daniel Holbert
Andrea Marchesini
Kyle Huey
Aidin Gharibnavaz
Carsten "Tomcat" Book
Birunthan Mohanathas
Phil Ringnalda
Luke Wagner