844958 - Restart idle-daily timer logic broken

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[:Irving Reid (No longer working on Firefox)]

The patch in bug 792037 looks to me like it broke the code that reschedules the idle-daily timer, in cases where the timer fires before a full day has expired.

Bug 792037 changes how variable "newTime" is set, at around http://dxr.mozilla.org/mozilla-central/widget/xpwidgets/nsIdleService.cpp#l277; instead of just storing the timestamp of when we expected our timer to trigger, it stores the interval between "now" and the expected trigger.

Then, in line 290, we subtract "now" again, leading to a large negative (coerced to unsigned) number of milliseconds for the timer to re-wait.

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: Fix logic for rescheduling idle-daily timer if it fires early

Don't subtract "now" from "newTime" twice in calculation of remaining time for idle-daily timer

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 717994 [details] [diff] [review]
Fix logic for rescheduling idle-daily timer if it fires early

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

Argh, osrry about that. Not sure how I missed that subtract. What were the side effects?

Comment 3

[:Irving Reid (No longer working on Firefox)]

Found by inspection - didn't observe the effects directly, but I assume it would cause the daily timer to not fire for a very long time if it ever fired early.

Comment 4

[:Irving Reid (No longer working on Firefox)]

With the attached fix and logging in place, I'm seeing that the timer is often firing a 10-20 milliseconds early on OS X 10.7.

Backing out my fix but keeping my logging, daily-idle is failing on Mac; the timer triggers early and resets to a very large delay (45 days or so...)

Changing platforms affected to all, though I'm not sure whether it really does affect platforms other than Android and OS X.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Should we ask for tracking on this?

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to Irving Reid (:irving) from comment #5)
> Should we ask for tracking on this?

Sure, this sounds pretty bad.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 717994 [details] [diff] [review]
Fix logic for rescheduling idle-daily timer if it fires early

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +280,5 @@
>      // Timer returned early, reschedule to the appropriate time.
>      PRTime newTime = self->mExpectedTriggerTime - now;
>  
>      // Add 10 ms to ensure we don't undershoot, and never get a "0" timer.
>      newTime += 10 * PR_USEC_PER_MSEC;

I think the bug has also been favored by not renaming newTime, we need a better name that hilights the fact it's a delay.  Thus, I'd suggest renaming newTime to delayTime.

Comment 8

[(dormant account)]

Can we add some instrumentation to get test coverage for this?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Taras Glek (:taras) from comment #8)
> Can we add some instrumentation to get test coverage for this?

test coverage for the idle service has always been complicated, you may add code to "simulate" idle hooking into the service, but then that code could have its own bugs and could not be really effective. Or you would need a special setup to properly handle the system idles, those tests would take minutes so it's unlikely we can add them to common mochitests.
I suspect would be easier to add telemetry and keep an eye on its variations.
Though, maybe someone has better ideas.

Comment 10

[(dormant account)]

(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #9)
> (In reply to Taras Glek (:taras) from comment #8)
> > Can we add some instrumentation to get test coverage for this?
> 
> test coverage for the idle service has always been complicated, you may add
> code to "simulate" idle hooking into the service, but then that code could
> have its own bugs and could not be really effective. Or you would need a
> special setup to properly handle the system idles, those tests would take
> minutes so it's unlikely we can add them to common mochitests.
> I suspect would be easier to add telemetry and keep an eye on its variations.
> Though, maybe someone has better ideas.

that actually sounds better. Can someone file a followup bug to add telemetry to make sure idle service is behaving as expected?

Comment 11

[Marco Bonardo [:mak]]

I have filed bug 845558.

Comment 12

[:Irving Reid (No longer working on Firefox)]

Inbound with the variable name changed as requested by :mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/21dbfb381ded


Requesting tracking for versions 19 (where bug 792037 landed and caused this regression) forward.

The consequence of this bug is that the idle-daily service may not fire reliably on platforms where timer events can occur before the exact to-the-millisecond time they were requested; this applies to Mac OS and Android, at least.

With idle-daily broken a number of maintenance tasks such as telemetry gathering, Places and History expiration, and sqlite database vacuuming will not happen.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21dbfb381ded

Comment 14

[Alex Keybl [:akeybl]]

Please nominate for uplift to Aurora/Beta once this has had a little bake time on Nightly. As for FF19, AUS/blocklist are on the update/application-timer so does that mean Telemetry would be the most impacted consumer?

Comment 15

[:Irving Reid (No longer working on Firefox)]

FF 19-21 will trigger one idle-daily event shortly after startup, if it hasn't done one for at least a day. As long as the browser restarts now and again we'll get some events, just not the one-per-day we want.

I'm checking with the metrics team to see if our telemetry metrics for 19 are showing a significant difference; at this point it doesn't look like there has been a huge drop in submissions but I'll keep monitoring.

Comment 16

[Alex Keybl [:akeybl]]

Untracking for FF19 given our current understanding of the situation. Thanks Irving.

Please nominate for uplift at your earliest convenience.

Comment 17

[:Irving Reid (No longer working on Firefox)]

Attachment: Correctly reschedule idle-daily timer, as landed with nit fixed

Attaching patch as landed on trunk, for aurora/beta approval request. The only change from the reviewed version is the variable rename requested in https://bugzilla.mozilla.org/show_bug.cgi?id=844958#c7

(also, given that I landed this on trunk, am I supposed to set "status-firefox-22 -> fixed" or is that someone else's job?)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 792037
User impact if declined: idle-daily tasks will not be done daily for long running sessions
Testing completed (on m-c, etc.): Patch has been in nightly for a week with no issues
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 722806 [details] [diff] [review]
Correctly reschedule idle-daily timer, as landed with nit fixed

Approving for Aurora and Beta in support of better Telemetry data.

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Irving Reid (:irving) from comment #17)
> (also, given that I landed this on trunk, am I supposed to set
> "status-firefox-22 -> fixed" or is that someone else's job?)

The combination of target milestone and Resolved/Fixed implies status-firefox. You can always set it if you'd like once landed - it's a stylistic choice.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Alex Keybl [:akeybl] from comment #19)
> (In reply to Irving Reid (:irving) from comment #17)
> > (also, given that I landed this on trunk, am I supposed to set
> > "status-firefox-22 -> fixed" or is that someone else's job?)
> 
> The combination of target milestone and Resolved/Fixed implies
> status-firefox. You can always set it if you'd like once landed - it's a
> stylistic choice.

I usually only worry about setting it when I'm uplifting to other branches, just so you can see all in one place where the patch ended up.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6579262c994a
https://hg.mozilla.org/releases/mozilla-beta/rev/750d23f82309