Closed Bug 1251276 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Whiteboard: btpp-active)

Attachments

(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.
https://hg.mozilla.org/mozilla-central/rev/2534d7a87dc1
https://hg.mozilla.org/mozilla-central/rev/89429721fef3
Status: NEW → RESOLVED
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.