Closed Bug 1316871 Opened 8 years ago Closed 7 years ago

SetTimeout isn't throttled for background tabs

Categories

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

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: farre, Assigned: farre)

References

Details

Attachments

(2 files, 9 obsolete files)

1.89 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.24 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
      No description provided.
Assignee: nobody → afarre
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+
(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.
Attachment #8809839 - Attachment is obsolete: true
Attachment #8810342 - Attachment is obsolete: true
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
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.
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.
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.
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)
Keywords: checkin-needed
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
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)
Well, talking with Boris in IRC about this.
Flags: needinfo?(michael)
Flags: needinfo?(afarre)
(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.
(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.)
Attachment #8813729 - Attachment is obsolete: true
Attachment #8814015 - Flags: review?(amarchesini)
Attachment #8811268 - Attachment is obsolete: true
Attachment #8814021 - Flags: review?(amarchesini)
Attachment #8814015 - Flags: review?(amarchesini)
Attachment #8814021 - Flags: review?(amarchesini)
Attachment #8814015 - Attachment is obsolete: true
Attachment #8815300 - Flags: review?(bkelly)
Attachment #8814021 - Attachment is obsolete: true
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+
Attachment #8815301 - Flags: review?(bkelly) → review+
Attachment #8815300 - Attachment is obsolete: true
Attachment #8816811 - Attachment is obsolete: true
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+
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6474932e4e65
Throttle background setTimeouts. r=bkelly
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0d6750d202
Ensure that we don't replace load popup. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/6474932e4e65
https://hg.mozilla.org/mozilla-central/rev/5d0d6750d202
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: