nsIdleService never fires the "back" notification if there's only one listener

RESOLVED FIXED in Firefox 16



5 years ago
5 years ago


(Reporter: florian, Assigned: florian)


(Blocks: 1 bug, {regression})

Dependency tree / graph

Firefox Tracking Flags

(firefox15- wontfix, firefox16+ fixed)



(1 attachment)

Created attachment 648337 [details] [diff] [review]

My code has an idle listener and I noticed a while ago (after updating to Mozilla 12 or 13, I'm not sure) that the "back" notification isn't received reliably any more.

When starting my build with NSPR_LOG_MODULES=idleService:5, with an idle observer of 60s, just before the idle notification is received, I see this in my terminal:
-1600657632[60abf0]: idleService: Get idle time: polled 60010 msec, valid = 1
-1600657632[60abf0]: idleService: Get idle time: time since reset 60008 msec
-1600657632[60abf0]: idleService: Idle timer callback: current idle time 60008 msec
-1600657632[60abf0]: idleService: ReconfigureTimer: no idle or waiting observers
-1600657632[60abf0]: idleService: Idle timer callback: tell observer 1aeaace0 user is idle
-1600657632[60abf0]: idleService: Get idle time: polled 60013 msec, valid = 1
-1600657632[60abf0]: idleService: Get idle time: time since reset 60011 msec

I think I shouldn't have the "ReconfigureTimer: no idle or waiting observers" line here.

Inspecting the code makes me believe this issue happens only if there's only one idle observer (or maybe also if all idle observers are expected to become idle at the same time; if that's possible).

After applying the attached patch, this line disappears from my terminal output and my idle observer correctly receives the "back" notification.
Attachment #648337 - Flags: review?(netzen)
Comment on attachment 648337 [details] [diff] [review]

Review of attachment 648337 [details] [diff] [review]:

Forwarding review to a widget peer, I'm not confident in reviewing this change myself.
Attachment #648337 - Flags: review?(netzen) → review?(vladimir)
Comment on attachment 648337 [details] [diff] [review]

Looks fine to me; could get a similar effect by setting

  mAnyObserverIdle = numberOfPendingNotifications > 0;

before the ReconfigureTimer() call (and moving the numberOfPendingNotifications declaration before too).  Might be a little easier to read than burying the mAnyObserverIdle setting above, but looks fine either way.
Attachment #648337 - Flags: review?(vladimir) → review+
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> Comment on attachment 648337 [details] [diff] [review]
> Patch
> Looks fine to me; could get a similar effect by setting
>   mAnyObserverIdle = numberOfPendingNotifications > 0;

Wouldn't this set mAnyObserverIdle to false if it was already true (because some observers had already become idle) but there are no new observer becoming idle?

I don't know this code enough to determine quickly if this can really happen or not, so the version in attachment 648337 [details] [diff] [review] seems less risky to me.

Of course we could also write:
  if (numberOfPendingNotifications > 0)
    mAnyObserverIdle = true;
Er yes, that's a good point.  I originally had the if() in my head and then thought "hey we can simplify this" :)  But the original version is fine as well.
Ok, thanks. I checked in the original version:

Thanks for the super fast review!
Already a few versions of gecko have shipped with this bug, but I think it's a regression that could affect add-on authors, so we may want to fix it sooner than Firefox 17. Setting tracking flags.
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
Comment on attachment 648337 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720493
User impact if declined: Add-ons relying on receiving a notification when the user is no longer idle may be broken.

Risk to taking this patch (and alternatives if risky): I think this is a low risk patch.
Attachment #648337 - Flags: approval-mozilla-beta?
Attachment #648337 - Flags: approval-mozilla-aurora?
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
I'd like to get a sense of how many addons are affected by this, adding Jorge to the bug to weigh in. At this point in the Firefox 15 Beta cycle I'm not very inclined towards taking this but would consider for uplift to Aurora based on feedback from addons about how widespread this issue is for our addons.
tracking-firefox16: ? → +
Many add-ons use the nsIIdleService (about 60 on AMO), but I haven't heard of any that are broken because of this. Given that this shipped in 13, either it isn't a big deal or developers have coded around it.

I suggest this is uplifted to Aurora, but it's not necessary for Beta.
status-firefox15: --- → wontfix
status-firefox16: --- → affected
tracking-firefox15: ? → -
Comment on attachment 648337 [details] [diff] [review]

We'll take this on Aurora then, I'm wontfixing/minusing for Beta because it's too late in the cycle to risk a change like this to addons.
Attachment #648337 - Flags: approval-mozilla-beta?
Attachment #648337 - Flags: approval-mozilla-beta-
Attachment #648337 - Flags: approval-mozilla-aurora?
Attachment #648337 - Flags: approval-mozilla-aurora+
Marking checkin-needed for the aurora check-in.
Keywords: checkin-needed
status-firefox16: affected → fixed
Keywords: checkin-needed
Duplicate of this bug: 789898
You need to log in before you can comment on or make changes to this bug.