Closed Bug 844958 Opened 7 years ago Closed 7 years ago

Restart idle-daily timer logic broken

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox19 - ---
firefox20 + fixed
firefox21 + fixed
firefox22 + fixed

People

(Reporter: Irving, Assigned: Irving)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Don't subtract "now" from "newTime" twice in calculation of remaining time for idle-daily timer
Attachment #717994 - Flags: review?(mak77)
Attachment #717994 - Flags: feedback?(jmathies)
Attachment #717994 - Attachment is patch: true
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?
Attachment #717994 - Flags: feedback?(jmathies) → feedback+
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.
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.
OS: Android → All
Should we ask for tracking on this?
(In reply to Irving Reid (:irving) from comment #5)
> Should we ask for tracking on this?

Sure, this sounds pretty bad.
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.
Attachment #717994 - Flags: review?(mak77) → review+
Can we add some instrumentation to get test coverage for this?
(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.
(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?
I have filed bug 845558.
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.
https://hg.mozilla.org/mozilla-central/rev/21dbfb381ded
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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?
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.
Untracking for FF19 given our current understanding of the situation. Thanks Irving.

Please nominate for uplift at your earliest convenience.
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
Attachment #717994 - Attachment is obsolete: true
Attachment #722806 - Flags: approval-mozilla-beta?
Attachment #722806 - Flags: approval-mozilla-aurora?
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.
Attachment #722806 - Flags: approval-mozilla-beta?
Attachment #722806 - Flags: approval-mozilla-beta+
Attachment #722806 - Flags: approval-mozilla-aurora?
Attachment #722806 - Flags: approval-mozilla-aurora+
(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.
(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
You need to log in before you can comment on or make changes to this bug.