Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsIdleService: timeout event doesn't fire regularly on non polling mode

RESOLVED FIXED in mozilla12

Status

()

Core
Widget
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

6 years ago
On non-polling mode device like Gonk (Android too?), after first idle event, the timer won't be rescheduled for next check.
(Assignee)

Comment 1

6 years ago
Created attachment 585355 [details] [diff] [review]
Part 1, Sanity check nextWaitTime
Attachment #585355 - Flags: review?
(Assignee)

Comment 2

6 years ago
Created attachment 585356 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event
Attachment #585356 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #585355 - Flags: review? → review?(mozstuff)
(Assignee)

Updated

6 years ago
Attachment #585356 - Flags: review? → review?(mozstuff)
Hi Kan-Ru, I would like to review this, but it won't be until tonight (CET).
Comment on attachment 585355 [details] [diff] [review]
Part 1, Sanity check nextWaitTime

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

Yeah, that will probably work better :)
Attachment #585355 - Flags: review?(mozstuff) → review+
Comment on attachment 585356 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event

>+#include "android/log.h"
>+#define LOG(args...)                                            \
>+    __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
>+#ifdef VERBOSE_LOG_ENABLED
>+# define VERBOSE_LOG(args...)                           \
>+    __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
>+#else
>+# define VERBOSE_LOG(args...)                   \
>+    (void)0
>+#endif
>+

Are you sure you want to add this part to the non-android specific part of the source?

>+    /* If any listeners have entered "non-idle" state, we need to
>+     * recalculate the timeout */
>+    if (notifyList.Count() > 0) {
>+      aNoTimeReset = false;
>+    }
>+

I see where you are going with this, but I'm thinking it might be better to re factor a bit...

The only place the argument (aNoTimeReset) can become true, is from the call from ResetIdleTimeOut, the idea with it, in this context, iirc, is to prevent constant resetting of the timer.

I am aware that your code does not change this behavior, but it is kind of overwriting the purpose of the parameter, so my thoughts are along the lines of getting rid of the parameter all together, and make the detection internally in the function, I'm imagining a logic along the lines of:

---

If (user activity detected)
{
  if (anything idle)
  {
    send "non-idle" events;
    recalc timers;
  }
  else // nothing idle
  {
    bail out; // a timer is already running
  }
}
else // idling
{
  for (any new stuff that has become idle)
  {
   send "idle" event;
  }
  recalc timers
}

And it could probably be made more pretty by splitting it up into a number of smaller functions :)

What do you think about this idea?  If you think it is too time consuming at the moment we could leave your change as a quick fix, and then make a new bug with this and assign it to me, then I can look into it probably within the next week or so, where I'll have some evenings to spare for this.
(Assignee)

Comment 6

6 years ago
(In reply to Mike Kristoffersen (:MikeK) from comment #5)
> Comment on attachment 585356 [details] [diff] [review]
> Part 2, Reschedule timer after non-idle event
> 
> >+#include "android/log.h"
> >+#define LOG(args...)                                            \
> >+    __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
> >+#ifdef VERBOSE_LOG_ENABLED
> >+# define VERBOSE_LOG(args...)                           \
> >+    __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
> >+#else
> >+# define VERBOSE_LOG(args...)                   \
> >+    (void)0
> >+#endif
> >+
> 
> Are you sure you want to add this part to the non-android specific part of
> the source?

Oops. I didn't mean to include this. It just... slipped in ;)

> >+    /* If any listeners have entered "non-idle" state, we need to
> >+     * recalculate the timeout */
> >+    if (notifyList.Count() > 0) {
> >+      aNoTimeReset = false;
> >+    }
> >+
> 
> I see where you are going with this, but I'm thinking it might be better to
> re factor a bit...
> 
> The only place the argument (aNoTimeReset) can become true, is from the call
> from ResetIdleTimeOut, the idea with it, in this context, iirc, is to
> prevent constant resetting of the timer.
> 
> I am aware that your code does not change this behavior, but it is kind of
> overwriting the purpose of the parameter, so my thoughts are along the lines
> of getting rid of the parameter all together, and make the detection
> internally in the function, I'm imagining a logic along the lines of:
> 
> ---
> 
> If (user activity detected)
> {
>   if (anything idle)
>   {
>     send "non-idle" events;
>     recalc timers;
>   }
>   else // nothing idle
>   {
>     bail out; // a timer is already running
>   }
> }
> else // idling
> {
>   for (any new stuff that has become idle)
>   {
>    send "idle" event;
>   }
>   recalc timers
> }
> 
> And it could probably be made more pretty by splitting it up into a number
> of smaller functions :)
> 
> What do you think about this idea?  If you think it is too time consuming at
> the moment we could leave your change as a quick fix, and then make a new
> bug with this and assign it to me, then I can look into it probably within
> the next week or so, where I'll have some evenings to spare for this.

I agree. At first I also want to remove the parameter all together, but then thought it might be useful to force recalculate the timer, so I took the minimum approach instead.

I will attach an refactored version later.
(Assignee)

Comment 7

6 years ago
Created attachment 585698 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event, v2
Assignee: nobody → kchen
Attachment #585356 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #585356 - Flags: review?(mozstuff)
Attachment #585698 - Flags: review?(mozstuff)
Comment on attachment 585698 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event, v2

I like the bootstrapTimer to ensure everything gets up and running :) assuming you tested it on both a system that uses poll mode and one that doesn't then I only have one comment left:

 void
 nsIdleService::ResetIdleTimeOut()
 {
-  // A zero in mLastIdleReset indicates that this function has never been
-  // called.
-  bool calledBefore = mLastIdleReset != 0;
   mLastIdleReset = PR_IntervalToSeconds(PR_IntervalNow());
   if (!mLastIdleReset) mLastIdleReset = 1;
 
   // Now check if this changes anything
   // Note that if we have never been called before, we cannot do the
   // optimization of passing true to CheckAwayState, which avoids
   // calculating the timer (because if we have never been called before,
   // we need to recalculate the timer and start it there).
-  CheckAwayState(calledBefore);
+  CheckAwayState();
 }

The comment here, about passing true to CheckAwayState might be confusing to someone who doesn't know the history of the function ;)

Otherwise it looks fine r+.
Attachment #585698 - Flags: review?(mozstuff) → review+
(Assignee)

Comment 9

6 years ago
(In reply to Mike Kristoffersen (:MikeK) from comment #8)
> Comment on attachment 585698 [details] [diff] [review]
> Part 2, Reschedule timer after non-idle event, v2
> 
> I like the bootstrapTimer to ensure everything gets up and running :)
> assuming you tested it on both a system that uses poll mode and one that
> doesn't then I only have one comment left
> [..]

I found we might want reintroduce the parameter to explicit recalculate the timer when new observer is added. Otherwise it might occur in conjunction with user activity then the timer will not be scheduled. One obvious case is when I test it in the web console the observer is added immediately after I hit the enter key.
What would happen if you called "RescheduleIdleTimer" after adding an observer?
(Assignee)

Comment 11

6 years ago
RescheduleIdleTimer needs idleTime to calculate the next wait time, so if I want to call RescheduleIdleTimer in addIdleObserver it would duplicate a large portion of CheckAwayState.
(Assignee)

Comment 12

6 years ago
Created attachment 585996 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event, v3

how about this one
Attachment #585996 - Flags: review?(mozstuff)
Comment on attachment 585996 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event, v3


>   // Set the time for last user activity.
>   mLastHandledActivity = curTime - idleTime;
> 
>   /**
>+   * Too short since last check.
>+   * Bail out
>+   */
>+  if (!lastTime && !aNewObserver) {
>+    return;
>+  }
>+

Since we only use seconds internally, it might be an idea to move this one up a bit, as this will probably be the most frequent reason for bailing out (think about mouse moves)

>+  bool userActivity = lastTime > idleTime;
>+
>+  if (userActivity) {
>+    if (TryNotifyBackState(idleTime) || idleTime) {
>+      RescheduleIdleTimer(idleTime);
>+    }
>+  }
>+
>+  if (!userActivity || aNewObserver || bootstrapTimer) {
>+    TryNotifyIdleState(idleTime);
>+    RescheduleIdleTimer(idleTime);
>+  }

This took me a while to verify, but I think it is as it should be.

>+bool
>+nsIdleService::TryNotifyBackState(PRUint32 aIdleTime)
>+{
>+  // We need a text string to send with any state change events.
>+  nsAutoString timeStr;
>+  timeStr.AppendInt(aIdleTime);
>+

We actually don't need to create this string unless we find something that should be notified, and usually we won't find anything, so I suggest we make it conditional to whether we find something.

>+bool
>+nsIdleService::TryNotifyIdleState(PRUint32 aIdleTime)
>+{
>+  // We need a text string to send with any state change events.
>+  nsAutoString timeStr;
>+  timeStr.AppendInt(aIdleTime);
>+

Same as above...

----

I tried playing with it locally, I'll attach a diff or it updated to trunk to illustrate what I was thinking...

I can't help to think that the code seems overly complicated, especially that CheckAwayState, it is much better after you split it into several functions, but still... (this is by no means introduced by your patch, it was like that originally) - it might be a rewarding exercise to make it neater - but lets leave that for another time :)
Created attachment 586253 [details] [diff] [review]
Suggestion of where things could be moved to

This is a merge of the two parts (attachment 585355 [details] [diff] [review] & 585996) updated to trunk and with an illustration of the edits I suggested in the review.
(Assignee)

Comment 15

6 years ago
(In reply to Mike Kristoffersen (:MikeK) from comment #14)
> Created attachment 586253 [details] [diff] [review]
> Suggestion of where things could be moved to
> 
> This is a merge of the two parts (attachment 585355 [details] [diff] [review]
> [review] & 585996) updated to trunk and with an illustration of the edits I
> suggested in the review.

Your patch looks great; I think you can land it as is. :)
Comment on attachment 585996 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event, v3

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

r+ if updated with my previous comments
Attachment #585996 - Flags: review?(mozstuff) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #15)
> (In reply to Mike Kristoffersen (:MikeK) from comment #14)
> > Created attachment 586253 [details] [diff] [review]
> > Suggestion of where things could be moved to
> > 
> > This is a merge of the two parts (attachment 585355 [details] [diff] [review]
> > [review] & 585996) updated to trunk and with an illustration of the edits I
> > suggested in the review.
> 
> Your patch looks great; I think you can land it as is. :)

But complicate the review logic :) - It's your patch with a few lines moved, besides I think I lost my commit access due to inactivity (I have no idea what the rules for committing are these days).
(Assignee)

Comment 18

6 years ago
Created attachment 586354 [details] [diff] [review]
Part 1, Sanity check nextWaitTime

--
Attachment #585355 [details] [diff] - Attachment is obsolete: true
Attachment #586354 - Flags: review?
(Assignee)

Comment 19

6 years ago
Created attachment 586355 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event

--
Attachment #585356 [details] [diff] - Attachment is obsolete: true
Attachment #586355 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #585355 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #585698 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #585996 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #586354 - Flags: review? → review?(mozstuff)
(Assignee)

Updated

6 years ago
Attachment #586355 - Flags: review? → review?(mozstuff)
Attachment #586354 - Flags: review?(mozstuff) → review+
Attachment #586355 - Flags: review?(mozstuff) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

6 years ago
Created attachment 587568 [details] [diff] [review]
Part 1, Sanity check nextWaitTime
Attachment #586354 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 587569 [details] [diff] [review]
Part 2, Reschedule timer after non-idle event
Attachment #586253 - Attachment is obsolete: true
Attachment #586355 - Attachment is obsolete: true
Kan-Ru, please make sure to use a proper name as the reviewer in the commit message. "mozstuff" is not such a name.
http://hg.mozilla.org/integration/mozilla-inbound/rev/e44700517c2d
http://hg.mozilla.org/integration/mozilla-inbound/rev/751ce658ce73
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/e44700517c2d
https://hg.mozilla.org/mozilla-central/rev/751ce658ce73
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
The changes to the idle service here appear to have drastically increased the frequency of Bug 517482.
Blocks: 517482
Hmm... I'll try to see if I can find a reason for this in the coming weekend...
Ok, I think I have an idea now...

There are at least two things going on - the random errors that we have had for a long time where the timer isn't timing out within the 1s grace that the test allows is probably due to the fact that the service internally only calculates with a resolution of 1s, that can be fixed by increasing the internal resolution or increasing the grace period in the test, I propose increasing the internal resolution.

The second thing is that some times the service fails to restart it's internal timer, in nsIdleService::CheckAwayState(bool aNewObserver):
if (userActivity) {
    if (TryNotifyBackState(idleTime) || idleTime) {
      RescheduleIdleTimer(idleTime);
    }

here RescheduleIdleTimer isn't called, as the second "if" evaluates to false, I'm not sure why this happens yet, but have been able to provoke it several times in a test environment. 

(However I suspect that it has to do with the service running non-instantly, like, because time is passing while the code is executing)

This could lead to the failing test cases where it complains that there are no call backs.

I'll try to provide a fix for one or both of these one of the coming days.  Maybe we should open new bugs instead of piggybacking it on this one?
(In reply to Mike Kristoffersen (:MikeK) from comment #27)
> I'll try to provide a fix for one or both of these one of the coming days. 
> Maybe we should open new bugs instead of piggybacking it on this one?

Yes please!
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> (In reply to Mike Kristoffersen (:MikeK) from comment #27)
> > I'll try to provide a fix for one or both of these one of the coming days. 
> > Maybe we should open new bugs instead of piggybacking it on this one?
> 
> Yes please!

I have opened bug 720490 and bug 720493 to handle the issues I found.
You need to log in before you can comment on or make changes to this bug.