Closed Bug 564118 Opened 14 years ago Closed 14 years ago

Fennec wakes up every second due to IdleService

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
Other
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec2.0b2+)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: mjarvin, Assigned: azakai)

References

Details

(Whiteboard: battery)

Attachments

(1 file, 5 obsolete files)

Patch v5
5.19 KB, patch
MikeK
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100407 Ubuntu/9.04 (jaunty) Shiretoko/3.5.9 AutoPager/0.5.2.2 (http://www.teesoft.info/)
Build Identifier: Fennec 2.0a1pre 20100505013402

On mobile device, timer which is triggered every second causes lots of battery consumption. Currently this happens with Fennec constantly. I have identified at least two places, where such a timer is used. One is in XPCJSRuntime::WatchdogMain and other is in Fennecs chrome (idleServiceObserver in BrowserView.js). 

Timer in XPCJSRuntime was already discussed when #477187 was committed, but for my opinion mobile devices aren't taken into consideration with current implementation.

Althrough idleServiceObserver is only called once per idle/back situation, it creates one second timer internally in nsIdleService::CheckAwayState. 

Reproducible: Always

Steps to Reproduce:
1. Start Fennec in debugger
2. Open about:blank
3. Leave device idle
4. Put a breakpoint to XPCJSRuntime::WatchdogMain(void *arg) or  nsIdleService::CheckAwayState(bool aNoTimeReset)



Actual Results:  
Breakpoints are constantly triggered in one second interval.

Expected Results:  
Breakpoints should not be triggered so often.
Whiteboard: battery
Are you sure that we are only waking up for this reason? Last time I looked into this, firefox would wake up > 20 times a second for various reasons even if no window was open at all, and > 100 for google.com, so fixing this particular bug didn't seem worth while. Fennec might behave much better though.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I checked this case with powertop on device. For my opinion this seems to be case when device is on idle state: I start fennec from commandline using -url about:blank. I wait for couple of minutes to let everything settle down and start powertop. Fennec process is waking up from 180 to about 250 times in three minute period. For my opinion this corresponds to couple of one sec timers.

I have also noticed, that there comes massive amount of wakeups, if ssl-connection fails (for example if extension update check fails).
The Watchdog issue is handled in bug 568730 (which has a patch waiting for review).

The ssl issue is dealt with in bug 567339 (seems it isn't an SSL issue, but to do with images; there is a patch that will fix that, in bug 359608, waiting for feedback).

I will look into the idleService issue.
(In reply to comment #0)
> Althrough idleServiceObserver is only called once per idle/back situation, it
> creates one second timer internally in nsIdleService::CheckAwayState. 
> 

I tested this now and I see a 5-second timer in nsIdleService. Which makes sense since

   #define MIN_IDLE_POLL_INTERVAL 5

there. Can you give more details about your build, and under what conditions you see a 1-second timer?
I'm using qt port, but last time I checked this happened also in gtk port. 

MIN_IDLE_POLL_INTERVAL doesn't affect because in 

if (UsePollMode() &&
       anyOneIdle &&
       nextWaitTime > MIN_IDLE_POLL_INTERVAL) {
     nextWaitTime = MIN_IDLE_POLL_INTERVAL;
}

anyOneIdle is always false.

And 5 second doesn't sound to be enough for mobile device. It would prevent for example kernel sleep states.
(In reply to comment #5)
> I'm using qt port, but last time I checked this happened also in gtk port. 
> 

I just tried with both the Qt and GTK ports - I see 5 seconds in both, not 1 second. I am testing with this:

--- a/widget/src/xpwidgets/nsIdleService.cpp
+++ b/widget/src/xpwidgets/nsIdleService.cpp
@@ -272,6 +272,7 @@ nsIdleService::IdleTimerCallback(nsITime
 void
 nsIdleService::CheckAwayState(bool aNoTimeReset)
 {
+printf("!!idletimer!! %x\n", PR_Now());
   /**
    * Find our last detected idle time (it's important this happens before the
    * call below to GetIdleTime, as we use the two values to detect if there

It prints once per 5 seconds. How are you testing, that you see once per second? (Perhaps you are seeing some other timer fire 1/second?)

> MIN_IDLE_POLL_INTERVAL doesn't affect because in 
> 
> if (UsePollMode() &&
>        anyOneIdle &&
>        nextWaitTime > MIN_IDLE_POLL_INTERVAL) {
>      nextWaitTime = MIN_IDLE_POLL_INTERVAL;
> }
> 
> anyOneIdle is always false.
> 

Why isn't anyOneIdle being set to true in the loop there? It works for me here as intended.

> And 5 second doesn't sound to be enough for mobile device. It would prevent for
> example kernel sleep states.

Yes, I agree, but lower priority than other 1/sec timers (e.g. JS GC) if this is 1/5 seconds. So let's figure that out first.
(In reply to comment #5)
> I'm using qt port, but last time I checked this happened also in gtk port. 
> 
> MIN_IDLE_POLL_INTERVAL doesn't affect because in 
> 
> if (UsePollMode() &&
>        anyOneIdle &&
>        nextWaitTime > MIN_IDLE_POLL_INTERVAL) {
>      nextWaitTime = MIN_IDLE_POLL_INTERVAL;
> }
> 
> anyOneIdle is always false.
> 
> And 5 second doesn't sound to be enough for mobile device. It would prevent for
> example kernel sleep states.

anyOneIdle is set to true when someone is found to be idle. What this code does, it creates timer that will expire in 5 seconds or less if anyone is idle, just to check if we wouldn't be idle anymore. This is total waste of resources if return from to idle state can be observed with less consuming methods. At least with Maemo 6, there comes a signal about returning to active state, so this should not be used for it. This can be done easily by setting nsIdleServiceQt::UsePollMode() to return false and calling nsIdleService::ResetIdleTimeOut() when we observe the system active. Also nsIdleServiceQt::PollIdleTime(PRUint32 *aIdleTime) should only return false for Maemo 6, since that implementation is not used for it.
(In reply to comment #7)
> if return from to idle state can be observed with less consuming methods. At
> least with Maemo 6, there comes a signal about returning to active state, so
> this should not be used for it. This can be done easily by setting
> nsIdleServiceQt::UsePollMode() to return false and calling
> nsIdleService::ResetIdleTimeOut() when we observe the system active. Also
> nsIdleServiceQt::PollIdleTime(PRUint32 *aIdleTime) should only return false for
> Maemo 6, since that implementation is not used for it.

So actually, we can poll as long as we get "system-idle" or whatever notification of user not active anymore, but after that the polling needs to be stopped until we get "system-active" notification.
Still about this polling. What is the purpose for it? Is it just for a safety if we don't notice ending of idle time when user interacts again? If so, 5s timer for it is quite brutal way of doing it. Shouldn't we just make sure that ending of idle time is always noticed and nsIdleService::ResetIdleTimeOut() is then used for notifying the observers about the change?
Depends on: 584660
IdleService notifications are for *both* when the user idles for X seconds, and for when the user becomes active after that. Detecting the end of idle time - the second part - is usually easy (see e.g. widget/src/qt/nsWindow.cpp:2538). But detecting that the user has been idle for X seconds is not supported on all platforms, so the IdleService code simulates that using polling, basically manual checking of the idle time.

If we find that any of our mobile platforms support that without polling, we should fix it.
Summary: Fennec is waking up every second when idle because of timers → Fennec is waking up every 5 seconds due to IdleService
is this bugs
 "Bug 508518 - Implement nsUITimerCallback with one-shot timer" - 
 "Bug 477850 - Avoid watchdog ticks when idle" connected somehow?
(In reply to comment #11)
> is this bugs
>  "Bug 508518 - Implement nsUITimerCallback with one-shot timer" - 

Another 1/5 seconds timer that would be good to get rid of. Partial list is over here:

https://wiki.mozilla.org/Mobile/Powersaving/Wakeups#Some_Wakeups_of_Note

>  "Bug 477850 - Avoid watchdog ticks when idle" connected somehow?

This looks the same as bug 568730, which has an r+'ed patch and will hopefully land soon. I'll comment in that bug.
(In reply to comment #10)
> IdleService notifications are for *both* when the user idles for X seconds, and
> for when the user becomes active after that. Detecting the end of idle time -
> the second part - is usually easy (see e.g. widget/src/qt/nsWindow.cpp:2538).
> But detecting that the user has been idle for X seconds is not supported on all
> platforms, so the IdleService code simulates that using polling, basically
> manual checking of the idle time.
> 

Yes those are for both, but I still don't get why we need to know that user has been idle for X seconds. Shouldn't we just concentrate on when the next idle observer needs to be idled. When nsIdleService::CheckAwayState() is called, it will pick the sleeping time to be the interval to next idle observer going idle. So if there is not going to be any action before that or any new observers added, nsIdleService will wake up when the next idle observer needs to be idled. Isn't this the only thing that we need to know about idle time?
> Yes those are for both, but I still don't get why we need to know that user has
> been idle for X seconds. Shouldn't we just concentrate on when the next idle
> observer needs to be idled. When nsIdleService::CheckAwayState() is called, it
> will pick the sleeping time to be the interval to next idle observer going
> idle. So if there is not going to be any action before that or any new
> observers added, nsIdleService will wake up when the next idle observer needs
> to be idled. Isn't this the only thing that we need to know about idle time?

Yes, I believe this code could be rewritten that way - schedule a wakeup for when the next idle listener wants to be notified of X seconds idle time, and reschedule it if another registers meanwhile, etc.
We will fix the polling (5-second timer) in bug 584660.

The remaining thing to figure out is whether there is a 1-second timer caused by a bug with the idleservice not being initialized properly (which apparently is fixable with ResetIdleTimeOut in the patch in bug 584660). I will investigate.
Assignee: nobody → azakai
Summary: Fennec is waking up every 5 seconds due to IdleService → Fennec is waking up every second due to IdleService
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
I can't confirm a 1-second timer in a gtk or a qt build. I see just the 5-second timer, which we will fix for Meego in bug 584660 (it's already fixed for Android). Note: For finding out what the causes of wakeups are, this experimental approach has been useful here:

  http://mozakai.blogspot.com/2010/09/tools-for-debugging-wakeups.html

As for fixing the idle service in general, after looking into it, it would require changing the interface and all the specific idleservice implementations. So just fixing it for Fennec as mentioned before seems the best plan for now.

So, marking this as invalid. And I think we can remove the blocking-fennec as well.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
The new background cycle collector really needs long sleep times to function properly, if we can fix this properly for the entire platform and not just fennec that would be awesome.
(In reply to comment #17)
> The new background cycle collector really needs long sleep times to function
> properly, if we can fix this properly for the entire platform and not just
> fennec that would be awesome.

Not sure I understand. We were talking just now about about disabling idleservice polling on Fennec, I don't follow how that connects? Or is that regarding the JS Watchdog timer stuff mentioned previously in this bug? I believe that was fixed in bug 568730? Or is there some new background cycle collector issue we should be aware of?
The background cycle collector needs the native event loop to sleep as long as possible. Whatever we can do for that would be great.
(In reply to comment #19)
> The background cycle collector needs the native event loop to sleep as long as
> possible. Whatever we can do for that would be great.

Oh, ok, now I see.

Essentially all the work in Fennec on timers will benefit the entire platform. The idle service is the only exception. But we should get around to fixing that too.

As a reminder for that, reopening & renaming this bug. But we should remove blocking-fennec, I think, fixing this properly should be a long-term goal.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Fennec is waking up every second due to IdleService → Rewrite IdleService to never run unnecessary timers for polling
(In reply to comment #16)
> I can't confirm a 1-second timer in a gtk or a qt build. I see just the
> 5-second timer, which we will fix for Meego in bug 584660 (it's already fixed
> for Android). Note: For finding out what the causes of wakeups are, this
> experimental approach has been useful here:
> 

What was the use case you were using. I tried this once more and when testing with the device and just starting browser to "about:home" and not touching anywhere, it will result to nsIdleService::CheckAwayState being called between one second interval. As soon as I will touch to screen it will settle to that 5s interval.
Which device? And can you reproduce this on any kind of desktop build?

Just to make sure we are doing the same thing, how are you checking it is nsIdleService that is being called at the 1 second interval?
Ok, this is not visible on desktop. I found the difference also and it comes from line http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsIdleService.cpp#223

223   if (!polledIdleTimeIsValid && 0 == mLastIdleReset) {
224     *idleTime = 0;
225     return NS_OK;
226   }

On desktop we don't stop to this if, since we are using polling, but if we are not using polling, then we will stop to here every time as long as mLastIdleReset==0 and that is true as long there haven't been ResetIdleTimeOut call. With desktop idle time works since we will return

229   if (0 == mLastIdleReset) {
230     *idleTime = polledIdleTimeMS;
231     return NS_OK;
232   }

You can reproduce this one second timeout on desktop by modifying line 223 to look like this:

223   if (0 == mLastIdleReset) {

Then you get CheckAwayState called between one second before some user interaction is done. There must be one second idleObserver which is causing this and since it will never gets idled before user interaction(idleTime==0), it keeps waking up. Correction would be to find a suitable place to call ResetIdleTimeOut during the launching of the browser. Without that we will have different kind of behavior on platform not using polling, like Maemo and Android.
Jon, thanks for the info! This is indeed reproducible on desktop (with that patch).

Investigation shows the 1-second idle observer at fault here is this:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/BrowserView.js#213

I opened a separate bug for the proper rewriting of the idle service, bug 596044. That requires changing interfaces and lots of testing on all the relevant platforms, so likely not feasible for Firefox4/Fennec2. For Fennec2, however, we should prevent the problem somehow. Options:

1. Does that timer need to be 1 second?
2. As Jon suggested, call ResetIdleTimeOut at some point in startup. But if done in Firefox code, would potentially need a lot of testing. Or, would we want to do it just in mobile widget code somehow?
Summary: Rewrite IdleService to never run unnecessary timers for polling → Fennec wakes up every second due to IdleService
mbrubeck informs me that the timer is in a file that will be removed this week anyhow, as part of bug 576192.
(In reply to comment #17)
> The new background cycle collector really needs long sleep times to function
> properly, if we can fix this properly for the entire platform and not just
> fennec that would be awesome.

Trying to move conversation into other bugs, since this is soon closed, but I would still like to ask about this: is that new cycle collector going to have affect to bug 508518? Cycle collector is causing use-time problems there, since 5s timer is used for checking if cycle collection is needed. Is so, please give comments to that bug.
(In reply to comment #24)
> 1. Does that timer need to be 1 second?
> 2. As Jon suggested, call ResetIdleTimeOut at some point in startup. But if
> done in Firefox code, would potentially need a lot of testing. Or, would we
> want to do it just in mobile widget code somehow?

1. So that timer is going away, but the real bug was not using 1s timer and now that timer is going to be next shortest idle timer used. Real problem is that idle service is now working differently for different platforms. This is bound to cause problems. So good that we now have a bug about fixing this issue.
2. This depends mostly how this different behavior on different platforms is going to be solved. First thing give answer to this questing: Is idle time meaning user idle time counted for:
 (a) hole system
 (b) only for the browser
If it is (a), then polling is needed probably on all platforms, but there may be ways to reduce that. For example on MeeGo we can get information of when user becomes active after user was idle, so we don't need to poll in between time. It may be that platforms depending on screen saver implementation could get same kind of information. We also don't need to poll on any platform before first idle observer becomes idle. So polling would be needed only after first observer have become idle and until system goes to some kind of idle state from which returning is possible to observe.
 If it is (b), then polling is not needed on any platform and we can go forward to fix that bug 596044 by removing it from every platform. Like you said, that would require a lot of testing. We could start with small though and with this approach the first fix could be to find suitable place to call ResetIdleTimeOut on the start-up.
(In reply to comment #27)
> (In reply to comment #24)
> > 1. Does that timer need to be 1 second?
> > 2. As Jon suggested, call ResetIdleTimeOut at some point in startup. But if
> > done in Firefox code, would potentially need a lot of testing. Or, would we
> > want to do it just in mobile widget code somehow?
> 
> 1. So that timer is going away, but the real bug was not using 1s timer and now
> that timer is going to be next shortest idle timer used. Real problem is that
> idle service is now working differently for different platforms. This is bound
> to cause problems. So good that we now have a bug about fixing this issue.

Agreed.

> 2. This depends mostly how this different behavior on different platforms is
> going to be solved. First thing give answer to this questing: Is idle time
> meaning user idle time counted for:
>  (a) hole system
>  (b) only for the browser

The idle time reported in Firefox is for Firefox itself, or should be.

> If it is (a), then polling is needed probably on all platforms, but there may
> be ways to reduce that. For example on MeeGo we can get information of when
> user becomes active after user was idle, so we don't need to poll in between
> time. It may be that platforms depending on screen saver implementation could
> get same kind of information. We also don't need to poll on any platform before
> first idle observer becomes idle. So polling would be needed only after first
> observer have become idle and until system goes to some kind of idle state from
> which returning is possible to observe.
>  If it is (b), then polling is not needed on any platform and we can go forward
> to fix that bug 596044 by removing it from every platform. Like you said, that
> would require a lot of testing. We could start with small though and with this
> approach the first fix could be to find suitable place to call ResetIdleTimeOut
> on the start-up.

I am worried even changing that could break something, if anything depends on the current behavior. We'd need to test very carefully.
Hmm, why isn't ResetIdleTimeout being called? It seems to be called from all implementations that don't have polling,

http://mxr.mozilla.org/mozilla-central/ident?i=ResetIdleTimeOut&filter=

In fact it seems the proper behavior is for ResetIdleTimeout to be called from a widget location. Otherwise, without polling, there is no sense of having any idea of the current idle time.

Is the Qt call to ResetIdleTimeout not being reached?
(In reply to comment #29)
> Hmm, why isn't ResetIdleTimeout being called? It seems to be called from all
> implementations that don't have polling,
> 
It is called from nsWindow::UserActivity() like with most of the platforms. There is this bug 594662 with Qt though that should be an easy fix.

> In fact it seems the proper behavior is for ResetIdleTimeout to be called from
> a widget location. Otherwise, without polling, there is no sense of having any
> idea of the current idle time.
> 
yes that is correct behavior.

> Is the Qt call to ResetIdleTimeout not being reached?
It is reached when it is used, but the problem is with all the platform not using polling, that idle time is returned as 0 when ResetIdleTimeOut haven't yet be called.
This is getting really ugly, but how about using imaginary idle time when ResetIdleTimeOut haven't been called for platforms that are not using polling. Using this 1/1000 of current time would be big enough to idle all observers so that they wouldn't cause any wake-ups. Then as soon as user will interact with browser this wouldn't be used anymore. Of course when idle service is eventually fixed correct form of giving only idle time counted for browser interactions we would get rid of this also.
Attachment #475484 - Flags: feedback?(azakai)
Depends on: 594662
Comment on attachment 475484 [details] [diff] [review]
Uses 1/1000 of current time as idle time when ResetIdleTimeOut haven't been called.

This is just a hack, which might solve the current problem, but might cause other problems as well.

We should fix bug 594662, so that ResetIdleTimeout is actually called. I agree that it makes sense in general to have the idle time reset on startup, or using a hack like in this patch, but let's leave that change to removing polling entirely (bug 596044).

Meanwhile fixing bug 594662 should solve the problem for Fennec 2.0.
Attachment #475484 - Flags: feedback?(azakai) → feedback-
(In reply to comment #32)
> Comment on attachment 475484 [details] [diff] [review]
> This is just a hack, which might solve the current problem, but might cause
> other problems as well.
> 
Yes, it might cause, but it still would be safer than returning 0 which is just completely wrong. That is saying that user is interacting constantly. Using some "sane" value would be better. Other thing is that this would only affect platforms not using the polling.

> We should fix bug 594662, so that ResetIdleTimeout is actually called. I agree
> that it makes sense in general to have the idle time reset on startup, or using
> a hack like in this patch, but let's leave that change to removing polling
> entirely (bug 596044).
> 
> Meanwhile fixing bug 594662 should solve the problem for Fennec 2.0.
Unfortunately fixing bug 594662 wont fix this issue. That strictly Qt bug is about adding moving of the mouse to be counted as interaction also, like it is counted on other platforms also. Maybe there are some other interactions also that need to be added, but it wont correct this one. In here we need the idle service to work correctly when user has not done any interaction. This is not only Maemo issue, but should also be visible on Android, since polling is not used there.
(In reply to comment #33)
> (In reply to comment #32)
> > Comment on attachment 475484 [details] [diff] [review] [details]
> > This is just a hack, which might solve the current problem, but might cause
> > other problems as well.
> > 
> Yes, it might cause, but it still would be safer than returning 0 which is just
> completely wrong. That is saying that user is interacting constantly. Using
> some "sane" value would be better. Other thing is that this would only affect
> platforms not using the polling.
> 

If we have had a design document you would have been able to see that the idea is to not trigger an idle state while the browser is starting up - admitted it looks like we miss some code that makes us leave this state without user activity...
Jon, you are absolutely correct.

Ok, this patch makes GetIdleTime return an error, and not 0, if we have no valid idle time to report. Returning 0 was confusing the only place (CheckAwayState) that calls GetIdleTime and leading it to run a timer for checking the need for calling observers for idle or active time - but we cannot make such calls anyhow if we as yet have no valid information at all.

This causes no changes on any tier-1 platform, as it affects only cases without polling for idle time (all the tier-1 platforms have polling). It affects platforms without polling, namely Meego, Android, Windows CE. Will prevent the unnecessary timer for them.

Also some minor cleanup of the code in GetIdleTime is in the patch.
Attachment #475484 - Attachment is obsolete: true
Attachment #476035 - Flags: review?
Attachment #476035 - Flags: review? → review?(mkristoffersen)
 Yes, this is better than returning some imaginary time like in my patch. Now it wont continue like it would have received valid time 0, but makes separation to fact that we haven't started idle time.
 After this idle service is working in both cases(polling and not polling), only problem is that they are working differently. Platforms using polling will continue using system idle time and when browser is started idle time is running already.
 Platforms not using the polling will continue using browser idle time and when browser is started the idle time starts only after user interaction. Both could be valid cases, but only one way should be applied. Of course we could intentionally let mobile platforms have different solution than in desktop platforms, but that would then need to be well documented and most probably it still would lead to problems. Fixing bug 596044 on the other hand would solve this problem.
  We could maybe prevent using idle time already running in start-up by changing

+  if (!polledIdleTimeIsValid && 0 == mLastIdleReset)
+    return NS_ERROR_FAILURE;

to

+  if (0 == mLastIdleReset)
+    return NS_ERROR_FAILURE;

but this would also need to be tested well and in the end of the day we still would need to remove polling if system idle time is not wanted.
As the purpose of this is to prevent heavy processing while the user is handling the browser, would it be an idea to return "not" idle until the browser has finished launching - I'm not sure we have a hook somewhere, but we might be able to define one.

With this information we could change the service so the first idle time reported/used was a delta from this "finished loading" time no matter if we use poling or not, and before this report no idle time, or 1 ms or whatever is reasonable...
(In reply to comment #37)
> As the purpose of this is to prevent heavy processing while the user is
> handling the browser, would it be an idea to return "not" idle until the
> browser has finished launching - I'm not sure we have a hook somewhere, but we
> might be able to define one.
> 
I like the idea, if finished launching is meaning that browser is visible and ready for user input. If it is just started to background, I guess we can't count idle time from that point.

> With this information we could change the service so the first idle time
> reported/used was a delta from this "finished loading" time no matter if we use
> poling or not, and before this report no idle time, or 1 ms or whatever is
> reasonable...
This will work if we use "no idle time"/error/whatever telling that idle time has not started yet. I guess we can't return 1ms or any other number either since that would give a wrong message.
(In reply to comment #38)
> (In reply to comment #37)
> > As the purpose of this is to prevent heavy processing while the user is
> > handling the browser, would it be an idea to return "not" idle until the
> > browser has finished launching - I'm not sure we have a hook somewhere, but we
> > might be able to define one.
> > 
> I like the idea, if finished launching is meaning that browser is visible and
> ready for user input. If it is just started to background, I guess we can't
> count idle time from that point.

Yeah, I was thinking of "browser is visible and ready for user input" - or rather would be visible and ready for user input if it was the foreground application that the OS would send user input to. But not like during the time where it is checking/updating plugins... or whatever keeps the browser from launching to my desktop sometimes...

> 
> > With this information we could change the service so the first idle time
> > reported/used was a delta from this "finished loading" time no matter if we use
> > poling or not, and before this report no idle time, or 1 ms or whatever is
> > reasonable...
> This will work if we use "no idle time"/error/whatever telling that idle time
> has not started yet. I guess we can't return 1ms or any other number either
> since that would give a wrong message.

Maybe we need to change the interpretation of "starting up" inside the service, but why is returning 0 as "we have had no idle time" a problem in it self?
(In reply to comment #39)
> but why is returning 0 as "we have had no idle time" a problem in it self?
For the reasons how it is causing problems right now. In other words 0 is "we just now had interaction" which will cause idle timer with shortest interval to wake up again and again until idle time it is waiting will be passed. If we return error instead and return straight away from nsIdleService::CheckAwayState function, we wont set up another wake-up before nsIdleService::CheckAwayState is called after that start-up is done. This way idle observers wont cause unnecessary wake-ups during the start-up or when started to background.
(In reply to comment #39)
> Maybe we need to change the interpretation of "starting up" inside the service,
> but why is returning 0 as "we have had no idle time" a problem in it self?

The symptom is exactly as Jon said.

The logical problem, as I see it, is this:

* CheckAwayState checks for which observers it needs to notify, about away or active-again.
* Without _any_ information about the idle state - activity or non-activity - it makes no sense to send either kind of notification. And indeed neither is actually sent.
* _But_, since 'no information' is not handled properly, we still run a timer at a constant rate, needlessly. The patch fixes that. A timer will be run only when we have valid data, which is necessary for sending any kind of notification.
Unless you can convince me that I'm wrong in the following, I'm not going to accept the patch:

I don't think that the software, with the patch, can enter idle mode, if there are no new subscribers after the first user interaction.

The reason I claim this is:

1) The timer used to detect idlemode is only started at end of nsIdleService::CheckAwayState

2) This code can't be executed if CheckAwayState is called with an in parameter that is "true".

3) There are only two places that calls this with a "false" in parameter - One is AddIdleObserver, the other is the timer callback.  AddIdleObserver will only be called if a new observer is added, and until that happens the timer is never going to be started, and hence, can't expire.
I think we should look into a solution that will actually handle the case where GetIdleTime returns 0, as this is a valid use case (if less than 1 s has elapsed since the last user interaction (imagine you are moving the mouse around all the time).

It sounds like our problem is that we let some observers enter idle mode, even we aren't in idle mode - I think that is what we should fix.

Having GetIdleTime return an error as a "normal" return value requires that all callers must be able to handle an extra code path (you could claim that they would have to do that anyway, but I don't think that argument holds in Mozilla code, where error checking seems to be avoided if possible).
Attached patch Patch v2 (obsolete) — — Splinter Review 
(In reply to comment #42)
> I don't think that the software, with the patch, can enter idle mode, if there
> are no new subscribers after the first user interaction.

Heh, good point :) Ok, I removed the in parameter in this patch, which was just a minor optimization anyhow. This should fix that issue.

(In reply to comment #43)
> I think we should look into a solution that will actually handle the case where
> GetIdleTime returns 0, as this is a valid use case (if less than 1 s has
> elapsed since the last user interaction (imagine you are moving the mouse
> around all the time).

The case of 0 idle time - no idle time has passed - is very different from the error the patch returns.

0 idle time is *already* handled properly by the code. All the comparisons to the idle time work with 0 just as well as with other values.

The bug was that 0 was returned when it shouldn't, in the case of an actual failure - the case where there is a complete inability to say what the idle time was. The code used to say 0 in that case, which is misleading (and misled the code into running an unneeded timer). Instead, the patch makes that failure case return an actual failure code.

There is no additional place in the codebase that calls this, so no immediate need to add any new error-handling code. But anyhow, such places *would* need to be fixed, since 0 idle time != no idea what the idle time is. In other words, those theoretical places could suffer from similar bugs to the one just fixed here.
Attachment #476035 - Attachment is obsolete: true
Attachment #476035 - Flags: review?(mkristoffersen)
(In reply to comment #44)
> Created attachment 476312 [details] [diff] [review]
> Patch v2
> 
> (In reply to comment #42)
> > I don't think that the software, with the patch, can enter idle mode, if there
> > are no new subscribers after the first user interaction.
> 
> Heh, good point :) Ok, I removed the in parameter in this patch, which was just
> a minor optimization anyhow. This should fix that issue.

You do realize that without that "minor optimization" we will restart the timer for each user event, right?
Yes, I thought it wouldn't be very significant though.

How many user events do we expect to get per second?
(In reply to comment #44)
> The bug was that 0 was returned when it shouldn't, in the case of an actual
> failure - the case where there is a complete inability to say what the idle
> time was. The code used to say 0 in that case, which is misleading (and misled
> the code into running an unneeded timer). Instead, the patch makes that failure
> case return an actual failure code.

I thought the first bug was that we let observers enter idle mode without knowing that we were in idle mode?  - it is true that we couldn't/can't handle this, but that is a side effect of being in a wrong state.

If you want to prevent CheckAwayState from doing anything before we know our state, then I think it would be much clearer if we did an explicit check for it as one of the first things in CheckAwayState (like "if (!mLastIdleReset) return ...").  Alternatively not call CheckAwayState from the AddIdleObserver if we are not initialized.  

(Actually it would probably be best to have a boolean member that kept the state of the service having a valid idle time or not - as I no longer think using a magic value in mLastIdleReset is a clever idea, if we change the definition of our initial state from "not being idle" to "unkown").

Sorry you and Jon are being dragged into this code...
(In reply to comment #46)
> Yes, I thought it wouldn't be very significant though.
> 
> How many user events do we expect to get per second?

Well, depends on how fast you can type or click your mouse (press+release) - but the worst case scenario is probably the mouse move events - I would expect that could go into the hundreds per second, like if you are playing a browser game?
(In reply to comment #48)
> (In reply to comment #46)
> > Yes, I thought it wouldn't be very significant though.
> > 
> > How many user events do we expect to get per second?
> 
> Well, depends on how fast you can type or click your mouse (press+release) -
> but the worst case scenario is probably the mouse move events - I would expect
> that could go into the hundreds per second, like if you are playing a browser
> game?

Oh well, the limiting factor is probably the speed of the processor, not the rate the mouse can sample the position change...
(In reply to comment #47)
> I thought the first bug was that we let observers enter idle mode without
> knowing that we were in idle mode?

I am only aware of the one bug I mentioned before, of returning 0 when there is no valid data at all in GetIdleTime.

> 
> If you want to prevent CheckAwayState from doing anything before we know our
> state, then I think it would be much clearer if we did an explicit check for it
> as one of the first things in CheckAwayState (like "if (!mLastIdleReset) return
> ...").

Sounds good, it can be made more explicit that way.

> (Actually it would probably be best to have a boolean member that kept the
> state of the service having a valid idle time or not - as I no longer think
> using a magic value in mLastIdleReset is a clever idea, if we change the
> definition of our initial state from "not being idle" to "unkown").

I don't understand what the change is. The current source says,

  // A zero in mLastIdleReset indicates that this function has never been
  // called.

so mLastIdleReset was always used that way.

The patch does not change mLastIdleReset. It changes the return value of GetIdleTime, stopping it from returning 0 when 0 is logically incorrect.

(In reply to comment #48)
> (In reply to comment #46)
> > Yes, I thought it wouldn't be very significant though.
> > 
> > How many user events do we expect to get per second?
> 
> Well, depends on how fast you can type or click your mouse (press+release) -
> but the worst case scenario is probably the mouse move events - I would expect
> that could go into the hundreds per second, like if you are playing a browser
> game?

I thought OSes throttle this stuff. But anyhow we can throttle it ourselves.

I am trying to rewrite the code for that, but I don't understand

http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsIdleService.cpp#401

- 1 means the timer should fire in 1 second, 2 in 2 seconds, 3 in 3 seconds, but 0 means - never fire...? :( Is this an additional bug?
(In reply to comment #50)
> (In reply to comment #47)
> > I thought the first bug was that we let observers enter idle mode without
> > knowing that we were in idle mode?
> 
> I am only aware of the one bug I mentioned before, of returning 0 when there is
> no valid data at all in GetIdleTime.

Yeah, I misunderstood one of the previous comments, which I read as we woke up the idle observers, where it was the idle service we woke up.  If we define the startup sequence, as one where we won't be in idle, I don't see it as a bug to return 0 - it means "We are not in idle" we know that per definition during startup.  And I think it is a complication to the interface to add a "we have no valid data" state. - I'm not saying I won't accept it - I'm saying, I think it is a bad idea :)

 
> > (Actually it would probably be best to have a boolean member that kept the
> > state of the service having a valid idle time or not - as I no longer think
> > using a magic value in mLastIdleReset is a clever idea, if we change the
> > definition of our initial state from "not being idle" to "unkown").
> 
> I don't understand what the change is. The current source says,
> 
>   // A zero in mLastIdleReset indicates that this function has never been
>   // called.
> 
> so mLastIdleReset was always used that way.

Yeah, it was used that way - but you are about to change the behavior from "not idle" to "undefined" in the initial state - but anyway I think a boolean is a better idea (again, because it is more explicit, and not a magic value, you can see in the code that we even fake the idle time to 1, even if it is actually 0)

> (In reply to comment #48)
> > (In reply to comment #46)
> > > Yes, I thought it wouldn't be very significant though.
> > > 
> > > How many user events do we expect to get per second?
> > 
> > Well, depends on how fast you can type or click your mouse (press+release) -
> > but the worst case scenario is probably the mouse move events - I would expect
> > that could go into the hundreds per second, like if you are playing a browser
> > game?
> 
> I thought OSes throttle this stuff. But anyhow we can throttle it ourselves.

Yes, I think it is normal for the OS to give the mouse move events low priority, compared to other events send to the application - so I suppose that means that we get as many as we can handle.

> 
> I am trying to rewrite the code for that, but I don't understand
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsIdleService.cpp#401
> 
> - 1 means the timer should fire in 1 second, 2 in 2 seconds, 3 in 3 seconds,
> but 0 means - never fire...? :( Is this an additional bug?

I don't think it is a bug, I think it would be a bug to request a timer that would expire immediately.  - I didn't check, but I don't hope the code will ever try to request that during normal operation?
I was thinking a bit about this bug and the idle service during the weekend.  Would it make sense to start something like timer with a fixed length during startup (like 1 or 5 minutes) - if we haven't had any user activity at the time this timer expires, then we could start the idle timers at this point.

The reason for doing this is to ensure that the idle observers actually gets called, even there are no user interactions.
> > 
> > I am trying to rewrite the code for that, but I don't understand
> > 
> > http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsIdleService.cpp#401
> > 
> > - 1 means the timer should fire in 1 second, 2 in 2 seconds, 3 in 3 seconds,
> > but 0 means - never fire...? :( Is this an additional bug?
> 
> I don't think it is a bug, I think it would be a bug to request a timer that
> would expire immediately.  - I didn't check, but I don't hope the code will
> ever try to request that during normal operation?

You are right, a 0 can never appear in that function in fact. So the check for aDelay != 0 is unnecessary actually.

(In reply to comment #52)
> I was thinking a bit about this bug and the idle service during the weekend. 
> Would it make sense to start something like timer with a fixed length during
> startup (like 1 or 5 minutes) - if we haven't had any user activity at the time
> this timer expires, then we could start the idle timers at this point.
> 
> The reason for doing this is to ensure that the idle observers actually gets
> called, even there are no user interactions.

This sounds similar to Jon's suggestion before, to add a call to ResetIdleTimeout during startup, except to add that call a little later. I'm not sure what the benefit is of doing it after a minute or 5 as opposed to immediately. The downside is that we will have timers firing needlessly during that minute or 5 minutes.

And the other downside remains, that this changes behavior and might break things. But again, we might do this just for non-polling OSes, leaving our tier-1 platforms unchanged. If you are ok with that, then let's do it that way (but I would support not waiting, or waiting for a very short time, to minimize unneeded timers).
(In reply to comment #53)
> > I don't think it is a bug, I think it would be a bug to request a timer that
> > would expire immediately.  - I didn't check, but I don't hope the code will
> > ever try to request that during normal operation?
> 
> You are right, a 0 can never appear in that function in fact. So the check for
> aDelay != 0 is unnecessary actually.

Maybe it should be made into an assert instead...

> 
> (In reply to comment #52)
> > I was thinking a bit about this bug and the idle service during the weekend. 
> > Would it make sense to start something like timer with a fixed length during
> > startup (like 1 or 5 minutes) - if we haven't had any user activity at the time
> > this timer expires, then we could start the idle timers at this point.
> > 
> > The reason for doing this is to ensure that the idle observers actually gets
> > called, even there are no user interactions.
> 
> This sounds similar to Jon's suggestion before, to add a call to
> ResetIdleTimeout during startup, except to add that call a little later. I'm
> not sure what the benefit is of doing it after a minute or 5 as opposed to
> immediately. The downside is that we will have timers firing needlessly during
> that minute or 5 minutes.

It sounds like I didn't get explain what I meant clearly enough, if you think it will make a timers fire needlessly - I'm talking about a new single timer, that fires after some time - and defines the beginning of idle time, unless it has already been started by user interaction.

> 
> And the other downside remains, that this changes behavior and might break
> things. But again, we might do this just for non-polling OSes, leaving our
> tier-1 platforms unchanged. If you are ok with that, then let's do it that way
> (but I would support not waiting, or waiting for a very short time, to minimize
> unneeded timers).

As the one second timer issue goes away in another bug, we might postpone this so it doesn't change the behavior of the SW until after the next release (FF/Fennec).

Btw did we come to a conclusion on whether or not we should stop using the pool mode, and only count idle time as browser idle time??? - I kept an eye on the newsgroups but didn't see any discussions on it (but then again, I don't read all newsgroups).
> > 
> > (In reply to comment #52)
> > > I was thinking a bit about this bug and the idle service during the weekend. 
> > > Would it make sense to start something like timer with a fixed length during
> > > startup (like 1 or 5 minutes) - if we haven't had any user activity at the time
> > > this timer expires, then we could start the idle timers at this point.
> > > 
> > > The reason for doing this is to ensure that the idle observers actually gets
> > > called, even there are no user interactions.
> > 
> > This sounds similar to Jon's suggestion before, to add a call to
> > ResetIdleTimeout during startup, except to add that call a little later. I'm
> > not sure what the benefit is of doing it after a minute or 5 as opposed to
> > immediately. The downside is that we will have timers firing needlessly during
> > that minute or 5 minutes.
> 
> It sounds like I didn't get explain what I meant clearly enough, if you think
> it will make a timers fire needlessly - I'm talking about a new single timer,
> that fires after some time - and defines the beginning of idle time, unless it
> has already been started by user interaction.

Until that new timer fires, we will have the old behavior, though, no? Which includes firing unnecessary timers. They will stop only when this new timer fires, and resets the idle time. Or do I still not understand? (See comments below, maybe this is connected to the other issue?)

> 
> > 
> > And the other downside remains, that this changes behavior and might break
> > things. But again, we might do this just for non-polling OSes, leaving our
> > tier-1 platforms unchanged. If you are ok with that, then let's do it that way
> > (but I would support not waiting, or waiting for a very short time, to minimize
> > unneeded timers).
> 
> As the one second timer issue goes away in another bug, we might postpone this
> so it doesn't change the behavior of the SW until after the next release
> (FF/Fennec).

The underlying issue will remain. The 'fastest' idle timer will define the delay (see discussion in previous comments). Currently this is a 1-second timer. After that timer goes away, it will probably be a 5-second timer (if I recall correctly that is the next fastest one). We do need to fix this issue.

> 
> Btw did we come to a conclusion on whether or not we should stop using the pool
> mode, and only count idle time as browser idle time??? - I kept an eye on the
> newsgroups but didn't see any discussions on it (but then again, I don't read
> all newsgroups).

There is a separate bug, bug 596044, for thinking about avoiding polling mode entirely, for platforms that support doing so.
(In reply to comment #55)
> Until that new timer fires, we will have the old behavior, though, no? Which
> includes firing unnecessary timers. They will stop only when this new timer
> fires, and resets the idle time. Or do I still not understand? (See comments
> below, maybe this is connected to the other issue?)

Ahh... no, I want the patch you are making for this - the remaining problem as I recall it, was that the idle time wouldn't start until the first user interaction - it is this problem that I suggest we solve with a timer.

> > As the one second timer issue goes away in another bug, we might postpone this
> > so it doesn't change the behavior of the SW until after the next release
> > (FF/Fennec).
> 
> The underlying issue will remain. The 'fastest' idle timer will define the
> delay (see discussion in previous comments). Currently this is a 1-second
> timer. After that timer goes away, it will probably be a 5-second timer (if I
> recall correctly that is the next fastest one). We do need to fix this issue.

but that will never be solved will it? - if some part of the software requests to be woken 5 second after the last user activity, then we need a 5 second timer?  - If we are going to change that, then it is who ever request the 5 second timer that should be changed.

I thought this bug had evolved to only deal with the case before the first user interaction as the algorithm was sub-optimal?

> 
> > 
> > Btw did we come to a conclusion on whether or not we should stop using the pool
> > mode, and only count idle time as browser idle time??? - I kept an eye on the
> > newsgroups but didn't see any discussions on it (but then again, I don't read
> > all newsgroups).
> 
> There is a separate bug, bug 596044, for thinking about avoiding polling mode
> entirely, for platforms that support doing so.

I'll stop discussing it in this bug then :)
> 
> I thought this bug had evolved to only deal with the case before the first user
> interaction as the algorithm was sub-optimal?
> 

Ok, I didn't understand before. Yes, that is what this bug should fix.

The remaining issue could be fixed with a timer as you suggested. It seems like a hack though, in order to cover up a bug. If we are ok with that, why not just do what was suggested before, to call ResetIdleTimeout() during initialization (for platforms without polling)? I think that is the simplest and safest solution at this point.
(In reply to comment #57)
> > 
> > I thought this bug had evolved to only deal with the case before the first user
> > interaction as the algorithm was sub-optimal?
> > 
> 
> Ok, I didn't understand before. Yes, that is what this bug should fix.
> 
> The remaining issue could be fixed with a timer as you suggested. It seems like
> a hack though, in order to cover up a bug. If we are ok with that, why not just
> do what was suggested before, to call ResetIdleTimeout() during initialization
> (for platforms without polling)? I think that is the simplest and safest
> solution at this point.

It might be considered a bug - but until we have someone sending a global event to indicate when we are done with loading/initialization of the browser we might have to be a bit creative in defining when idle time starts :)

I'm not sure what you mean by calling ResetIdleTimeout during initialization and what it would help? - I don't think it will do what we want until GetIdleTime returns a sane value...
> I'm not sure what you mean by calling ResetIdleTimeout during initialization
> and what it would help? - I don't think it will do what we want until
> GetIdleTime returns a sane value...

For example, the entire patch could be to set a timer for 5 seconds in the idle service, and at that time call ResetIdleTimeout. Sort of like the other idea from before (but without the other changes).

Calling ResetIdleTimeout will ensure a valid value is returned from GetIdleTime - it will no longer think we are constantly active.
(In reply to comment #59)
> > I'm not sure what you mean by calling ResetIdleTimeout during initialization
> > and what it would help? - I don't think it will do what we want until
> > GetIdleTime returns a sane value...
> 
> For example, the entire patch could be to set a timer for 5 seconds in the idle
> service, and at that time call ResetIdleTimeout. Sort of like the other idea
> from before (but without the other changes).
> 
> Calling ResetIdleTimeout will ensure a valid value is returned from GetIdleTime
> - it will no longer think we are constantly active.

What is the point of waiting 5 seconds then, couldn't we just do:

mLastIdleReset = PR_IntervalToSeconds(PR_IntervalNow()); 

(or call ResetIdleTimeout) in the initialization function? and forget about being not-idle or anything during startup?
(In reply to comment #60)
> (In reply to comment #59)
> > > I'm not sure what you mean by calling ResetIdleTimeout during initialization
> > > and what it would help? - I don't think it will do what we want until
> > > GetIdleTime returns a sane value...
> > 
> > For example, the entire patch could be to set a timer for 5 seconds in the idle
> > service, and at that time call ResetIdleTimeout. Sort of like the other idea
> > from before (but without the other changes).
> > 
> > Calling ResetIdleTimeout will ensure a valid value is returned from GetIdleTime
> > - it will no longer think we are constantly active.
> 
> What is the point of waiting 5 seconds then, couldn't we just do:
> 
> mLastIdleReset = PR_IntervalToSeconds(PR_IntervalNow()); 
> 
> (or call ResetIdleTimeout) in the initialization function? and forget about
> being not-idle or anything during startup?

There are some complications here.

We do not want to change anything if polling is available, since (1) there is no bug in polling, and (2) our tier-1 platforms all use polling, and we don't want to change their behavior at all.

So we can just check for polling during startup, and set mLastIdleReset as you said. However, polling may not succeed immediately, the various implementations may only return true after some valid idle time happens (see for example the windows one). So checking later would be the safer way to go. But even this is not perfect in theory. Any ideas?
(In reply to comment #61)
> There are some complications here.
> 
> We do not want to change anything if polling is available, since (1) there is
> no bug in polling, and (2) our tier-1 platforms all use polling, and we don't
> want to change their behavior at all.
> 
> So we can just check for polling during startup, and set mLastIdleReset as you
> said. However, polling may not succeed immediately, the various implementations
> may only return true after some valid idle time happens (see for example the
> windows one). So checking later would be the safer way to go. But even this is
> not perfect in theory. Any ideas?

If a platform don't support pooling initially, then I'm afraid it will change behavior if we are going to fix this bug, as we can't know if pooling will become available later.  It must also mean that the platform initially has the problem this bug deals with?

I don't suppose we want to introduce a new state "pooling not ready" at this time as that would lead to a bigger change?
Attached patch Patch v3 (obsolete) — — Splinter Review 
Ok, this is back to the previous idea, but also resolves the problem found in it. The first time ResetIdleTimeout is called, it will *not* do the optimization of not recalculating the timer - so it can start the timer properly - but afterwards, the optimization will be used - so we do not become slower.
Attachment #476312 - Attachment is obsolete: true
Attachment #477768 - Flags: review?(mkristoffersen)
Comment on attachment 477768 [details] [diff] [review]
Patch v3

It's fine, assuming it doesn't break any of the stuff on the try server and you change the PRBool in nsIdleService::ResetIdleTimeOut to "bool"

I don't know if this needs an SR, as it technically contains an interface change?

You can change the following if you like, or leave it the way you wrote it:

calledBefore = mLastIdleReset != 0;

It could be made more explicit, either:

calledBefore = (0 != mLastIdleReset);  

(Getting used to putting the constant to the left, when comparing, ensures that you get a compile error the day you miss a keystroke and type a = 0, instead of a == 0, or a != 0 - the parentheses prevents people from getting confused when we have assignments and boolean expressions in the same line )

or shorter:

calledBefore = !mLastIdleReset;
Attachment #477768 - Flags: review?(mkristoffersen) → review+
(In reply to comment #64)
> calledBefore = !mLastIdleReset;

Hrmm... what I think what I meant to say was:

calledBefore = mLastIdleReset;
(In reply to comment #65)
> (In reply to comment #64)
> > calledBefore = !mLastIdleReset;
> 
> Hrmm... what I think what I meant to say was:
> 
> calledBefore = mLastIdleReset;

Which will probably give a warning on the type conversion, there are ways around that, but maybe better to do the explicit comparison against zero as you did originally.
(In reply to comment #64)
> I don't know if this needs an SR, as it technically contains an interface
> change?

In my opinion this fixes a bug in the implementation of an existing interface (and adds a comment to the idl to clarify that). But it could also be seen as an interface change I guess. dougt, I think you have been following this bug, what do you think?
I'd say since you're not changing the interface, you don't *need* an SR. It wouldn't hurt though.
(In reply to comment #68)
> I'd say since you're not changing the interface, you don't *need* an SR. It
> wouldn't hurt though.

You don't need to change the prototype of a function in the IDL in order to change an interface - we change the behavior which is as much part of the interface as the actual parameters or return types.
Comment on attachment 477768 [details] [diff] [review]
Patch v3

Looks like we can't decide if sr is needed, so let's ask the module owner ;) roc, would you like to sr this? It's a bugfix, but arguably changes an interface.

The change is documented in the .idl in a comment. Basically, getting idleTime from the idle service used to return 0 if there was no data (no calls were made to reset the idle time, and the particular platform cannot manually poll for idle time). This patch fixes that bug, it will now give an error.
Attachment #477768 - Flags: superreview?(roc)
Comment on attachment 477768 [details] [diff] [review]
Patch v3

Have you checked callers to make sure they can handle this?
Attachment #477768 - Flags: superreview?(roc) → superreview+
(In reply to comment #71)
> Comment on attachment 477768 [details] [diff] [review]
> Patch v3
> 
> Have you checked callers to make sure they can handle this?

Yes, there is only one caller of that function, in the idle service in fact. (Actually the original motivation for this patch was that that function got confused due to 0 meaning either 'no idle time' or 'no idea what the idle time is', which led to bad stuff happening.)

Thanks for the speedy response!
tracking-fennec: 2.0+ → 2.0b2+
(In reply to comment #72)
> Yes, there is only one caller of that function, in the idle service in fact.
> (Actually the original motivation for this patch was that that function got
> confused due to 0 meaning either 'no idle time' or 'no idea what the idle time
> is', which led to bad stuff happening.)
This isn't actually true.  There are multiple JS callers in our tree that also use it:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesDBUtils.jsm#136
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsLivemarkService.js#288
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2972
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3027
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#236

Most of these don't actually handle the change where it might throw now.

Additionally, this is an API change that would be happening after feature freeze (beta 7) that could break add-ons.  We are supposed to be in a stable API post-beta7, and this would break that.
Some of those checks are actually useless since the observer is called when the time has elapsed.
nsBrowserGlue does a useless check, livemarkservice is checking exceptions, placesDBUtils is going to change in bug 570387. updateService needs to be updated.
But I agree this is calling for an SR.
(In reply to comment #74)
> But I agree this is calling for an SR.
Well, this got sr+, but it doesn't look like it's going to land for beta 7 either, which means we can't take it for 4.0
Attached patch Patch v4 (obsolete) — — Splinter Review 
Ok, let's get around those problems by not changing the interface at all. At the cost of slightly ugly code.
Attachment #477768 - Attachment is obsolete: true
Attachment #481272 - Flags: review?
Attachment #481272 - Flags: review? → review?(mkristoffersen)
(In reply to comment #76)
> Created attachment 481272 [details] [diff] [review]
> Patch v4
> 
> Ok, let's get around those problems by not changing the interface at all. At
> the cost of slightly ugly code.

Style issue (Convention dictates, bla. bla. bla....):
------------
Change PRBool to "bool" in nsIdleService.h line 219 and nsIdleService.cpp line 201

Minor issue (the patch will work fine without this optional change):
------------
mPooledIdleTimeIsValid, isn't initialized to a default value, while it is set to a valid value in GetIdleTime that is called before its first use, I would recommend to initialize it anyway.

---

With the style issue fixed, the patch is fine.
Attachment #481272 - Flags: review?(mkristoffersen) → review+
Attached patch Patch v5 — — Splinter Review 
Fixed those issues.

MikeK, I changed the indentation in the constructor when adding the new initialization. I've seen two forms in various places in the code, so not sure which is better. Is the one in the patch ok?
Attachment #481272 - Attachment is obsolete: true
Attachment #481718 - Flags: review?(mkristoffersen)
Comment on attachment 481718 [details] [diff] [review]
Patch v5

(In reply to comment #78)
> Created attachment 481718 [details] [diff] [review]
> Patch v5
> 
> Fixed those issues.
> 
> MikeK, I changed the indentation in the constructor when adding the new
> initialization. I've seen two forms in various places in the code, so not sure
> which is better. Is the one in the patch ok?

The indentation you changed is fine with me.

One last thing - it is very nice that you have changed the PR_Bool, to bool, and that you initialize mPolledIdleTimeIsValid - but I think we should initialize it to "false" not "PR_FALSE" so it fits the bool.

With that one change it's approved, I don't need to see the updated patch - good work!
Attachment #481718 - Flags: review?(mkristoffersen) → review+
http://hg.mozilla.org/mozilla-central/rev/e1042569cee4
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: