Closed
Bug 1316871
Opened 8 years ago
Closed 8 years ago
SetTimeout isn't throttled for background tabs
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: farre, Assigned: farre)
References
Details
Attachments
(2 files, 9 obsolete files)
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → afarre
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8809839 -
Flags: review?(bkelly)
Assignee | ||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Comment on attachment 8809839 [details] [diff] [review]
0001-Bug-1316871-Throttle-background-setTimeouts.-r-bkell.patch
Review of attachment 8809839 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed.
::: dom/base/nsGlobalWindow.cpp
@@ +12639,5 @@
> // Now clamp the actual interval we will use for the timer based on
> uint32_t nestingLevel = sNestingLevel + 1;
> uint32_t realInterval = interval;
> + if (aIsInterval || nestingLevel >= DOM_CLAMP_TIMEOUT_NESTING_LEVEL ||
> + (mOuterWindow && mOuterWindow->IsBackground())) {
To match the logic in DOMMinTimeoutValue() this should be:
!mOuterWindow || mOuterWindow->IsBackground()
Attachment #8809839 -
Flags: review?(bkelly) → review+
Comment 4•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #3)
> !mOuterWindow || mOuterWindow->IsBackground()
It would probably be a good idea to make a helper routine for this that could be called in both places. IsBackgroundInternal() or whatever.
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8809839 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8810342 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Got a bunch of perma-failing test with that try run. Updating and trying again with:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3988eab7d04c6861257b23e6cf91432443977a79
Comment 9•8 years ago
|
||
Some of these may be racy tests that the background delay is now causing those races to lose. For example, the timeouts in this test seem a bit suspect:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/XMLHttpRequest/open-url-multi-window-6.htm
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/XMLHttpRequest/resources/init.htm
But I'm not 100% sure I understand the condition the test is trying to validate.
Comment 10•8 years ago
|
||
If you want to unblock the requestIdleCallback() bug from this you could just use gMinBackgroundTimeoutValue for the timeout value there instead of 0. So you won't rely on the timer code fixing the value for you, but set it explicitly instead.
Assignee | ||
Comment 11•8 years ago
|
||
I think that the test tries to validate that you can't do an XMLHttpRequest on a document you've navigated away from. That this is racy seems to indicate that we somehow fail in moving a document to an inactive state, even though the onload of a new document has fired. Why this is worse when we throttle the load I can't really understand though.
On the bright side, I found a way that perhaps doesn't fix that the test is racy, but at least takes us back to the racy behaviour we had before fixing throttling of setTimeout. I did this by moving the call to setTimeout in open-url-multi-6.htm to the window opened by Window.open instead of the main test window.
Assignee | ||
Comment 12•8 years ago
|
||
This is looking good after fixing the test and rebasing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc982d75113ec3800e8016a247150bb1d82e2655
Ben, we talked about uplifting this one as well, is that still true?
Attachment #8810367 -
Attachment is obsolete: true
Flags: needinfo?(bkelly)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Sorry, I don't think the test change was reviewed yet.
I would like to uplift this, but I think the test may show a problem with our implementation. We should be trying to maintain ordering of timers within a TabGroup or at least a DocGroup. Our background test, however, only checks if the current window is in the background.
I think a better fix here would be to make our background check only return true if all windows in the TabGroup are in the background. What do you think?
Flags: needinfo?(bkelly) → needinfo?(afarre)
Keywords: checkin-needed
Comment 14•8 years ago
|
||
I think the way to do this would be to add a TabGroup::AllWindowsInBackground() getter. It would iterate over the TabGroup::mWindows internally and check its background state.
Michael, does this sound ok to you?
Flags: needinfo?(michael)
Comment 15•8 years ago
|
||
Well, talking with Boris in IRC about this.
Flags: needinfo?(michael)
Flags: needinfo?(afarre)
Comment 16•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #14)
> I think the way to do this would be to add a
> TabGroup::AllWindowsInBackground() getter. It would iterate over the
> TabGroup::mWindows internally and check its background state.
Yeah, this function will probably be useful in other places too.
Comment 17•8 years ago
|
||
(In reply to :Ehsan Akhgari from comment #16)
> (In reply to Ben Kelly [:bkelly] from comment #14)
> > I think the way to do this would be to add a
> > TabGroup::AllWindowsInBackground() getter. It would iterate over the
> > TabGroup::mWindows internally and check its background state.
>
> Yeah, this function will probably be useful in other places too.
Well, it turns out we don't need it here. Plan from IRC is fix a bfcache/session-store race so this test always fails. We're only passing due to luck today. Then we will mark it expected fail until we can fix XHR. (Root problem is xhr.open is working on bfcached windows.)
Comment 18•8 years ago
|
||
IRC discussion:
http://logs.glob.uno/?c=content#c410522
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #18)
> IRC discussion:
>
> http://logs.glob.uno/?c=content#c410522
http://logs.glob.uno/?c=mozilla%23content&s=23+Nov+2016#c410522
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8813729 -
Attachment is obsolete: true
Attachment #8814015 -
Flags: review?(amarchesini)
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8811268 -
Attachment is obsolete: true
Attachment #8814021 -
Flags: review?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Attachment #8814015 -
Flags: review?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Attachment #8814021 -
Flags: review?(amarchesini)
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8814015 -
Attachment is obsolete: true
Attachment #8815300 -
Flags: review?(bkelly)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8815301 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8814021 -
Attachment is obsolete: true
Comment 24•8 years ago
|
||
Comment on attachment 8815300 [details] [diff] [review]
0001-Bug-1316871-Throttle-background-setTimeouts.-r-bkell.patch
Review of attachment 8815300 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsGlobalWindow.h
@@ +1725,5 @@
> mozilla::dom::TabGroup* TabGroupOuter();
>
> + bool IsBackgroundInternal() const
> + {
> + return !mOuterWindow || mOuterWindow->IsBackground();
nit: I highly encourage putting implementation in cpp files unless there is a documented perf benefit for this method. It makes it easier to find code and also avoids large recompiles if the contents of this method need to change.
Attachment #8815300 -
Flags: review?(bkelly) → review+
Updated•8 years ago
|
Attachment #8815301 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8815300 -
Attachment is obsolete: true
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8816813 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8816811 -
Attachment is obsolete: true
Comment 27•8 years ago
|
||
Comment on attachment 8816813 [details] [diff] [review]
0001-Bug-1316871-Throttle-background-setTimeouts.-r-bkell.patch
Review of attachment 8816813 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Just for future reference, for a small change like this addressing review feedback from an r+ you don't really need to re-request review again.
Attachment #8816813 -
Flags: review?(bkelly) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 28•8 years ago
|
||
Comment 29•8 years ago
|
||
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6474932e4e65
Throttle background setTimeouts. r=bkelly
Comment 30•8 years ago
|
||
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0d6750d202
Ensure that we don't replace load popup. r=bkelly
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6474932e4e65
https://hg.mozilla.org/mozilla-central/rev/5d0d6750d202
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•