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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This makes it easier to reason about what CancelAllTimeouts can actually do (e.g. it cannot throw a JS exception).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change WorkerPrivate::CancelAllTimeouts to no longer call RunExpiredTimeouts

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the JSContext argument of WorkerPrivate::CancelAllTimeouts

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> And the assertion that we have a timer will fail.

Ah, indeed.  Documented.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2534d7a87dc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/89429721fef3

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2534d7a87dc1
https://hg.mozilla.org/mozilla-central/rev/89429721fef3