Crash in mozilla::dom::Timeout::When

RESOLVED FIXED in Firefox 55

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: marcia, Assigned: bkelly)

Tracking

({crash, regression})

55 Branch
mozilla55
Unspecified
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

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
Flags: needinfo?(bkelly)
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.
Assignee: nobody → bkelly
Blocks: 1363829
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
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.
Attachment #8875736 - Attachment is obsolete: true
Priority: -- → P2
See Also: → 1371389
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.
Attachment #8875716 - Flags: review?(ehsan)
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.
Attachment #8875717 - Flags: review?(ehsan)
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.
Attachment #8875719 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8875716 - Flags: review?(ehsan) → review+

Comment 11

2 years ago
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?
Attachment #8875717 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8875719 - Flags: review?(ehsan) → review+

Comment 12

2 years ago
Pushed by bkelly@mozilla.com:
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
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.