Closed Bug 1251276 Opened 4 years ago Closed 4 years ago

Inline the tiny bit of RunExpiredTimeouts that CancelAllTimeouts cares about into CancelAllTimeouts


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: bzbarsky, Assigned: bzbarsky)



(Whiteboard: btpp-active)


(2 files)

This makes it easier to reason about what CancelAllTimeouts can actually do (e.g. it cannot throw a JS exception).
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.
Attachment #8724187 - Flags: review?(khuey)
Whiteboard: btpp-active
Comment on attachment 8724187 [details] [diff] [review]
part 1.  Change WorkerPrivate::CancelAllTimeouts to no longer call RunExpiredTimeouts

Review of attachment 8724187 [details] [diff] [review]:

::: dom/workers/WorkerPrivate.cpp
@@ +5190,1 @@
>      mTimerRunning = false;

We do this because CancelAllTimeouts could be called multiple times (if, for example we're notified at different levels as we attempt to kill the thread more severely, and the worker thread is not scheduled until they are all in the queue).  If that happens, we'll try to run *this* code multiple times.  And the assertion that we have a timer will fail.
Attachment #8724187 - Flags: review?(khuey) → review+
> And the assertion that we have a timer will fail.

Ah, indeed.  Documented.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.