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

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Florian Quèze [:florian]]

Attachment: Patch

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.

Comment 1

[Brian R. Bondy [:bbondy]]

Comment on attachment 648337 [details] [diff] [review]
Patch

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

Forwarding review to a widget peer, I'm not confident in reviewing this change myself.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 648337 [details] [diff] [review]
Patch

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.

Comment 3

[Florian Quèze [:florian]]

(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;

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 5

[Florian Quèze [:florian]]

Ok, thanks. I checked in the original version:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de433963900f

Thanks for the super fast review!

Comment 6

[Florian Quèze [:florian]]

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.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 648337 [details] [diff] [review]
Patch

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de433963900f

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 648337 [details] [diff] [review]
Patch

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.

Comment 12

[Florian Quèze [:florian]]

Marking checkin-needed for the aurora check-in.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6fe5bff856