Screen turns idle and then sleep after changing the Date or Time

RESOLVED FIXED in Firefox 18

Status

Firefox OS
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: shelly, Assigned: shelly)

Tracking

unspecified
B2G C2 (20nov-10dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

(Whiteboard: [LOE:S])

Attachments

(2 attachments, 10 obsolete attachments)

12.20 KB, patch
shelly
: review+
Details | Diff | Splinter Review
2.92 KB, patch
shelly
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Observe the screen darkens and then goes to sleep right after changing the Date or Time from Settings.
(Assignee)

Updated

6 years ago
Assignee: nobody → slin
I guess the IdleAPI could listen to time change events?
(Assignee)

Updated

6 years ago
Whiteboard: [LOE:S]
(Assignee)

Comment 2

6 years ago
Created attachment 672749 [details] [diff] [review]
V1: fix of IdleService

I've added a SystemTimeChangeObserver to the gonk's IdleService, and make it reset the idle timeout after getting a notification of time change from system.

Hi Mounir, could you review the patch? Thanks.
Attachment #672749 - Flags: review?(mounir)
Comment on attachment 672749 [details] [diff] [review]
V1: fix of IdleService

I think Justin knows that code.
Attachment #672749 - Flags: review?(mounir) → review?(justin.lebar+bug)
Comment on attachment 672749 [details] [diff] [review]
V1: fix of IdleService

The correct way to fix this, which should make the idle service correct on all platforms instead of just on B2G, is to switch the idle service to using PRIntervalTime instead of whatever it uses now.

How hard would that be to do, Shelly?
Comment on attachment 672749 [details] [diff] [review]
V1: fix of IdleService

Please re-flag for review if you think we should go with this fix instead of the cross-platform one.
Attachment #672749 - Flags: review?(justin.lebar+bug)
Nice find :).
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Priority: -- → P2
(Assignee)

Comment 7

6 years ago
Comment on attachment 672749 [details] [diff] [review]
V1: fix of IdleService

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

Hi Justin, the problem of this issue is the idleService resets its LastUserInteraction timer when users tap on the ok button to change date/time, but at that moment, the system time hasn't changed yet, and when the system time really get changed, the idleService doesn't reset its timer of LastUserInteraction, thus result in a wrong elapse time.

Please correct me if I'm wrong, after surveying the PR_IntervalNow() function, I find it using the time of day on unix platform too, please see the following for more details,

http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prinrval.c#36
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/private/primpl.h#1916
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/md/_linux.h#634
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/unix.c#3020
And this is the final trace:
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/md/_unixos.h#484

So if I use the PR_IntervalNow() to calculate the elapse time, the value is still base on the system time.
Attachment #672749 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 8

6 years ago
Created attachment 673140 [details] [diff] [review]
V2: IdleService fix for gonk platform

This version fix the compiling error.
Attachment #672749 - Attachment is obsolete: true
Attachment #672749 - Flags: review?(justin.lebar+bug)
Attachment #673140 - Flags: review?(justin.lebar+bug)

Comment 9

6 years ago
> Please correct me if I'm wrong, after surveying the PR_IntervalNow() function, I 
> find it using the time of day on unix platform too,

Wow, that is distressingly bad!  That's clearly not what it's supposed to do.  Thanks for checking!

I looked at the implementation of TimeStamp, and it appears to be correct.  (See xpcom/ds/TimeStamp_posix.cpp.)  Could we use that?
^ Sorry, that was really me (I was signed into the wrong account.)
> Wow, that is distressingly bad!

I filed bug 803846 on investigating whether we need to do something about PR_IntervalNow in general.
Comment on attachment 673140 [details] [diff] [review]
V2: IdleService fix for gonk platform

I'd still prefer to fix this properly, if we can.

One problem that this patch would cause is: Suppose we change the system time often (say by a small amount each time).  That's possible -- it just depends on how frequently the cell tower sends us a new time, right?  If we get frequent updates to the system time, this code will make it so that we go idle much less quickly than we should!
Attachment #673140 - Flags: review?(justin.lebar+bug)
We should be using TimeStamp in gecko.
(Assignee)

Comment 14

6 years ago
Created attachment 674093 [details] [diff] [review]
V3: IdleService fix using TimeStamp

Hi Justin, could you give me some feedback? This version of fix replaces the PRTime and uses TimeStamp and TimeDuration instead. Thanks.
Attachment #673140 - Attachment is obsolete: true
Attachment #674093 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 674093 [details] [diff] [review]
V3: IdleService fix using TimeStamp

This is absolutely the right idea, yes.  Some comments below:

>diff --git a/widget/xpwidgets/nsIdleService.h b/widget/xpwidgets/nsIdleService.h
>+using namespace mozilla;

Please don't put |using namespace| in a header file.  Doing so causes all
headers included after this one and all files which include this header to have
that namespace in scope, which defeats the purpose of namespaces.

>@@ -150,26 +153,25 @@ protected:
>   /**
>    * Ensure that the timer is expiring at least at the given time
>    *
>    * The function might not restart the timer if there is one running currently
>    *
>-   * @param aNextTimeoutInPR
>+   * @param aNextTimeoutInTS
>    *        The last absolute time the timer should expire
>    */
>-  void SetTimerExpiryIfBefore(PRTime aNextTimeoutInPR);
>+  void SetTimerExpiryIfBefore(TimeStamp aNextTimeoutInTS);

Here and elsewhere, we don't need the "InTS" suffix.  The "InPR" and "InS"
suffixes are there to add units to a numeric type.  But the TimeStamp class
already has units.

>   /**
>    * Stores the next timeout time, 0 means timer not running
>    */
>-  PRTime mCurrentlySetToTimeoutAtInPR;
>-
>+  TimeStamp mCurrentlySetToTimeoutAtInTS;

Please add the blank line back here.

>@@ -185,27 +187,26 @@ private:
>    * Boolean indicating if any observers are in idle mode
>    */
>   bool mAnyObserverIdle;
> 
>   /**
>    * Delta time from last non idle time to when the next observer should switch
>    * to idle mode
>    *
>-   * Time in seconds
>+   * Time in TimeDuration

I don't think this line is necessary anymore, since TimeDuration has units.

>diff --git a/widget/xpwidgets/nsIdleService.cpp b/widget/xpwidgets/nsIdleService.cpp
>@@ -376,20 +376,20 @@ nsIdleService* gIdleService;
> 
> already_AddRefed<nsIdleService>
> nsIdleService::GetInstance()
> {
>   nsRefPtr<nsIdleService> instance(gIdleService);
>   return instance.forget();
> }
> 
>-nsIdleService::nsIdleService() : mCurrentlySetToTimeoutAtInPR(0),
>+nsIdleService::nsIdleService() : mCurrentlySetToTimeoutAtInTS(TimeStamp()),
>                                  mAnyObserverIdle(false),
>-                                 mDeltaToNextIdleSwitchInS(UINT32_MAX),
>-                                 mLastUserInteractionInPR(PR_Now())
>+                                 mDeltaToNextIdleSwitchInTD(TimeDuration::FromSeconds((double)UINT32_MAX)),
>+                                 mLastUserInteractionInTS(TimeStamp::Now())

The comment on mDeltaToNextIdleSwitchInTD says that it's 0 if there are no
observers, but that appears to be incorrect.  In any case, UINT32_MAX is not
the greatest possible TimeDuration; it's a hack to use that to mean "infinity".

The obvious thing to do would be to add TimeDuration::Infinity().  But that
would require adding branches to many of the TimeDuration methods, and I don't
know if that's acceptable.

We should ask roc to find out.  If it's not, we can keep track of the infinity
state ourselves in a separate variable.

I didn't read carefully past this point in the patch.  It looks basically
right, but I'd like us to resolve this UINT32_MAX issue, and also to get rid of
the other casts to uint32_t, unless they're absolutely necessary.
Attachment #674093 - Flags: feedback?(justin.lebar+bug) → feedback+
roc, how would you feel about adding TimeDuration::Infinity() with arithmetic semantics like the floating-point infinity?  This seems sane to me because TimeStamp presents itself basically as a double.  But it would cost some branches and possibly an extra member within the TimeDuration class.
What would the semantics be? Would you have a TimeStamp::Infinity() as well? Negative infinity? NaN? :-)
My gut feeling is that if we don't need infinity beyond this one case, you'd be better of using a separate flag to indicate infinity.
(Assignee)

Comment 19

6 years ago
Created attachment 674528 [details] [diff] [review]
v3: IdleService fix and clean up

Hi Justin, I've cleaned up the code and took out all the redundant suffix such as InTS/InTD, remove unnecessary type cast. As for the hack to an infinite value, I still leave it as it is now since we don't have an "infinite" value for TimeStamp now, but correct the comment. Could you review my patch? Thanks a lot.
(Assignee)

Updated

6 years ago
Attachment #674528 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

6 years ago
Attachment #674093 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> What would the semantics be? Would you have a TimeStamp::Infinity() as well?

Oh gosh, if you added infinity to a TimeStamp...

> Negative infinity? NaN? :-)

Okay, you've convinced me this is a bad idea.
Comment on attachment 674528 [details] [diff] [review]
v3: IdleService fix and clean up

It's still not cool to use UINT32_MAX to mean infinity.  If we're not going to add TimeDuration::Infinity(), we'll need to make due with adding a flag.  Or maybe we can use 0 to mean infinity in this case, like the comment originally said!  :)
Attachment #674528 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 22

6 years ago
Created attachment 674606 [details] [diff] [review]
v4: Remove the usage of UINT32_MAX

This patch removes the usage of UINT32_MAX and actually uses 0 to mean infinity.
Attachment #674528 - Attachment is obsolete: true
Attachment #674606 - Flags: feedback?(justin.lebar+bug)
>diff --git a/widget/xpwidgets/nsIdleService.h b/widget/xpwidgets/nsIdleService.h
> #include "nsIIdleServiceInternal.h"
> #include "nsCOMPtr.h"
> #include "nsITimer.h"
> #include "nsTArray.h"
> #include "nsIObserver.h"
> #include "nsIIdleService.h"
> #include "nsCategoryCache.h"
> #include "nsWeakReference.h"
>+#include "mozilla/TimeStamp.h"
> 
> /**
>  * Class we can use to store an observer with its associated idle time
>  * requirement and whether or not the observer thinks it's "idle".
>  */
> class IdleListener {
> public:
>   nsCOMPtr<nsIObserver> observer;
>@@ -150,25 +151,25 @@ protected:
>   virtual bool UsePollMode();
> 
> private:
>   /**
>    * Ensure that the timer is expiring at least at the given time
>    *
>    * The function might not restart the timer if there is one running currently
>    *
>-   * @param aNextTimeoutInPR
>+   * @param aNextTimeout
>    *        The last absolute time the timer should expire
>    */
>-  void SetTimerExpiryIfBefore(PRTime aNextTimeoutInPR);
>+  void SetTimerExpiryIfBefore(mozilla::TimeStamp aNextTimeout);
> 
>   /**
>    * Stores the next timeout time, 0 means timer not running
>    */
>-  PRTime mCurrentlySetToTimeoutAtInPR;
>+  mozilla::TimeStamp mCurrentlySetToTimeoutAt;
> 
>   /**
>    * mTimer holds the internal timer used by this class to detect when to poll
>    * for idle time, when to check if idle timers should expire etc.
>    */
>   nsCOMPtr<nsITimer> mTimer;
> 
>   /**
>@@ -185,26 +186,24 @@ private:
>    * Boolean indicating if any observers are in idle mode
>    */
>   bool mAnyObserverIdle;
> 
>   /**
>    * Delta time from last non idle time to when the next observer should switch
>    * to idle mode
>    *
>-   * Time in seconds
>-   *
>    * If this value is 0 it means there are no active observers
>    */
>-  uint32_t mDeltaToNextIdleSwitchInS;
>+  mozilla::TimeDuration mDeltaToNextIdleSwitch;
> 
>   /**
>    * Absolute value for when the last user interaction took place.
>    */
>-  PRTime mLastUserInteractionInPR;
>+  mozilla::TimeStamp mLastUserInteraction;
> 
> 
>   /**
>    * Function that ensures the timer is running with at least the minimum time
>    * needed.  It will kill the timer if there are no active observers.
>    */
>   void ReconfigureTimer(void);
> 
>
>diff --git a/widget/xpwidgets/nsIdleService.cpp b/widget/xpwidgets/nsIdleService.cpp
>--- a/widget/xpwidgets/nsIdleService.cpp
>+++ b/widget/xpwidgets/nsIdleService.cpp
>@@ -376,20 +376,20 @@ nsIdleService* gIdleService;
> 
> already_AddRefed<nsIdleService>
> nsIdleService::GetInstance()
> {
>   nsRefPtr<nsIdleService> instance(gIdleService);
>   return instance.forget();
> }
> 
>-nsIdleService::nsIdleService() : mCurrentlySetToTimeoutAtInPR(0),
>+nsIdleService::nsIdleService() : mCurrentlySetToTimeoutAt(TimeStamp()),
>                                  mAnyObserverIdle(false),
>-                                 mDeltaToNextIdleSwitchInS(UINT32_MAX),
>-                                 mLastUserInteractionInPR(PR_Now())
>+                                 mDeltaToNextIdleSwitch(0),
>+                                 mLastUserInteraction(TimeStamp::Now())
> {
> #ifdef PR_LOGGING
>   if (sLog == NULL)
>     sLog = PR_NewLogModule("idleService");
> #endif
>   MOZ_ASSERT(!gIdleService);
>   gIdleService = this;
>   mDailyIdle = new nsIdleServiceDaily(this);
>@@ -437,29 +437,30 @@ nsIdleService::AddIdleObserver(nsIObserv
>   if (!mTimer) {
>     nsresult rv;
>     mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   // Check if the newly added observer has a smaller wait time than what we
>   // are waiting for now.
>-  if (mDeltaToNextIdleSwitchInS > aIdleTimeInS) {
>+  if (mDeltaToNextIdleSwitch > TimeDuration::FromSeconds(aIdleTimeInS) ||

Please do the conversion of aIdleTimeInS to a TimeDuration only once.

>+      mDeltaToNextIdleSwitch.ToSeconds() == 0) {

Relying on the precise outcome of floating-point arithmetic like this makes me
nervous (although I think it will behave correctly in this case); how about we check

        mDeltaToNextIdleSwitch == TimeStamp(0)?

>@@ -502,43 +503,46 @@ nsIdleService::RemoveIdleObserver(nsIObs
> NS_IMETHODIMP
> nsIdleService::ResetIdleTimeOut(uint32_t idleDeltaInMS)
> {
>   PR_LOG(sLog, PR_LOG_DEBUG,
>          ("idleService: Reset idle timeout (last interaction %u msec)",
>           idleDeltaInMS));
> 
>   // Store the time
>-  mLastUserInteractionInPR = PR_Now() - (idleDeltaInMS * PR_USEC_PER_MSEC);
>+  mLastUserInteraction = TimeStamp::Now() - TimeDuration::FromMilliseconds(idleDeltaInMS);
> 
>   // If no one is idle, then we are done, any existing timers can keep running.
>   if (!mAnyObserverIdle) {
>     PR_LOG(sLog, PR_LOG_DEBUG,
>            ("idleService: Reset idle timeout: no idle observers"));
>     return NS_OK;
>   }
> 
>   // Mark all idle services as non-idle, and calculate the next idle timeout.
>   Telemetry::AutoTimer<Telemetry::IDLE_NOTIFY_BACK_MS> timer;
>   nsCOMArray<nsIObserver> notifyList;
>-  mDeltaToNextIdleSwitchInS = UINT32_MAX;
>+  mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(0);

TimeDuration(0) (or just TimeDuration(), which is equivalent).

>   // Loop through all listeners, and find any that have detected idle.
>   for (uint32_t i = 0; i < mArrayListeners.Length(); i++) {
>     IdleListener& curListener = mArrayListeners.ElementAt(i);
> 
>     // If the listener was idle, then he shouldn't be any longer.
>     if (curListener.isIdle) {
>       notifyList.AppendObject(curListener.observer);
>       curListener.isIdle = false;
>     }
>-
>+    // Assign the first element to it since it is initialized to 0.
>+    if (mDeltaToNextIdleSwitch.ToSeconds() == 0) {

== TimeDuration(0)

>+      mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(curListener.reqIdleTime);

Can we make IdleListener.reqIdleTime be a TimeDuration?  Because...

>+    }
>     // Check if the listener is the next one to timeout.
>-    mDeltaToNextIdleSwitchInS = NS_MIN(mDeltaToNextIdleSwitchInS,
>-                                       curListener.reqIdleTime);
>+    mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(NS_MIN((uint32_t)mDeltaToNextIdleSwitch.ToSeconds(),
>+                                                              curListener.reqIdleTime));

...that would make this line much simpler.  (It'd be a straight NS_MIN of the
two TimeDurations.)

>@@ -639,17 +643,17 @@ nsIdleService::StaticIdleTimerCallback(n
> {
>   static_cast<nsIdleService*>(aClosure)->IdleTimerCallback();
> }
> 
> void
> nsIdleService::IdleTimerCallback(void)
> {
>   // Remember that we no longer have a timer running.
>-  mCurrentlySetToTimeoutAtInPR = 0;
>+  mCurrentlySetToTimeoutAt = TimeStamp();
> 
>   // Get the current idle time.
>   uint32_t currentIdleTimeInMS;
> 
>   if (NS_FAILED(GetIdleTime(&currentIdleTimeInMS))) {
>     PR_LOG(sLog, PR_LOG_ALWAYS,
>            ("idleService: Idle timer callback: failed to get idle time"));
> #ifdef ANDROID
>@@ -667,41 +671,40 @@ nsIdleService::IdleTimerCallback(void)
>                       "Idle timer callback: current idle time %u msec",
>                       currentIdleTimeInMS);
> #endif
> 
>   // Check if we have had some user interaction we didn't handle previously
>   // we do the calculation in ms to lessen the chance for rounding errors to
>   // trigger wrong results, it is also very important that we call PR_Now AFTER
>   // the call to GetIdleTime().
>-  if (((PR_Now() - mLastUserInteractionInPR) / PR_USEC_PER_MSEC) >
>-      currentIdleTimeInMS)
>+  if ((TimeStamp::Now() - mLastUserInteraction) > TimeDuration::FromMilliseconds(currentIdleTimeInMS))

I'd prefer if we converted currentIdleTimeInMS to a TimeDuration just once.
Also, please wrap lines to 80 characters.

>   {
>     // We had user activity, so handle that part first (to ensure the listeners
>     // don't risk getting an non-idle after they get a new idle indication.
>     ResetIdleTimeOut(currentIdleTimeInMS);
> 
>     // NOTE: We can't bail here, as we might have something already timed out.
>   }
> 
>   // Find the idle time in S.
>   uint32_t currentIdleTimeInS = currentIdleTimeInMS / PR_MSEC_PER_SEC;
> 
>   // Restart timer and bail if no-one are expected to be in idle
>-  if (mDeltaToNextIdleSwitchInS > currentIdleTimeInS) {
>+  if (mDeltaToNextIdleSwitch > TimeDuration::FromMilliseconds(currentIdleTimeInMS)) {
>     // If we didn't expect anyone to be idle, then just re-start the timer.
>     ReconfigureTimer();
>     return;
>   }
> 
>   // Tell expired listeners they are expired,and find the next timeout
>   Telemetry::AutoTimer<Telemetry::IDLE_NOTIFY_IDLE_MS> timer;
> 
>-  // We need to initialise the time to the next idle switch.
>-  mDeltaToNextIdleSwitchInS = UINT32_MAX;
>+  // We need to initialize the time to the next idle switch.
>+  mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(0);

TimeDuration(0)

>@@ -711,19 +714,23 @@ nsIdleService::IdleTimerCallback(void)
>         // Then add the listener to the list of listeners that should be
>         // notified.
>         notifyList.AppendObject(curListener.observer);
>         // This listener is now idle.
>         curListener.isIdle = true;
>         // Remember we have someone idle.
>         mAnyObserverIdle = true;
>       } else {
>+        // Assign the first element to it since it is initialized to 0.
>+        if (mDeltaToNextIdleSwitch.ToSeconds() == 0) {

TimeDuration(0)

>+          mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(curListener.reqIdleTime);
>+        }
>         // Listeners that are not timed out yet are candidates for timing out.
>-        mDeltaToNextIdleSwitchInS = NS_MIN(mDeltaToNextIdleSwitchInS,
>-                                           curListener.reqIdleTime);
>+        mDeltaToNextIdleSwitch = TimeDuration::FromSeconds(NS_MIN((uint32_t)mDeltaToNextIdleSwitch.ToSeconds(),
>+                                                                  curListener.reqIdleTime));

Maybe put an |else| after the if statement above (and in the other place that
you do this?) to make it clearer what's going on?

> void
> nsIdleService::ReconfigureTimer(void)
> {
>   // Check if either someone is idle, or someone will become idle.
>-  if (!mAnyObserverIdle && UINT32_MAX == mDeltaToNextIdleSwitchInS) {
>+  if (!mAnyObserverIdle && 0 == mDeltaToNextIdleSwitch.ToSeconds()) {

Please flip this around; we do i == 0, not 0 == i.  And TimeDuration(0).

>     // If not, just let any existing timers run to completion
>     // And bail out.
>     PR_LOG(sLog, PR_LOG_DEBUG,
>            ("idleService: ReconfigureTimer: no idle or waiting observers"));
> #ifdef ANDROID
>   __android_log_print(ANDROID_LOG_INFO, "IdleService",
>                       "ReconfigureTimer: no idle or waiting observers");
> #endif
>     return;
>   }
>+  // Double check if there are no active observers.
>+  if (0 == mDeltaToNextIdleSwitch.ToSeconds()) {
>+    return;
>+  }

ibid.

But...why are you adding this exactly?  It's effectively removing the
|!mAnyObserverIdle| check from the if statement above.

>   // Check if we should correct the timeout time because we should poll before.
>   if (mAnyObserverIdle && UsePollMode()) {
>-    PRTime pollTimeout = curTimeInPR +
>-                         MIN_IDLE_POLL_INTERVAL_MSEC * PR_USEC_PER_MSEC;
>+    TimeStamp pollTimeout = curTime +
>+                            TimeDuration::FromMilliseconds(MIN_IDLE_POLL_INTERVAL_MSEC);

This line is too long, but if you wrap it after the =, it might be short enough.

These are mostly nits (aside from the question above), but do you mind if I have another look at this?

I'm not sure who owns this code, exactly, but I see that gcp and mak have reviewed in here lately.  I feel comfortable reviewing this, but we should ask them if they want to have a look too.
Comment on attachment 674606 [details] [diff] [review]
v4: Remove the usage of UINT32_MAX

gcp, mak, are you OK delegating this review to me?  (Do you even own this code?  :)
Attachment #674606 - Flags: feedback?(justin.lebar+bug)
>Do you even own this code?  :)

No, I just broke and then fixed it recently. I'll have a look anyway ;-)
(Assignee)

Comment 26

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #23)

> These are mostly nits (aside from the question above), but do you mind if I
> have another look at this?

Not at all, and thanks for your feedback, I'll clean it up in the mean time. :)
 
> I'm not sure who owns this code, exactly, but I see that gcp and mak have
> reviewed in here lately.  I feel comfortable reviewing this, but we should
> ask them if they want to have a look too.
(Assignee)

Comment 27

6 years ago
Created attachment 674953 [details] [diff] [review]
New methonds for TimeDuration

Hi, I'm proposing two new methods for TimeDuration. The IsZero() returns true if this is a zero duration(or should I call IsNull?), and GetMin() returns the smaller duration. It could save some type casting work here, but I'm not sure if they are worth adding?
Attachment #674953 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 674953 [details] [diff] [review]
New methonds for TimeDuration

IsZero looks good. Couldn't you just as easily use NS_MIN as GetMin?
TimeDuration::GetMin(a, b) should be the same as NS_MIN(a, b).  Or does that not work for some reason?

I think IsZero/IsNull() is not particularly obvious compared to |foo == TimeDuration(0)| so I'm not sold on adding it, myself.  But I don't feel very strongly.

What might be cool is if we could let you do |foo == 0|, but not |foo == 1|, and if we could verify that statically.  (As we do with the constructor, or like CSS lets you specify a unitless 0 length but not a unitless 1 length.)  But I don't know if that's possible, and maybe it's over-engineering.
Actually, foo == 0 would just work right now, right?
(Assignee)

Comment 31

6 years ago
roc, NS_MIN works just fine, my bad!

I just thought IsZero would make it look a little bit cleaner, but I'm totally cool with using foo == 0.
> but I'm totally cool with using foo == 0.

I'm happy with that if roc is.
Attachment #674953 - Flags: feedback?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #32)
> > but I'm totally cool with using foo == 0.
> 
> I'm happy with that if roc is.

Yes.
(Assignee)

Comment 34

6 years ago
Created attachment 675171 [details] [diff] [review]
v5: More clean up

Code style fix and clean up.
Attachment #674606 - Attachment is obsolete: true
Attachment #674953 - Attachment is obsolete: true
Attachment #675171 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Comment 35

6 years ago
Created attachment 675493 [details] [diff] [review]
v6: Use TimeDuration instead of TimeStamp to store the next timeout

Hi Justin, after running the v5 patch on try server, I got this result:

https://tbpl.mozilla.org/?tree=Try&rev=ef291eab32a1

The orange result indicates an assertion of using > to compare TimeStamp if either of the compared element is null(zero), which in this case, |mCurrentlySetToTimeoutAt| might be zero. Although its original logic isn't incorrect, but I think using TimeDuration instead of TimeStamp can avoid the assertion?
Attachment #675493 - Flags: feedback?(justin.lebar+bug)
I don't understand how this is supposed to work.

mCurrentlySetToTimeoutAt is now a duration, not an absolute time.  It reflects...what, exactly?  It seems like it reflects how far into the future we want to fire the alarm, measured during the call to SetTimerExpiryIfBefore().  But then how can you read a meaningful value out of mCurrentlySetToTimeoutAt anywhere else?  (That is, how can this be a member variable?)

This is all pretty confusing to me.  If you think this is the way to go, it seems like a lot of comments and probably some variable names need to be changed, so we can understand why this is correct.

It sounds to me like it would be simpler to build a wrapper class around TimeStamp which gives you the semantics for operator < that you want.
> It sounds to me like it would be simpler to build a wrapper class around TimeStamp which 
> gives you the semantics for operator < that you want.

An alternative would be to write LessThan(x, y) and Min(x, y) methods for TimeStamps which check IsNull().
Comment on attachment 675171 [details] [diff] [review]
v5: More clean up

> +  if (mDeltaToNextIdleSwitch > aIdleTime ||
> +      mDeltaToNextIdleSwitch == TimeDuration()) {

I thought we decided on == 0 here and elsewhere?  Or does that not work?
Attachment #675171 - Flags: feedback?(justin.lebar+bug)
Attachment #675493 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Comment 39

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #36)
> I don't understand how this is supposed to work.
> 
> mCurrentlySetToTimeoutAt is now a duration, not an absolute time.  It
> reflects...what, exactly?  It seems like it reflects how far into the future
> we want to fire the alarm, measured during the call to
> SetTimerExpiryIfBefore().  

From the previously logic, SetTimerExpiryIfBefore(aNextTimeout) starts the timer with a callback function, and calculates the delay time of callback base on either mCurrentlySetToTimeoutAt or aNextTimeout, by doing mCurrentlySetToTimeoutAt - now to get a delay duration. So that's why I think using TimeDuration does not really affect the result, but true, the name of this parameter should really be something else.

> But then how can you read a meaningful value out
> of mCurrentlySetToTimeoutAt anywhere else?  (That is, how can this be a
> member variable?)
> 

Sorry, I don't really get it?

> This is all pretty confusing to me.  If you think this is the way to go, it
> seems like a lot of comments and probably some variable names need to be
> changed, so we can understand why this is correct.
> 
> It sounds to me like it would be simpler to build a wrapper class around
> TimeStamp which gives you the semantics for operator < that you want.
(Assignee)

Comment 40

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #38)
> Comment on attachment 675171 [details] [diff] [review]
> v5: More clean up
> 
> > +  if (mDeltaToNextIdleSwitch > aIdleTime ||
> > +      mDeltaToNextIdleSwitch == TimeDuration()) {
> 
> I thought we decided on == 0 here and elsewhere?  Or does that not work?

Yes it works, I shall change it in the next patch.
What does mCurrentlySetToTimeoutAt represent?  What would you rename it to?

In the existing code, I think we calculate how long we have to wait until the currently-set timer will fire by doing mCurrentlySetToTimeoutAt - now, like you say.

> Sorry, I don't really get it?

Yeah, it's confusing, sorry.  :)  In this patch, mCurrentlySetToTimeoutAt is a duration.  It can't represent how long we have to wait until the currently-set timeout will fire, because you have to take now() into account to compute that.  So I don't understand how mCurrentlySetToTimeoutAt can hold a meaningful value.

Does that make sense?

Whatever we do with the TimeStamps, it seems we should encapsulate this IsNull()-checking for TimeDuration somehow, whether it's in some a new wrapper you'll write or with new comparison/min methods, or something.  It's easy to leave off an "== 0" and mess up the logic.  We could have similar methods for TimeStamp, which might solve your mCurrentlySetToTimeoutAt problem.
(Assignee)

Comment 42

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #41)
> What does mCurrentlySetToTimeoutAt represent?  What would you rename it to?
> 
> In the existing code, I think we calculate how long we have to wait until
> the currently-set timer will fire by doing mCurrentlySetToTimeoutAt - now,
> like you say.
> 
If we go with the duration implementation, maybe something like mCurrentlySetTimeoutDelay?

> > Sorry, I don't really get it?
> 
> Yeah, it's confusing, sorry.  :)  In this patch, mCurrentlySetToTimeoutAt is
> a duration.  It can't represent how long we have to wait until the
> currently-set timeout will fire, because you have to take now() into account
> to compute that.  So I don't understand how mCurrentlySetToTimeoutAt can
> hold a meaningful value.
> 
> Does that make sense?
> 
Yep, crystal clear, thanks a lot:)

> Whatever we do with the TimeStamps, it seems we should encapsulate this
> IsNull()-checking for TimeDuration somehow, whether it's in some a new
> wrapper you'll write or with new comparison/min methods, or something.  It's
> easy to leave off an "== 0" and mess up the logic.  We could have similar
> methods for TimeStamp, which might solve your mCurrentlySetToTimeoutAt
              ^^^^^^^^^^ Do you mean TimeDuraton here? Or I should write a wrapper with new comparison method for TimeStamp because we do get zero TimeStamp in this case?
> problem.

I think the assertion for TimeStamp is more of a semantic reason. Please see the following for more details:

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h#250

While in the original logic of IdleService, it uses |mCurrentlySetToTimeoutAtInPR = 0| to indicate there are no timer running. When I swap it with TimeStamp, keeping |mCurrentlySetToTimeoutAt = TimeStamp()| as no timer running, the result is correct but I got an orange result from try server(due to the assertion). I then notice there isn't a isNull()-checking for TimeDuration(and from the semantic point of view, a duration can be zero), that's why I switch from using TimeStamp to TimeDuration.

But if you suggest it's better to keep the original logic, which is, using TimeStamp to keep track of an absolute time instead of passing a duration, I think I can avoid the comparison of two TimeStamp by calculate their duration inside the function, and compare with the two time durations.
> I think the assertion for TimeStamp is more of a semantic reason.

Agreed; I don't think we should change the semantics of TimeStamp itself.

> But if you suggest it's better to keep the original logic, which is, using TimeStamp to 
> keep track of an absolute time instead of passing a duration,

I don't have a problem in principal using a TimeDuration, but of course the code has to be correct.  :)  I see how to write correct code using a TimeStamp, while it's not clear to me how to do this correctly using a TimeDuration.  But maybe you have an idea...

If you think the best way to solve this is to use a TimeStamp and write a wrapper method for comparing two TimeStamps (and probably another one for the MIN operation), that would be fine with me.

> Do you mean TimeDuraton here? Or I should write a wrapper with new comparison method 
> for TimeStamp because we do get zero TimeStamp in this case?

Sorry that wasn't clear.  I just mean that we probably shouldn't write

  if (x == 0 || x < y)

and

  x = x == 0 ? y : min(x, y)

over and over again, for whatever type you decide to use for x and y.
(Assignee)

Comment 44

6 years ago
Created attachment 676542 [details] [diff] [review]
v7: more clean up and wrapper implementation

Please find my latest changes from the following:

- Add a wrapper to return the shorter delta time from last non idle time to when the next observer should switch.
- Add a wrapper to return true if there are no active observers.
- Pull mCurrentlySetToTimeoutAt.IsNull() ahead to avoid the assertion of comparing two time stamps.

Thanks!
Attachment #675171 - Attachment is obsolete: true
Attachment #675493 - Attachment is obsolete: true
Attachment #676542 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 676542 [details] [diff] [review]
v7: more clean up and wrapper implementation

>+  /**
>+   * Return the shorter delta time from last non idle time to when the next
>+   * observer should switch.
>+   *
>+   * If timer is not running, return the aReqIdleTime.
>+   */
>+  mozilla::TimeDuration
>+  GetShorterIdleSwitchTime(mozilla::TimeDuration aReqIdleTime);
>+
>+  /**
>+   * Return true if there are no active observers
>+   */
>+  bool NoActiveObservers() {
>+    return mDeltaToNextIdleSwitch == 0;
>+  }

I'm really sorry because I think I lead you astray: I don't think either of
these methods makes the code easier to follow.  While reviewing this patch I
found myself constantly looking back to see which member variable they touch.

Part of what makes this class difficult to follow is all the member variables
it uses.  You never know which member variables a method is going to read and
modify.  If you can write methods which don't use any member variables,
particularly when all the rest of the code uses the member variables directly,
as this class does, that often makes things clearer.

I was thinking that you might have something like

  GetMin(TimeDuration a, TimeDuration b);
  IsLessThan(TimeDuration a, TimeDuration b);

  GetEarliest(TimeStamp a, TimeStamp b);
  IsEarlierThan(TimeStamp a, TimeStamp b);

with a constraint that one of |a| and |b| must not be null/zero.  But maybe
that's too cumbersome and complicated for this situation.  It's kind of hard
for me to tell without writing the code.

I'd say we should just give up on this abstraction and manually check
TimeDuration == 0 and TimeStamp.IsNull(), but I've found at least one place
where it looks like you forgot to check the TimeDuration == 0, and you know to
look out for this bug!  That's not a knock against you; this is hard to get
right.  But it suggests to me it will be hard for others to write correct code
here.

What if we gave up on using TimeDuration and instead went back to storing this
mDeltatoNextIdleSwitch as a uint32_t?

I feel like I'm micromanaging your design decisions here, which is really bad
of me.  Feel free to completely ignore all of the specifics here if you think
I'm wrong.  I just want us to come up with a way to make this code correct,
easy to read, and easy to maintain.

>diff --git a/widget/xpwidgets/nsIdleService.cpp b/widget/xpwidgets/nsIdleService.cpp

>   // Restart timer and bail if no-one are expected to be in idle
>-  if (mDeltaToNextIdleSwitchInS > currentIdleTimeInS) {
>+  if (mDeltaToNextIdleSwitch > currentIdleTime) {

Don't you need to check mDeltaToNextIdleSwitch == 0 here?

> void
>-nsIdleService::SetTimerExpiryIfBefore(PRTime aNextTimeoutInPR)
>+nsIdleService::SetTimerExpiryIfBefore(TimeStamp aNextTimeout)
> {
>+#ifdef DEBUG
>+  TimeDuration nextTimeoutDuration = aNextTimeout - TimeStamp::Now();
>   PR_LOG(sLog, PR_LOG_DEBUG,
>-         ("idleService: SetTimerExpiryIfBefore: next timeout %lld usec",
>-          aNextTimeoutInPR));
>+         ("idleService: SetTimerExpiryIfBefore: next timeout %f msec from now",
>+          nextTimeoutDuration.ToMilliseconds()));
> #ifdef ANDROID
>   __android_log_print(ANDROID_LOG_INFO, "IdleService",
>-                      "SetTimerExpiryIfBefore: next timeout %lld usec",
>-                      aNextTimeoutInPR);
>+                      "SetTimerExpiryIfBefore: next timeout %f msec from now",
>+                      nextTimeoutDuration.ToMilliseconds());
>+#endif
> #endif

This is a functional change here and the other places where you do this; the
Android code as it exists will print to the logs even if we're not in a debug
build.  You probably should preserve the current behavior.

>     // Check that the timeout is actually in the future, otherwise make it so.
>-    PRTime currentTimeInPR = PR_Now();
>-    if (currentTimeInPR > mCurrentlySetToTimeoutAtInPR) {
>-      mCurrentlySetToTimeoutAtInPR = currentTimeInPR;
>+    TimeStamp currentTime = TimeStamp::Now();
>+    if (mCurrentlySetToTimeoutAt.IsNull() ||
>+        currentTime > mCurrentlySetToTimeoutAt) {

aNextTimeout shouldn't be null, so mCurrentlySetToTimeoutAt shouldn't be null
here either, right?  (Sorry, I deleted some important context above.)

> void
> nsIdleService::ReconfigureTimer(void)
> {
>-  // Check if either someone is idle, or someone will become idle.
>-  if (!mAnyObserverIdle && UINT32_MAX == mDeltaToNextIdleSwitchInS) {
>+  // Check if someone will become idle.
>+  if (NoActiveObservers()) {

Can you explain why it's safe to remove !mAnyObserverIdle here?  Not that I
don't believe you, but this code is hard to trace through.

>     // If not, just let any existing timers run to completion
>     // And bail out.
>     PR_LOG(sLog, PR_LOG_DEBUG,
>            ("idleService: ReconfigureTimer: no idle or waiting observers"));
> #ifdef ANDROID
>   __android_log_print(ANDROID_LOG_INFO, "IdleService",
>                       "ReconfigureTimer: no idle or waiting observers");

Please update these messages.  (And let us meditate on how sad it is that
this file does not have a unified logging macro.)
Attachment #676542 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Comment 46

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #45)
Hi Justin,
Thanks for the clarification.
> I'd say we should just give up on this abstraction and manually check
> TimeDuration == 0 and TimeStamp.IsNull(), but I've found at least one place
> where it looks like you forgot to check the TimeDuration == 0, and you know
> to
If it is the place you've pointed out below, please see my comment below.

> What if we gave up on using TimeDuration and instead went back to storing
> this
> mDeltatoNextIdleSwitch as a uint32_t?
> 

I'm cool with using either TimeDuration or uint32_t, the reason I chose TimeDuration is just because I get a return type of TimeDuration after subtracting two TimeStamps, and thought it would be a little bit clearer in a symmetric way. The main reason of seeing so many TimeDuration == 0 checks, is we now use |mDeltaToNextIdleSwitch == 0| instead of |mDeltaToNextIdleSwitch == UINT32_MAX| to mean there are no active observers. 

The original logic sort of hack around the checking by initializing it with a fake infinite value, so there's no need to write

a = (a == 0) ? b : NS_MIN(a, b);

or add |a == 0| in the condition of |if (a > b)|

> >diff --git a/widget/xpwidgets/nsIdleService.cpp b/widget/xpwidgets/nsIdleService.cpp
> 
> >   // Restart timer and bail if no-one are expected to be in idle
> >-  if (mDeltaToNextIdleSwitchInS > currentIdleTimeInS) {
> >+  if (mDeltaToNextIdleSwitch > currentIdleTime) {
> 
> Don't you need to check mDeltaToNextIdleSwitch == 0 here?
> 
No, because in the callback function, mDeltaToNextIdleSwitch shouldn't be 0, which is if there's no active observers, then we shouldn't fire a callback (please correct me if I'm wrong)

> 
> >     // Check that the timeout is actually in the future, otherwise make it so.
> >-    PRTime currentTimeInPR = PR_Now();
> >-    if (currentTimeInPR > mCurrentlySetToTimeoutAtInPR) {
> >-      mCurrentlySetToTimeoutAtInPR = currentTimeInPR;
> >+    TimeStamp currentTime = TimeStamp::Now();
> >+    if (mCurrentlySetToTimeoutAt.IsNull() ||
> >+        currentTime > mCurrentlySetToTimeoutAt) {
> 
> aNextTimeout shouldn't be null, so mCurrentlySetToTimeoutAt shouldn't be null
> here either, right?  (Sorry, I deleted some important context above.)
> 
Yes you're right.

> > void
> > nsIdleService::ReconfigureTimer(void)
> > {
> >-  // Check if either someone is idle, or someone will become idle.
> >-  if (!mAnyObserverIdle && UINT32_MAX == mDeltaToNextIdleSwitchInS) {
> >+  // Check if someone will become idle.
> >+  if (NoActiveObservers()) {
> 
> Can you explain why it's safe to remove !mAnyObserverIdle here?  Not that I
> don't believe you, but this code is hard to trace through.
> 
I totally agree with you that this file is very confusing.

To be honest, I don't have confidence saying removing !mAnyObserverIdle here is safe. I would probably keep the original logic and put another check after it, something like:

if (mDeltaToNextIdleSwitch == 0)  return;

Or not calling SetTimerExpiryIfBefore() if nextTimeoutAt is negative.

Again, it worked because mDeltaToNextIdleSwitchInS was an "infinite" value, so the callback seems never be fired.

I have a version switching back using uint32_t, but keeping its original implementation of initializing it with UINT32_MAX. Or should I use the 0 version, and manually checking mDeltaToNextIdleSwitchInS == 0 around (or wrapper it with wrapper)?
> No, because in the callback function, mDeltaToNextIdleSwitch shouldn't be 0, which is if there's no 
> active observers, then we shouldn't fire a callback (please correct me if I'm wrong)

>   // Restart timer and bail if no-one are expected to be in idle
>   if (mDeltaToNextIdleSwitchInS > currentIdleTimeInS) {
>     // If we didn't expect anyone to be idle, then just re-start the timer.
>     ReconfigureTimer();
>     return;
>   }

It seems like they're guarding against getting a callback too soon -- that is, when we haven't been idle for mDeltaToNextIdleSwitchInS seconds.  If that can happen, I'd guess that it could also happen that mDeltaToNextIdleSwitchInS is not initialized when we get a callback.  But I don't see how either can happen!

I'm not confident enough to get rid of the guard, so I'm probably not confident enough to get rid of the null-check either.  But if you switch to uint32_t, then it doesn't matter.

(Maybe that check is there because they used to modify mDeltaToNextIdleSwitchInS in RemoveIdleObserver; that's my best guess, but it doesn't apply anymore.)

> To be honest, I don't have confidence saying removing !mAnyObserverIdle here is safe.

Great, let's keep it then.  I don't want to think about it either.  :)

> I have a version switching back using uint32_t, but keeping its original implementation of 
> initializing it with UINT32_MAX.

Yes, this is what I was thinking would be sane, because then we can use > and MAX without any fuss.
(Assignee)

Comment 48

6 years ago
Created attachment 677261 [details] [diff] [review]
v8: Switch back using uint32_t

This patch should only change the usage of PRTime to TimeStamp. Thanks.
Attachment #676542 - Attachment is obsolete: true
Attachment #677261 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 677261 [details] [diff] [review]
v8: Switch back using uint32_t

Hooray.  Thanks, Shelly.

Just one nit here: Could you please change %f to %0.f in the logging statements?  We don't care about fractional ms here.
Attachment #677261 - Flags: feedback?(justin.lebar+bug) → feedback+
Attachment #677261 - Flags: feedback+ → review+
(Assignee)

Comment 50

6 years ago
Created attachment 677637 [details] [diff] [review]
part1: use TimeStamp instead of PRTime in IdleService

Thanks for your time. :D

Here's the try-server result:
https://tbpl.mozilla.org/?tree=Try&rev=bb277c4e16fe
https://tbpl.mozilla.org/?tree=Try&rev=2819105ddaf6
(Test failed at mochitest-browser-chrome, in file browser_dbg_createRemote.js, but doesn't look like it has something to do with changing the idle time to a monotonic time stamp.)
(Assignee)

Comment 51

6 years ago
Comment on attachment 677637 [details] [diff] [review]
part1: use TimeStamp instead of PRTime in IdleService

Carry r+.
Attachment #677637 - Flags: review+
(Assignee)

Updated

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

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6074493479

Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
> Should this have a test?

This is pretty hard to test without changing the system clock.  Maybe we can do that in a Marionette test; I don't really know how that works.
https://hg.mozilla.org/mozilla-central/rev/ed6074493479
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/d130df80561d
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Flags: in-testsuite? → in-testsuite-

Updated

6 years ago
Depends on: 811459
I believe this broke the idle notification reporting in Firefox (Windows).  See bug 811459.
Sorry but I had to back this out because of bug 811459. There is an uplift in the next few days and we don't want to regress desktop beta.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b72b916c65d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e98e3ed1a671
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/releases/mozilla-aurora/rev/5516a682b842
status-firefox18: fixed → affected
status-firefox19: fixed → affected

Updated

6 years ago
Target Milestone: --- → B2G C2 (20nov-10dec)
The fix for this bug has been called out as possibly having greater than normal risk to non-B2G platforms. Moving into the C2 milestone. We should try to resolve as soon as possible on mozilla-beta, to prevent the need for branching B2G from mozilla-beta earlier than planned.
tracking-firefox18: --- → ?
This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.
Priority: P2 → P1

Updated

6 years ago
tracking-firefox18: ? → ---
Do we have a plan for getting this to stick?
Flags: needinfo?(slin)
Yes; active discussion continues in bug 811459.
Flags: needinfo?(slin)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #677637 - Attachment description: v8: Final clean up. → part1: use TimeStamp instead of PRTime in IdleService
(Assignee)

Comment 65

6 years ago
Please checkin the part1 first, then the part2 from attachments, thank you very much.
(Assignee)

Comment 66

6 years ago
I notice that the original issue "Screen turns idle and then sleep after changing the Date or Time" is still there even with the patch applied. 

There are lots of changes related with screen timeout recently from the git log of screen_manager.js. Turns out that the patch fixes the issue after backing out the file screen_manager.js to revision cafa98c2979b6cdd8c5014b7f0a09ec87b87fda5.

Still trying to find out the root cause.
> I notice that the original issue "Screen turns idle and then sleep after changing the 
> Date or Time" is still there even with the patch applied. 

Okay, so presumably you /don't/ want checkin-needed here then?
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Depends on: 817951

Comment 68

6 years ago
After talking with Shelly, this should be a Gaia issue. Once 817951 is solved, this one should be resolved.
Component: General → Gaia

Comment 69

6 years ago
IMO, maybe we can close this one and keep bug 817951 opened to track this issue?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 70

6 years ago
Bug 817951 has fixed and landed.
(In reply to khu from comment #68)
> After talking with Shelly, this should be a Gaia issue. Once 817951 is
> solved, this one should be resolved.

No, both fix needs to land to prevent STR from happening. This is the Gecko part waiting for check-in.
Component: Gaia → General
(Assignee)

Comment 72

6 years ago
Comment on attachment 687642 [details] [diff] [review]
part2: fix of bug 811459

Carry r+. Please checkin the part1 first, then the part2, thanks.
Attachment #687642 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4998bde6d220
https://hg.mozilla.org/mozilla-central/rev/f3ef58ba9e55
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.