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?

I think we should probably fix this.

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.

Emanuele wants to give this a shot.

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

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?

fixed typo

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.

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.

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.....

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 on attachment 542467 [details] [diff] [review] nsGlobalWindow patch Review of attachment 542467 [details] [diff] [review]: -----------------------------------------------------------------

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.

revised logic.

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!

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

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.

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

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

The patch does fix bug 663020. Pushed again to try as http://tbpl.mozilla.org/?tree=Try&rev=2d1d65848976

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

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

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 on attachment 542642 [details] [diff] [review] nsGlobalWindow patch We decided this can wait for Firefox 7...denied for mozilla-aurora

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!

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