Closed Bug 796255 Opened 12 years ago Closed 12 years ago

Alarm doesn’t ring on time if the app is killed and phone is allowed to suspend

Categories

(Firefox OS Graveyard :: General, defect, P1)

defect

Tracking

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

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ghtobz, Assigned: airpingu)

References

Details

(Whiteboard: [label:clock])

Attachments

(3 files, 7 obsolete files)

[GitHub issue by fabi1cazenave on 2012-09-13T08:37:03Z, https://github.com/mozilla-b2g/gaia/issues/4682]
Yesterday at midnight plus something, I’ve set the alarm for 9:00 am. It rang at 9:21 am.
[GitHub comment by ian-liu on 2012-09-21T10:47:10Z]
@fabi1cazenave 
I can not reproduce it now.
Do you have more info in this case?
[GitHub comment by fabi1cazenave on 2012-09-21T10:50:10Z]
Yesterday, the alarm rang with a 2:16 hour delay (set to 9:00 at 23:20, rang at 11:16).
[GitHub comment by autonome on 2012-09-26T23:58:33Z]
we need some reproducible steps here.
[GitHub comment by nhirata on 2012-09-27T00:13:35Z]
Marking QA wanted.  I will try to look into it.
Priority: -- → P1
It looks like if the phone is asleep then it won't keep sound the alarm.

STR: 
1. set alarm for 15 mins ahead
2. sleep the phone for 20 mins

Expected: 
15 mins, the alarm rings

Actual:
alarm rings when you wake it up at 20 mins.
Whiteboard: [label:qa-wanted][label:clock] → [label:clock]
Severity: normal → critical
Thanks alex!  

To note, it's not the lockscreen as an issue.  ( I had turned off the lockscreen to verify )
Gene, can something in Alarm API triggers behavior like this?
cannot reproduce this bug,
I tried 1min, 5min, 20min and turn off all communication incldes wifi, bluetooth, gps and turn on airplane mode, still cannot reproduce.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> Gene, can something in Alarm API triggers behavior like this?

Absolutely we can. That's what Alarm API designed for ;)

1. If alarm fired when sleeping, it will wake it up and ring at the setting time.
2. If alarm fired when powering-off, it will still ring after powering-on (after homescreen loaded).
(In reply to Gene Lian [:gene] from comment #9)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> > Gene, can something in Alarm API triggers behavior like this?
> 
> Absolutely we can. That's what Alarm API designed for ;)
> 
> 1. If alarm fired when sleeping, it will wake it up and ring at the setting
> time.
> 2. If alarm fired when powering-off, it will still ring after powering-on
> (after homescreen loaded).

No. My question was, from the STR, can you think of anything wrong in the Alarm API that would cause this?
(In reply to Naoki Hirata :nhirata from comment #5)
> It looks like if the phone is asleep then it won't keep sound the alarm.
> 
> STR: 
> 1. set alarm for 15 mins ahead
> 2. sleep the phone for 20 mins
> 
> Expected: 
> 15 mins, the alarm rings
> 
> Actual:
> alarm rings when you wake it up at 20 mins.

Hi :nhirata :)

I cannot reproduce this bug with the latest codes today either. The alarm attention screen can successfully pop up during sleeping. I'm wondering this one has been fixed, though. I'd appreciate if you could please give it try again. If possible, is that convenient for you to open the following debugs:

http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#8
http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#8
http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmDB.jsm#10

Thanks!
Whiteboard: [label:clock] → [label:clock][label:qa-wanted]
Seems like it is fixed in today's build.  :fabi1cazenave, can you try again please?
Flags: needinfo?(kaze)
Whiteboard: [label:clock][label:qa-wanted] → [label:clock]
I don’t think the bug I reported was related to the lockscreen: the alarm rang, it just did not ring on time. There was a minimal error when I set the alarm for 10 min ahead, and this error becomes very significant when I set the alarm several hours ahead.

I’ll test the alarm to wake me up tomorrow, and I’ll report the result here.
Flags: needinfo?(kaze)
Hi Kaze,

I suggest you could put the mobile near your body, because currently the alarm doesn't sound due to Bug 800651. Vibration sill works, though. Hope you won't get late for work tomorrow...
Hi Kaze and Gene,

Maybe we can have a test for the issue every morning...
(In reply to Ian Liu [:ianliu] from comment #15)
> Hi Kaze and Gene,
> 
> Maybe we can have a test for the issue every morning...

Well, I'd expect it will go off a bit late then I can have a proper excuse to go work late... Ha, just kidding :P Actually, I did this many times before and it always works for me.
(In reply to Gene Lian [:gene] from comment #14)
> I suggest you could put the mobile near your body, because currently the
> alarm doesn't sound due to Bug 800651. Vibration sill works, though.

Hmm, good to know. Just set the alarm 14h ahead from now, I’ll tell you how it went.
also keep in mind bug 804702.  when you unlock the phone it will show the current time not the alarm time...
Ian - what do you need to move forward with this critical bug? Are we looking for explicit STR? If so, please add qawanted.
(In reply to Alex Keybl [:akeybl] from comment #19)
> Ian - what do you need to move forward with this critical bug? Are we
> looking for explicit STR? If so, please add qawanted.

added.

I am actually waken up by the Clock app alarm this morning (by the sound of vibration, no ringtone though), so I cannot reproduce this bug either.
Keywords: qawanted
It works for me too. I cannot reproduce this bug either.
I saw this in bug 803649 but it was fine this morning
Not sure if we need QAwanted... we couldn't reproduce this bug any more ( see comment 12 )
(In reply to Naoki Hirata :nhirata from comment #24)
> Not sure if we need QAwanted... we couldn't reproduce this bug any more (
> see comment 12 )

Sent email to see what info we can get from affected test drivers - they seem to still be running into alarm issues.
They may need a stable build past 10-18 (which is the last stable unagi build).
Also please let them know about bug 804702.
Gene and I find out a way to make the issue happened easily.

1. Set an alarm 2 minutes(bigger than 2 minutes) later.
2. Go back to home screen
3. Long press home screen to kill Clock APP from background
4. Wait the alarm goes off

We could reproduce it sometimes.
But not every time.
Finally I got a more accurate procedure to reproduce this issue. ;)

STR:
==========
  1) Unconnected the USB line (this is the key step!).
  2) Launch Clock App to set an alarm for 2 mins ahead.
  3) Long tap the home screen button and kill the Clock App.
  4) Turn off the screen (this is another key step!).
  5) Wait on the alarm firing.

Expected:
==========
  Alarm should go off at the given time (after about 2 mins).

Actual:
==========
  Alarm doesn't go off until you manually turn on the screen.

Notes:
==========
  1) If you didn't do either step (1) or (4), the alarm can go off!
  2) I've already check Alarm API and System Message API and both of them work well. That is, window_manager.js can receive the system message from Gecko to open the Clock App exactly at the alarm firing time. There might be something wrong in the window_manager.js for opening the Clock app.
Ian, is the STR in comment#28  enough for you to id the issue?
(In reply to James Ho from comment #29)
> Ian, is the STR in comment#28  enough for you to id the issue?

Yes, it's pretty accurate.
I find out there are some judgment incorrect in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1071 ~ L1131
I will find out the root cause.
(In reply to Ian Liu [:ianliu] from comment #30)
> I find out there are some judgment incorrect in
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> window_manager.js#L1071 ~ L1131
> I will find out the root cause.

console.log() through it then. I don't see if there is a problem but you are welcome to spend some time to verify it.

From the steps given in comment 28, I would say this could be an Gecko issue, since step (1) and (4) implies only if the phone is allowed to suspend this bug will occur.

I am not sure from my (or everyone's) overnight testing, I've did (3) or not. However I can sure that I tested the Clock app with (1) and (4) and it was alright.

(In reply to Gene Lian [:gene] from comment #28)
> That is, window_manager.js can receive the system message from
> Gecko to open the Clock App exactly at the alarm firing time. 

I suspect with the condition (1) and (4), window_manager.js still works, but Gecko failed to load the app into the newly created frame before going to suspend.
Keywords: qawanted
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #31)
> I suspect with the condition (1) and (4), window_manager.js still works, but
> Gecko failed to load the app into the newly created frame before going to
> suspend.

Looks like Gene found the root cause: exact above!
Component: Gaia → General
Summary: Alarm doesn’t ring on time → Alarm doesn’t ring on time if the app is killed and phone is allowed to suspend
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #32)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #31)
> > I suspect with the condition (1) and (4), window_manager.js still works, but
> > Gecko failed to load the app into the newly created frame before going to
> > suspend.
> 
> Looks like Gene found the root cause: exact above!

It sounds very weird to me that the window_manager.js still works for receiving "open-app" chrome event when the device suspends, but it would eventually hangs at [1] where |windows.appendChild(frame)| cannot completely load the app, thus cannot handle the coming system message, thus cannot launch the attention screen, thus cannot light up the screen, until we manually turn on the screen.

I'm wondering the telephony could have the same issue since it just does the same thing like alarm, but why the telephony works?!

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L884
Same reason that bug 797798 couldn't use window?
Should be able to upload a patch for solving this later... ;)
Hi Justin,

We need to provide:

  boolean           getCpuSleepAllowed();
  void              setCpuSleepAllowed(in boolean aAllowed);

for internal use, which are going to be called by JS codes. Could you please have a review when you have a chance? Thanks! :)
Attachment #675528 - Flags: review?(justin.lebar+bug)
(In reply to Gene Lian [:gene] from comment #35)
> Should be able to upload a patch for solving this later... ;)

\o/
Assignee: iliu → clian
Hi Vivien,

We have to make sure the CPU doesn't sleep after alarm fires, so that the alarm system messages can be handled at the accurate time. If we don't do this, the Clock app cannot be launched when the device is suspending, until we manually turn on the screen to wake it up.
Attachment #675532 - Flags: review?(21)
Comment on attachment 675532 [details] [diff] [review]
Part 2, CPU Cannot Sleep when Alarm Fires

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

That explain why I didn't wake up so many times! No more excuse for not waking up at the right time

::: dom/alarm/AlarmService.jsm
@@ +266,5 @@
>      debug("_onAlarmFired()");
>  
> +    // We have to make sure the CPU doesn't sleep after alarm fires,
> +    // so that alarm messages can be handled at the accurate time.
> +    pmService.setCpuSleepAllowed(false);

nit: I would have rename pmService to powerManagerService.

Also do you need to manually call pmService.setCpuSleepAllowed(true) at some point?
Attachment #675532 - Flags: review?(21) → review+
Comment on attachment 675532 [details] [diff] [review]
Part 2, CPU Cannot Sleep when Alarm Fires

Where do you set CPU sleep allowed again?

Even if you added a corresponding pmServicesetCpuSleepAllowed(true) somewhere else, that still wouldn't be right, because that's assuming that when we're done here, the CPU may sleep.  But someone else may be holding a CPU wake lock this entire time.

Isn't the right thing to do here to acquire the CPU wake lock (making sure to release it when you're done here)?
Attachment #675532 - Flags: review-
Comment on attachment 675528 [details] [diff] [review]
Part 1, Provide {get,set}CpuSleepAllowed() for Internal Use

I don't think this is necessary if we use wake locks...
Attachment #675528 - Flags: review?(justin.lebar+bug)
Thanks Justin! Now I understand the purpose of wake lock better! I'll upload a better solution to solving this issue from the point of view of Gaia. ;)
Attached patch Patch (obsolete) — Splinter Review
Hi Vivien and Tim,

After more thoughts, I decide to fix this issue from the Gaia end. Please see the notes as below:

1. I use wake lock mechanism to request a CPU-kind wake lock, which is hooked up to the |mozPower.cpuSleepAllowed| in screen_manager.js, so that we can ensure the CPU doesn't have chances to sleep during the process of loading frame, thus handling the alarm messages on time.

2. From the points of views of Alarm API and System Message API, they cannot know whether the app loading is finished or not, so that's why I put the logic in the window_manager.js which indicates a more explicit time period of frame-loading life cycle.

3. I think this change can also improve other non-alarm system messages to be handled on time. Also, I'm guessing this fix can also solve bug 798195. ;)

Could either of you please help review this patch? Thanks a lot!
Attachment #675528 - Attachment is obsolete: true
Attachment #675532 - Attachment is obsolete: true
Attachment #676094 - Flags: review?(timdream+bugs)
Attachment #676094 - Flags: review?(21)
Comment on attachment 676094 [details] [diff] [review]
Patch

r=me (w/ semicolon fix), you don't need to ask for a+ because this is a blocking+ issue.
Attachment #676094 - Flags: review?(timdream+bugs)
Attachment #676094 - Flags: review?(21)
Attachment #676094 - Flags: review+
Attached file Patch V2 (obsolete) —
Let's discuss this issue at the github thread: https://github.com/mozilla-b2g/gaia/pull/6049
Attachment #676094 - Attachment is obsolete: true
Attachment #676529 - Flags: review?(timdream+bugs)
Attachment #676529 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 676529 [details]
Patch V2

r+ commit 9ff579a
Attachment #676529 - Flags: review?(timdream+bugs) → review+
Comment on attachment 676529 [details]
Patch V2

I don't think this is quite right (as evinced by my github comment).
Attachment #676529 - Flags: feedback?(justin.lebar+bug) → feedback-
Comment on attachment 676529 [details]
Patch V2

Remove the r+ due to concern raised by jlebar
Attachment #676529 - Flags: review+
Thanks Tim and Justin for all the inputs. I'll upload another solution which needs changes both in the Gecko and Gaia. Please stay tuned. ;)
Hi Fabrice,

Changes are trivial in this part. We need to send a message back to the parent when a message is handled, so that the Alarm API can know when to unlock the CPU wake lock. I understand the System Message API mechanism shouldn't be aware of the message type, so it's Alarm API's responsibility to deal with the message by filtering the message type.
Attachment #676529 - Attachment is obsolete: true
Attachment #676933 - Flags: review?(fabrice)
Attached patch Part 2, Alarm API (obsolete) — Splinter Review
Hi Justin and Vivien,

According to the discussion thread at [1], we grab the CPU wake lock exactly at the time point of sending alarm system message, which won't be unlocked until we get a "SystemMessageManager:HandleMessageDone" message. This strategy considers a more accurate time period for locking the CPU wake lock and it meets the main purpose of Alarm API: firing system messages *on time*!

Note that I use a set to save multiple CPU wake locks, where each of them is indexed by a unique alarm ID for a fired alarm. Also, for safety, I set a watchdog timer to avoid locking the CPU wake lock too long, because it'd exhaust the battery quickly which is very bad. This could probably happen if the app failed to launch or handle the alarm message due to any unexpected reasons.

[1] https://github.com/mozilla-b2g/gaia/pull/6049
Attachment #676934 - Flags: review?(justin.lebar+bug)
Attachment #676934 - Flags: review?(21)
Attached file Part 3, Clock App
Hi Tim and Ian,

Part 1 and Part 2 can ensure the CPU doesn't fall into sleep before the alrm message handler is fully executed. However, we need some *Clock-App-specific* codes to grab another CPU wake lock, because there are still some async codes (like indexedDB) within the alarm message handler, which can only be awared of within the Clock App.

Again, for safety, I set a watchdog timer to avoid locking the CPU wake lock too long, because it'd exhaust the battery quickly which is very bad. This could probably happen if the attention screen failed to open due to any unexpected reasons.
Attachment #676939 - Flags: review?(timdream+bugs)
Attachment #676939 - Flags: review?(iliu)
Attached patch Part 2, Alarm API (obsolete) — Splinter Review
Sorry! Fix some nits. Please see comment 52 for the summary.
Attachment #676934 - Attachment is obsolete: true
Attachment #676934 - Flags: review?(justin.lebar+bug)
Attachment #676934 - Flags: review?(21)
Attachment #676958 - Flags: review?(justin.lebar+bug)
Attachment #676958 - Flags: review?(21)
Comment on attachment 676958 [details] [diff] [review]
Part 2, Alarm API

Hooray.  Thanks, Gene.

>+  _unlockCpuWakeLock: function _unlockCpuWakeLock(aAlarmId) {
>+    let cpuWakeLock = this._cpuWakeLocks[aAlarmId];
>+    if (cpuWakeLock) {
>+      cpuWakeLock.wakeLock.unlock();
>+      cpuWakeLock.wakeLock = null;
>+      cpuWakeLock.timer.cancel();
>+      delete this._cpuWakeLocks[aAlarmId];
>+      cpuWakeLock = null;
>+    }
>+  },

Although it's not hurting anything, you don't need these:

>       cpuWakeLock.wakeLock = null;
>       cpuWakeLock.timer = null;
>       cpuWakeLock = null;

cpuWakeLock = null doesn't do anything because that variable immediately goes
out of scope.  The other two don't do anything because you've made sure the
cpuWakeLock variable is garbage, so its members will be collected by the GC.
Attachment #676958 - Flags: review?(justin.lebar+bug) → review+
Attachment #676933 - Flags: review?(fabrice) → review+
Attached patch Part 2, Alarm API, V1.1 (obsolete) — Splinter Review
r=jlebar (addressing comment 55)
r=vivien (addressing comment 39)
Attachment #676958 - Attachment is obsolete: true
Attachment #676958 - Flags: review?(21)
Attachment #677269 - Flags: review+
Keywords: checkin-needed
Whiteboard: [label:clock] → [label:clock] [Please check in Part1 & Part2]
Comment on attachment 676939 [details]
Part 3, Clock App

I agree the refinement.
r=me
Attachment #676939 - Flags: review?(iliu) → review+
Comment on attachment 676939 [details]
Part 3, Clock App

r+ the commit dcd8690 with nit addressed.
Attachment #676939 - Flags: review?(timdream+bugs) → review+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #58)
> Comment on attachment 676939 [details]
> Part 3, Clock App
> 
> r+ the commit dcd8690 with nit addressed.

Done! Could you please help merge the PR? Thanks!
try: -b do -p all -u none -t none
https://tbpl.mozilla.org/?tree=Try&rev=7d9db24ee316

try: -b d -p linux64 -u all -t none
https://tbpl.mozilla.org/?tree=Try&rev=72f6c5b5e461
r=jlebar (addressing comment 55)
r=vivien (addressing comment 39)

Also, rebased with mozilla-inbound!
Attachment #677269 - Attachment is obsolete: true
Attachment #677316 - Flags: review+
Thanks Shian-Yow!
Whiteboard: [label:clock] [Please check in Part1 & Part2] → [label:clock]
You need to log in before you can comment on or make changes to this bug.