Last Comment Bug 779843 - nsIdleService never fires the "back" notification if there's only one listener
: nsIdleService never fires the "back" notification if there's only one listener
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Florian Quèze [:florian] [:flo] (PTO until August 29th)
:
Mentors:
: 789898 (view as bug list)
Depends on:
Blocks: Instantbird 720493
  Show dependency treegraph
 
Reported: 2012-08-02 07:21 PDT by Florian Quèze [:florian] [:flo] (PTO until August 29th)
Modified: 2012-10-02 02:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed


Attachments
Patch (1.62 KB, patch)
2012-08-02 07:21 PDT, Florian Quèze [:florian] [:flo] (PTO until August 29th)
vladimir: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-08-02 07:21:09 PDT
Created attachment 648337 [details] [diff] [review]
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] 2012-08-02 08:06:54 PDT
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] 2012-08-02 08:49:57 PDT
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] [:flo] (PTO until August 29th) 2012-08-02 09:40:18 PDT
(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] 2012-08-02 10:02:29 PDT
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] [:flo] (PTO until August 29th) 2012-08-02 10:43:42 PDT
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] [:flo] (PTO until August 29th) 2012-08-02 10:48:34 PDT
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] [:flo] (PTO until August 29th) 2012-08-02 10:50:20 PDT
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] 2012-08-02 19:08:37 PDT
https://hg.mozilla.org/mozilla-central/rev/de433963900f
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-03 11:42:40 PDT
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] 2012-08-03 13:11:57 PDT
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 2012-08-06 12:12:04 PDT
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] [:flo] (PTO until August 29th) 2012-08-07 11:09:34 PDT
Marking checkin-needed for the aurora check-in.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-08-07 14:08:03 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6fe5bff856
Comment 14 Ludovic Hirlimann [:Usul] 2012-10-02 02:29:34 PDT
*** Bug 789898 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.