602085 - idle-daily has become idle-session and is waiting for an idle time of more than 3 days

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The patch in bug 544240 forgot to set back PREF_LAST_DAILY, the result is that idle.lastDailyNotification is updated last at April, and idle-daily is not served anymore once a day, but once a session.

MikeK suggested to also handle the case where the pref is wrongly set in future (due to user changing system time), that should be easy to do, so I'll do it.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Asking blocking because idle-daily is used for heavy tasks or other stuff that is better not run per session.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this nice code

me->mIdleService->AddIdleObserver(me, MAX_IDLE_POLL_INTERVAL*1000);

is making idle-daily (currently session) wait for an idle of 300 000 seconds since the second param of AddIdleObserver is seconds, not milliseconds :(

this could explain a bunch of stuff, broken frecency, databases not vacuumed, database page_size not changing...

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Asking r to Shawn since he already reviewed idle code in the past.

I suggest (And I'll do) to file a bug to find a way to test idle, I have no idea how we could do it, but maybe someone has ideas and we could collect them there.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'd suggest to re-evaluate blocking of this bug for beta7, due to latest discovery about the fact idle-daily stuff never runs in current betas.

Comment 5

[Shawn Wilsher :sdwilsh]

While it's true that we don't do what we say we do, the API itself isn't broken, so I think it's OK.  It does, however, mean we aren't doing things we thought we were, which is sad, but I think we can live with until this is [likely] fixed in beta 8.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 481224 [details] [diff] [review]
patch v1.0

>+  // Check time of the last idle-daily notification.  If it was less than 24
>+  // hours ago listen for idle, otherwise set a timer for 24 hours from now.
This comment says the opposite of what the code does.  The good news is that the code is correct.

>+  bool mObservesIdle;
Add a comment about this like we do for other members please.

r=sdwilsh

Comment 7

[Shawn Wilsher :sdwilsh]

See also, bug 564118.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Addressed comments.

I'm going through tryserver before pushing because I want to check what effect could have re-enabling idle-daily on tests and talos. Any effect could not be considered a regression, but it is better being prepared.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

good news, tryserver tells me this can land.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/446f80f30026

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

re-enabling idle-daily was causing oranges on some frecency bug on osx and windows (tinderboxes are permanently idle, so this is not a surprise). we do frecency recalculation on idle-daily but I did not notice this on tryserver because I also had another unelated patch that was making those tests more robust.
To fix this I've pushed a followup that temporarily disables the history idle-daily observer, so that the only frecency changes are those activated by the tests.
http://hg.mozilla.org/mozilla-central/rev/97949d240701