1371020 - Crash in mozilla::dom::Timeout::When

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-b6635e6b-1a7c-407f-b0a4-7ebbc0170604.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170602030204: http://bit.ly/2rCsULL

Possible regression range based on build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bdb2387396b4a74dfefb7c983733eed3625e906a&tochange=aeb3d0ca558f034cbef1c5a68bd07dd738611494

Not sure if Bug 1363829 is involved, ni on bkelly

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

First guess is that we are trying to schedule the TimeoutExecutor on a frozen window and the Timeout->When() is asserting since its an illegal call in that case.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Avoid rescheduling the TimeoutExecutor in ClearTimeout() in some cases. r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Avoid scheduling TimeoutExecutor if the window becomes suspended in RunTimeout. r=ehsan

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Cleanup some IsFrozen()/IsSuspended() checking in TimeoutManager. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0edd8d225072294484f37b9139b70045c1822f22

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add a mochitest that executes clearTimeout() on a frozen window. r=ehsan

WIP test that doesn't work yet.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875736 [details] [diff] [review]
P4 Add a mochitest that executes clearTimeout() on a frozen window. r=ehsan

I think its going to be too racy to try to make a test that triggers this.  Also, I think we should close loop holes that allow us to run js code while in the bfcache.  I'll file a separate bug about getting IDB callbacks on a frozen window.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875716 [details] [diff] [review]
P1 Avoid rescheduling the TimeoutExecutor in ClearTimeout() in some cases. r=ehsan

Ehsan, this directly addresses the crash from comment 0.  It ensures that we don't try to reschedule the TimeoutExecutor when the window is frozen.  I use a check for IsSuspended() because a frozen window is always suspended and we also should not reschedule in that case.

I also added a check for the deferred deletion case since we are already handling a rescheduling of the timeout executor in RunTimeout() in that case.  That is not strictly necessary to fix this crash, but I think its most correct for this method.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875717 [details] [diff] [review]
P2 Avoid scheduling TimeoutExecutor if the window becomes suspended in RunTimeout. r=ehsan

I also reviewed other places we call TimeoutExecutor::MaybeSchedule().  Most of them are already covered by checks for mWindow.IsSuspended().  I think the one exception is in RunTimeout() when we exhaust the time budget.  This patch fixes that in RunTimeout.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875719 [details] [diff] [review]
P3 Cleanup some IsFrozen()/IsSuspended() checking in TimeoutManager. r=ehsan

While reviewing the code for TimeoutExecutor::MaybeSchedule() while suspended/frozen I discovered a few places where we could cleanup our existing checking.

In general we don't need to explicitly check for IsFrozen() if we are going to short-circuit on IsSuspended().  We have an invariant that if the window is frozen then its also suspended.

Similarly, if we are only operating on a non-suspended window then we don't have to worry about maybe sorting on TimeRemaining in the iterator.

Finally, Timeout::When() has a TimeStamp::IsNull() assertion already.  We don't need an extra one in TimeoutManager.

This patch does not change existing behavior.

Comment 11

[(no longer active)]

Comment on attachment 8875717 [details] [diff] [review]
P2 Avoid scheduling TimeoutExecutor if the window becomes suspended in RunTimeout. r=ehsan

Review of attachment 8875717 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/TimeoutManager.cpp
@@ +670,5 @@
>    // in order for timeouts to fire properly.
>    if (!nextDeadline.IsNull()) {
> +    // Note, we verified the window is not suspended at the top of
> +    // method and the window should not have been suspended while
> +    // executing the loop above since it doesn't call out to js.

Maybe officiate this with a MOZ_ASSERT too?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d832f5aee62a
P1 Avoid rescheduling the TimeoutExecutor in ClearTimeout() in some cases. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/536ad9812d0c
P2 Avoid scheduling TimeoutExecutor if the window becomes suspended in RunTimeout. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/28064e6e5ba4
P3 Cleanup some IsFrozen()/IsSuspended() checking in TimeoutManager. r=ehsan

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d832f5aee62a
https://hg.mozilla.org/mozilla-central/rev/536ad9812d0c
https://hg.mozilla.org/mozilla-central/rev/28064e6e5ba4

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Looking at crash stats, the last crash was build id 20170609030207.  Since this landed on June 9 it would have first been in the June 10 build.  So I think this shows the patches fixed the crash.