647001 - Timeouts clamped in background tabs don't immediately unclamp when switching to the tab

Status: VERIFIED FIXED

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

Description

[Johnny Stenback (:jst)]

Not sure how much we care here, but this can certainly be observed by users. To reproduce (stolen largely from bug 646972):

1)  Make sure you have new tabs set to open in background.
2)  Open the context menu for the "URL" field link on this bug.
3)  Select "open in new tab"
4)  Wait 10 seconds.
5)  Switch to the new tab.
6)  Observer when the timeouts start firing every 10ms or so.

What you can see, depending on where in a given second you happen to switch to a tab, is that you switch, it looks like the page is stuck for a short while (anywhere between 0 and 1 seconds), then it wakes up.

I'd imagine users could notice this on pages that animate with fairly high frequency, and the effect would be that it appears Firefox isn't animating when they switch to the tab, but then all of a sudden it comes to life.

We could fix this by making the call to nsGlobalWindow::SetIsBackground(PRBool aIsBackground) is called with aIsBackground set to false, and restarting clamped timers at their unclamped interval at that point, or somesuch.

Thoughts?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should probably fix this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I did think about this when fixing bug 646972.  I'll see what I can do.  Just reiniting the timers can work, if done carefully.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Emanuele wants to give this a shot.

Comment 4

[Emanuele Costa]

Attachment: nsGlobalWindow patch

I virtualized SetIsBackground and created a function that will cycle and re-init all the non cleared timers with the correct DOMMinTimeoutValue interval.

Comment 5

[Emanuele Costa]

also notice for the time being the for-loop may raise an error but the nsresult can be overwritten depending on the NS_ERROR behaviour. I am not sure what would it be better to do in case of  InitWithFuncCallback failure. To keep looping or return?

Comment 6

[Emanuele Costa]

Attachment: nsGlobalWindow patch

fixed typo

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 540728 [details] [diff] [review]
nsGlobalWindow patch

A few things wrong here:

1)  Most importantly, resetting for 

   timeout->mWhen = TimeStamp::Now() + nextInterval;

is wrong.  It will fire the timer too late.  Worse yet, it will increase the firing time each time SetIsBackground() is called, and does this for all timeouts, so long-running timers might well never fire with this setup (consider a timer set for 5 minutes, with the user switching away from the tab and then back every 3 minutes.

2)  Calling the method ResetTimeoutOrInterval is not so great.  The point of the method is to deal with resetting timers when going active, really.  So I would only call it when aIsBackground is false while mIsBackground is true.  And I'd call it something like UnclampTimeouts or ResetNonbackgroundTimeouts.

What I think you want to do in that method is to stop walking once you have passed all the timers that were clamped.  For each timer that was clamped you need to reset it for when it would have fired if the old clamp were not applied and the new one is applied instead.

Note that mInterval is 0 for non-repeating timeouts at the moment, so your code doesn't work correctly for those...  I bet with your patch a 5 minute setTimeout fires almost immediately if you switch away from the tab and then back.

Comment 8

[Emanuele Costa]

Attachment: nsGlobalWindow patch

Ok, this should be a step closer.

I tried all combinations of timeouts including switching tabs very quickly after setting a long timeout of 5secs:

<script>
  var lastTime = new Date();
  function f() {
    var now = new Date();
    document.body.appendChild(document.createTextNode((now - lastTime) + " "));
    lastTime = now;
  }
  setInterval(f, 5000)
</script>

Last thing that should be left doing is breaking the loop after unclamp timers are all processed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

timeout->mInterval is going to be 0 for non-repeating timers... so as far as I can tell, this will make those fire too soon.  Test something with a setTimeout(f, 3600000), for example.....

Comment 10

[Emanuele Costa]

Attachment: nsGlobalWindow patch

Some testing code for future reference, to test intervals:

<script>
  var lastTime = new Date();
  function f() {
    var now = new Date();
    document.body.appendChild(document.createTextNode((now - lastTime) + " "));
    lastTime = now;
  }
  setInterval(f, 0)
</script>

to test timeouts (pay attention to #define DOM_CLAMP_TIMEOUT_NESTING_LEVEL):

<script>

var lastTime = new Date();
  
function f() {
    var now = new Date();
    document.body.appendChild(document.createTextNode((now - lastTime) + " "));
    lastTime = now;
  setTimeout(f, 0)
}

document.write("<p>Starting timers.</p>");
f();

</script>

after DOM_CLAMP_TIMEOUT_NESTING_LEVEL they should start behaving like intervals.

I also took away the clumsy mInterval = 0 logic and replaced with a proper boolean mIsInterval in the corresponding if conditions.

Comment 11

[Emanuele Costa]

Comment on attachment 542467 [details] [diff] [review]
nsGlobalWindow patch

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542467 [details] [diff] [review]
nsGlobalWindow patch

>+void nsGlobalWindow::SetIsBackground(PRBool aIsBackground)
>+{
>+  nsPIDOMWindow::SetIsBackground(aIsBackground);
>+  if(PR_FALSE == aIsBackground)
>+    ResetNonbackgroundTimers();

I would prefer that you do something like:

  PRBool resetTimers = (!aIsBackground && IsBackground());
  nsPIDOMWindow::SetIsBackground(aIsBackground);
  if (resetTimers) {
    ResetNonbackgroundTimers();
  }

>@@ -8869,16 +8877,17 @@ nsGlobalWindow::SetTimeoutOrInterval(nsI
>   if (aIsInterval) {
>     timeout->mInterval = interval;
>   }

That set should be unconditional.

>+nsresult nsGlobalWindow::ResetNonbackgroundTimers()

I think this may be better named "ResetTimersForNonBackgroundWindow".

>+  nsTimeout *timeout;
>+  TimeDuration nextInterval = 0;
>+  TimeDuration previousInterval = 0;

These should all be declared in the for loop (|timeout| in the loop header, and the rest when you actually go to use them.

>+  TimeStamp now;

And this should either be declared in the loop or initialized here, not every time through the loop.

>+  for (timeout = FirstTimeout(); IsTimeout(timeout); timeout = timeout->Next()) {
>+    if (timeout->mIsInterval || timeout->mNestingLevel >= DOM_CLAMP_TIMEOUT_NESTING_LEVEL) {

You only need this test to check whether the timer was clamped to start with, right?  But that's not the right check, since clamping also depends on the timer's mInterval.  I'd just take this check out altogether.

You might also want to stop the loop once you get to a timer for which |mWhen - now > TimeDuration::FromMilliseconds(gMinBackgroundTimeoutValue)|, since anything after that point clearly doesn't need resetting (timers are stored in mWhen order, and anything that far in the future didn't get clamped to start with).

>+      /* We switched from/to background, re-init the timeout/interval appropriately */

We know we switched _from_ background here.

>+      // Compute time to next timeout for interval timer
>+      // using DOMMinTimeoutValue().

It might not be an interval timer.  How about:

  // Compute the interval the timer should have had if it had not been set in a background window

>+      nextInterval = TimeDuration::FromMilliseconds(NS_MAX(timeout->mInterval,
>+                                                         PRUint32(DOMMinTimeoutValue())));

Fix the indentation here.  And maybe s/nextInterval/interval/?

>+        // unclamp
>+        timeout->mWhen = timeout->mWhen - TimeDuration::FromMilliseconds(gMinBackgroundTimeoutValue);
>+
>+        rv = timeout->mTimer->InitWithFuncCallback(TimerCallback, timeout,
>+                                                   nextInterval.ToMilliseconds(),
>+                                                   nsITimer::TYPE_ONE_SHOT);

This looks wrong.  Say timeout->mInterval is 10s and we set the timer up 5s ago.  Then nextInterval will be 10s, timeout->mWhen will be "now + 5s".  This code will reset timeout->mWhen to "now + 4s" and then set a 10s timer.   So the timeout will fire after 15s, not 10s

I think the right thing to do here is to compute |interval| as above, then get the delay from the nsTimeout's timer (call that variable |oldDelay|).  Now you know that the timer was set up at timeout->mWhen - oldDelay, and hence its new firing time should be timeout->mWhen - oldDelay + nextInterval.  This is what timeout->mWhen should be reset to, and the delay passed to InitWithFuncCallback should then be the new timeout->mWhen - now.  Note that this means you don't need the mWhen-now check either (which is good, because that check is wrong; just because mWhen is close to now doesn't mean the timer's firing time needs changing; it could be a 10s timer set up 9.997s ago).

The other thing you will need to do when changing mWhen values is to make sure to move the nsTimeout to the right place in the list so it remains sorted in mWhen order.

>+++ b/dom/base/nsGlobalWindow.h
>   // Non-zero interval in milliseconds if repetitive timeout
>   PRUint32 mInterval;

That comment needs fixing.

Comment 13

[Emanuele Costa]

Attachment: nsGlobalWindow patch

revised logic.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542610 [details] [diff] [review]
nsGlobalWindow patch

>+++ b/dom/base/nsGlobalWindow.cpp
>+    if (timeout->mWhen - now > TimeDuration::FromMilliseconds(gMinBackgroundTimeoutValue))
>+      break;

This could use curly braces and a comment explaining why it's safe to break.

>+    if (TimeDuration::FromMilliseconds(oldInterval) > interval) {

Save TimeDuration::FromMilliseconds(oldInterval) in a temporary, instead of computing it twice?

>+      PR_REMOVE_LINK(timeout);
>+      InsertTimeoutIntoList(timeout);

These should come before the attempt to reinit the timer and ensuing early return on failure.

Also, you nee to do the |timeout = timeout->Next()| bit _before_ doing this, no?  And add a comment that says that since mWhen is strictly smaller now (something we should enforce by only unclamping if mWhen > now to start with), it's safe to do the reinsertion because it won't affect our list traversal.

>   // Non-zero interval in milliseconds if repetitive timeout
>   PRUint32 mInterval;

This comment still needs fixing.

The rest looks good!

Comment 15

[Emanuele Costa]

Attachment: nsGlobalWindow patch

added linked list proper handling and extra break logic in the loop

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542642 [details] [diff] [review]
nsGlobalWindow patch

>+ nsresult rv = NS_OK;

Declare this where it's used; converting the break on failure into a return makes that possible.

>+  nsTimeout *timeout = nsnull;

Again, that can go in the for loop header.

>+    TimeDuration interval = 0;
>+    PRUint32 oldIntervalMillisecs = 0;

Declare these where they're used, not up front?

>  for (nsTimeout *timeout = FirstTimeout(); IsTimeout(timeout); timeout = timeout->Next()) {

You don't want to do the timeout = timeout->Next() bit here, since you already advanced through the list.

>-    if (timeout->mWhen < now)

This should be <=, I think, so you can guarantee that after unclamping timeout->mWhen is _strictly_ less than the old mWhen.  Since it might equal |now| at that point, have to use <= here.

>+    if ((timeout->mWhen - now > TimeDuration::FromMilliseconds(gMinBackgroundTimeoutValue))
>+        || IsFrozen() || mTimeoutsSuspendDepth) {

The IsFrozen() || mTimeoutsSuspendDepth check should just come up front in this function.

r=me with the above fixed.  Thank you!

I'm going to make the above changes and a few comment changes that I think explain better what we're doing and push the result to try.

Comment 17

[Boris Zbarsky [:bzbarsky]]

http://tbpl.mozilla.org/?tree=Try&rev=13845146c863

Comment 18

[Boris Zbarsky [:bzbarsky]]

Er, that was clearly buggy; removing the |timeout = timeout->Next()| means you have to manually do it when not unclamping.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, and one other thing I found while testing this: have to call Release() on the timeout after InsertTimeoutIntoList().  And reset its mFiringDepth.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The patch does fix bug 663020.

Pushed again to try as http://tbpl.mozilla.org/?tree=Try&rev=2d1d65848976

Comment 21

[Boris Zbarsky [:bzbarsky]]

That failed a bunch of tests because we built a place that treated mInterval as boolean: clearInterval was setting it to 0 to signal that a currently-running interval should be canceled and its timer not reset.

I changed that code to set mIsInterval instead, which fixes the failures I spot-checked locally, and pushed to try again: http://tbpl.mozilla.org/?tree=Try&rev=71fa7c387168

Comment 22

[Boris Zbarsky [:bzbarsky]]

Rollup patch pushed as http://hg.mozilla.org/integration/mozilla-inbound/rev/720ec564772b

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542642 [details] [diff] [review]
nsGlobalWindow patch

We should probably consider taking this on Aurora.  This fixes a behavior regression on at least one site.

Comment 24

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/720ec564772b

Comment 25

[LegNeato]

Comment on attachment 542642 [details] [diff] [review]
nsGlobalWindow patch

We decided this can wait for Firefox 7...denied for mozilla-aurora

Comment 26

[Trif Andrei-Alin[:AlinT]]

Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

Could you give some specific steps how can i check if it was fixed?
I've followed the steps in comment 1,and i've noticed a little time out when switching tabs.
Thanks!

Comment 27

[Trif Andrei-Alin[:AlinT]]

Thanks for the enlightenment E.
Setting resoution to VERIFIED FIXED.