Closed Bug 1329006 Opened 3 years ago Closed 3 years ago

RescheduleTimeout() incorrectly sets Timeout::mTimeRemaining for suspended windows

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: regression)

Attachments

(1 file)

In bug 1303167 we changed how timer deadlines are handled when the window is frozen vs suspended.  Previously we shifted timers to fire after their current timer remaining elapsed after resume/thaw.

As of bug 1303167 we no longer shift timer for suspended windows.  We only shift for frozen windows.

The code effect of this is that frozen windows should set mTimeRemaining while suspended windows should continue to set mWhen (like normal windows do).

It appears, however, that I missed a case where mTimeRemaining was set.  In RescheduleTimeout() we always set mTimeRemaining if there is no nsITimer available even if we are not frozen.  We should fix this to account for the frozen/suspend state correctly.
Olli, I found this via inspection while looking at another bug.  See comment 0 for the full explanation, but we should be setting mTimeRemaining for frozen windows and mWhen for suspended windows.

The likely impact of this is that setInterval() might fire at the wrong time if rescheduled during sync xhr or a modal dialog.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a3ee8a15d3e0e26c7834ee8bb90e71de4d96317
Attachment #8824211 - Flags: review?(bugs)
Attachment #8824211 - Attachment description: bug1329006_reschedule.patch → Make RescheduleTimeouts() deadlines correctly for suspended windows. r=smaug
Comment on attachment 8824211 [details] [diff] [review]
Make RescheduleTimeouts() deadlines correctly for suspended windows. r=smaug

ok, this matches TimeoutManager::SetTimeout and so, but could you please add a good comment why frozen and suspended are handled differently.

Is there any way to test this?
Attachment #8824211 - Flags: review?(bugs) → review+
See Also: → 1329284
(In reply to Olli Pettay [:smaug] from comment #2)
> ok, this matches TimeoutManager::SetTimeout and so, but could you please add
> a good comment why frozen and suspended are handled differently.

I'll duplicate the comment used in the other location we set these values.  I also will unify the logic in a single method in bug 1329284.  I want to do it in a follow-up so I can uplift this fix to FF52.

> Is there any way to test this?

Unfortunately any test would be very timing dependent.  I think its difficult to automate without using very large delay values that can be unambiguously detected.
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44ab9ab869d5
Make RescheduleTimeouts() deadlines correctly for suspended windows. r=smaug
Comment on attachment 8824211 [details] [diff] [review]
Make RescheduleTimeouts() deadlines correctly for suspended windows. r=smaug

Approval Request Comment
[Feature/Bug causing the regression]: bug 1303167
[User impact if declined]: Unexpected behavior on sites using setInterval() and synchronous features like sync xhr or modal dialogs.
[Is this code covered by automated tests?]: Unfortunately its timing dependent so difficult to automate testing.
[Has the fix been verified in Nightly?]: It has not landed on m-c yet.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: This is a small change in a rare code path.  I only want to uplift it because its technically a regression in FF52 right now.
[String changes made/needed]: None
Attachment #8824211 - Flags: approval-mozilla-aurora?
Keywords: regression
Note, this will require a rebase to land on aurora.
https://hg.mozilla.org/mozilla-central/rev/44ab9ab869d5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8824211 [details] [diff] [review]
Make RescheduleTimeouts() deadlines correctly for suspended windows. r=smaug

fix for timeouts in suspended windows, aurora52+

looks like the patch applies ok to dom/base/nsGlobalWindow.cpp with sed 's/mWindow\.//g'
Attachment #8824211 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.