1118272 - Alarms API does not fire at the right time with mozPower.cpuSleepAllowed on

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: App that shows the problem

The attached app sets an alarm every 30 seconds. The new alarm is set when the old alarm fires.

1. Flash Flame with v188 base image
2. Make sure navigator.mozPower.cpuSleepAllowed is true (via system app or something)
3. Start the attached app (click the button)
4. Return to homescreen, disconnect from computer and sleep the device

... wait for 10 minutes ...

5. Open the app again

Expected: a list of date/times with exactly 30 seconds between every entry
Actual:   huge gaps, sometimes an alarm fires more than 2 minutes after it was scheduled

When the CPU is not asleep (connected to USB for example) the problem does not occur.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gaps in the list due to the bug

Comment 2

[Keven Kuo [:kkuo]]

Hi! Danny,

Could you help to take a look? Thanks

--
Keven

Comment 3

[Danny Liang [:dliang]]

Hi Jan,
After check your attached alarm.js, I think the API you used is right, it's same as moz used in clock app. I traced the call flow and make sure navigator.mozAlarms.add can set rtc alarm well, so I can confirm hal is normal.
I suspected the main problem is you didn't hold wake lock when you want to do something after wake up by alarm. If no wake lock was held, system will sleep quickly and application might not finish what it want to do. You can reference below link to know how to add wake lock:
https://developer.mozilla.org/en-US/docs/Web/API/Navigator.requestWakeLock

Moreover, I'd like to install your app to do further investigation, but I cannot install it successfully on master. Should I install it on other branch? or is there any tips?

Thanks,
Danny

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Danny Liang [:dliang] from comment #3)
> Hi Jan,
> After check your attached alarm.js, I think the API you used is right, it's
> same as moz used in clock app. I traced the call flow and make sure
> navigator.mozAlarms.add can set rtc alarm well, so I can confirm hal is
> normal.
> I suspected the main problem is you didn't hold wake lock when you want to
> do something after wake up by alarm. If no wake lock was held, system will
> sleep quickly and application might not finish what it want to do. You can
> reference below link to know how to add wake lock:
> https://developer.mozilla.org/en-US/docs/Web/API/Navigator.requestWakeLock
> 
> Moreover, I'd like to install your app to do further investigation, but I
> cannot install it successfully on master. Should I install it on other
> branch? or is there any tips?
> 
> Thanks,
> Danny

Hi, in the real code there is wake locks around the wake up-code, but I'll update the attached app first thing when I'm in the office.

To install this app, use WebIDE, add packaged app, and point to the directory of the app. It's not part of Gaia. Installed just fine for me on Flame.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Alright, I added the wake locks and all seems to be well. That means there is something wrong in our implementation, I'll dive into it again.

Thanks for pointing out the obvious :-)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that the alarm should still fire at the right time, even if the device has gone to sleep. So you shouldn't need to be holding a wakelock in order to get the callback at the right time.

Of course, after you get the callback you might need to grab a wakelock to make sure that the CPU doesn't go to sleep before you can do whatever it is that you're trying to do.

Though we should probably have some smarts to make sure that we give pages a chance to take a few actions before the CPU goes to sleep...

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Reopening. We had a situation where the interval was 30 minutes and on a number of occasions the alarms API fired too late (from 1.5 hours to 3 hours). I log into localStorage whenever the alarm fires, just in case there would be something wrong with the wakelock code, but there are no entries whatsoever. Trying to reproduce on a Flame at the moment.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

s/30 minutes/10 minutes

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Reproducible with a GP Keon (built from mc / gaia master), adjust the time in the attached code to 10 minutes, install app, let the device sleep. At one point alarm stopped firing for 1h30m.

Will see whether I can repro on Flame too.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

So I think that we actually do something wrong here. We use Android's alarm handler (ANDROID_ALARM_RTC_WAKEUP) in hal/gonk/GonkHal.cpp, but on the AlarmsManager page on Android docs (http://developer.android.com/reference/android/app/AlarmManager.html) it states:

> Note: Beginning with API 19 (KITKAT) alarm delivery is inexact: 
> the OS will shift alarms in order to minimize wakeups and battery use. 
> There are new APIs to support applications which need strict delivery 
> guarantees; see setWindow(int, long, long, PendingIntent) and 
> setExact(int, long, PendingIntent). 
> Applications whose targetSdkVersion is earlier than API 19 will 
> continue to see the previous behavior in which all alarms are delivered 
> exactly when requested. 

So we cannot rely on AlarmsManager anymore for alarms to fire at the right time based on this information.

Also related, regarding the use of ANDROID_ALARM_RTC_WAKEUP, I found this StackOverflow answer: http://stackoverflow.com/questions/5938213/android-alarmmanager-rtc-wakeup-vs-elapsed-realtime-wakeup

> Using ELAPSED_REALTIME_WAKEUP with AlarmManager will rely on a 
> monotonic clock starting from boot time "and continues to tick 
> even when the CPU is in power saving modes, so is the recommend 
> basis for general purpose interval timing".

So that might also not be the best choice after all...

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

Also ni Jonas as he is all-knowing

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

My reply looks a bit weird but I think:

- Replace ALARMS_SET with ALARMS_SETEXACT

And then see if it solves it.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Another thing. I guess there is some asynchronisity between the event firing in GonkHal.cpp and the moment we receive in content process.

Would it be possible that releasing the wakelock in GonkHal (https://github.com/mozilla/gecko-dev/blob/master/hal/gonk/GonkHal.cpp#L960) happens before the wakelock is acquired in the content process and therefore the kernel decides to go to sleep in between?

That would explain why most times everything works fine and sometimes we don't receive the event...

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #13)
> Would it be possible that releasing the wakelock in GonkHal
> (https://github.com/mozilla/gecko-dev/blob/master/hal/gonk/GonkHal.cpp#L960)
> happens before the wakelock is acquired in the content process and therefore
> the kernel decides to go to sleep in between?

We had a similar issue related to the power button so that might be a possibility. I don't know that code well enough to confirm it though.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Definitely sounds like we need to fix two things here:

1. Register with Gonk a callback with an exact timing
2. Internally grab a wakelock while firing the system message so that the app has a chance to grab its own
   wakelock before we release the internal one.

Sadly Gene who implemented the Alarm API is no longer with mozilla.

Ken, could you help find someone else to fix this bug?

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Note that sending a system message should already be acquiring wake-lock should result in two wake-locks being acquired (although it may of course be too late): I did some rundown on this that could be helpful in https://bugzilla.mozilla.org/show_bug.cgi?id=1087882#c16

Specifically, we expect wakelocks to be grabbed in ContentParent::MaybeTakeCPUWakeLock:
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1431

And in the actual guts of sending a system message:
https://dxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#753

Comment 17

[Danny Liang [:dliang]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #10)
> So I think that we actually do something wrong here. We use Android's alarm
> handler (ANDROID_ALARM_RTC_WAKEUP) in hal/gonk/GonkHal.cpp, but on the
> AlarmsManager page on Android docs
> (http://developer.android.com/reference/android/app/AlarmManager.html) it
> states:
> 
> > Note: Beginning with API 19 (KITKAT) alarm delivery is inexact: 
> > the OS will shift alarms in order to minimize wakeups and battery use. 
> > There are new APIs to support applications which need strict delivery 
> > guarantees; see setWindow(int, long, long, PendingIntent) and 
> > setExact(int, long, PendingIntent). 
> > Applications whose targetSdkVersion is earlier than API 19 will 
> > continue to see the previous behavior in which all alarms are delivered 
> > exactly when requested. 
> 
> So we cannot rely on AlarmsManager anymore for alarms to fire at the right
> time based on this information.
> 
> Also related, regarding the use of ANDROID_ALARM_RTC_WAKEUP, I found this
> StackOverflow answer:
> http://stackoverflow.com/questions/5938213/android-alarmmanager-rtc-wakeup-
> vs-elapsed-realtime-wakeup
> 
> > Using ELAPSED_REALTIME_WAKEUP with AlarmManager will rely on a 
> > monotonic clock starting from boot time "and continues to tick 
> > even when the CPU is in power saving modes, so is the recommend 
> > basis for general purpose interval timing".
> 
> So that might also not be the best choice after all...

Share what I know regarding to alarm and wakelock.

Linux support ANDROID_ALARM_RTC_WAKEUP and ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP alarms, both of them can wakeup device. In gonk hal, we just have interface for ANDROID_ALARM_RTC_WAKEUP.

By your implementation as following, I think alarm should work well.

function scheduleNextEvent() {
  var request = navigator.mozAlarms.add(new Date(Date.now() + 30000), 'honorTimezone', {
    type: 'yolo'
  });

For wake lock, it's dangerous to acquire/release it in different processes, it might cause device cannot sleep and power drain quickly. But we might add more wakelocks in the flow of wakeup alarm to make sure everything will be done successfully.

Comment 18

[Ken Chang[:kenkai]]

Hi Henry,
  Please take a look at this bug.

Comment 19

[Henry Chang [:hchang]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Definitely sounds like we need to fix two things here:
> 
> 1. Register with Gonk a callback with an exact timing
> 2. Internally grab a wakelock while firing the system message so that the
> app has a chance to grab its own
>    wakelock before we release the internal one.
> 
> Sadly Gene who implemented the Alarm API is no longer with mozilla.
> 
> Ken, could you help find someone else to fix this bug?

Hi Jonas,

I am looking into this issue and trying to interpret your comments below:
(Please correct me if I am wrong)

> 1. Register with Gonk a callback with an exact timing
Did you mean we need to call [1] to in the exact time later when
a wakelock has been acquired by content?

> 2. Internally grab a wakelock while firing the system message so that the
> app has a chance to grab its own
>    wakelock before we release the internal one.

Based on comment 16, it seems we don't need to grab an additional wakelock, isn't it?

Thanks :)

[1] http://hg.mozilla.org/mozilla-central/file/6bfc0e1c4b29/hal/gonk/GonkHal.cpp#l1005

Comment 20

[Henry Chang [:hchang]]

It looks we need to synchronize the dispatch of 'alarm' system message [1]
with GonkHal::InternalUnlockCpu() [2] and rely on the alarm message handler
in content synchronously acquires a wakelock.

[1] http://hg.mozilla.org/mozilla-central/file/6bfc0e1c4b29/dom/messages/SystemMessageManager.js#l137
[2] http://hg.mozilla.org/mozilla-central/file/6bfc0e1c4b29/hal/gonk/GonkHal.cpp#l1005

Comment 21

[Gregor Wagner [:gwagner]]

[Blocking Requested - why for this release]:
We need an alarm clock we can trust.

Comment 22

[:gerard-majax]

This is also impacting users of v2.2 community builds for Open C: https://bugzilla.frenchmozilla.org/show_bug.cgi?id=598

Comment 23

[Henry Chang [:hchang]]

Reading the code and found [1] would synchronously hold a wakelock at [2]
and so the subsequent InternalUnlockCpu() shouldn't actually release.
I may need to dig further to check if there's something I am missing.

[1] http://hg.mozilla.org/mozilla-central/file/6bfc0e1c4b29/hal/gonk/GonkHal.cpp#l1001
[2] http://hg.mozilla.org/mozilla-central/file/6bfc0e1c4b29/dom/messages/SystemMessageInternal.js#l753

Comment 24

[Henry Chang [:hchang]]

Hi Jan,

I think ANDROID_ALARM_RTC_WAKEUP has nothing to do with the alarm exactness.
It's the android framework that does the trick but not the kernel.

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #10)
> So I think that we actually do something wrong here. We use Android's alarm
> handler (ANDROID_ALARM_RTC_WAKEUP) in hal/gonk/GonkHal.cpp, but on the
> AlarmsManager page on Android docs
> (http://developer.android.com/reference/android/app/AlarmManager.html) it
> states:
> 
> > Note: Beginning with API 19 (KITKAT) alarm delivery is inexact: 
> > the OS will shift alarms in order to minimize wakeups and battery use. 
> > There are new APIs to support applications which need strict delivery 
> > guarantees; see setWindow(int, long, long, PendingIntent) and 
> > setExact(int, long, PendingIntent). 
> > Applications whose targetSdkVersion is earlier than API 19 will 
> > continue to see the previous behavior in which all alarms are delivered 
> > exactly when requested. 
> 
> So we cannot rely on AlarmsManager anymore for alarms to fire at the right
> time based on this information.
> 
> Also related, regarding the use of ANDROID_ALARM_RTC_WAKEUP, I found this
> StackOverflow answer:
> http://stackoverflow.com/questions/5938213/android-alarmmanager-rtc-wakeup-
> vs-elapsed-realtime-wakeup
> 
> > Using ELAPSED_REALTIME_WAKEUP with AlarmManager will rely on a 
> > monotonic clock starting from boot time "and continues to tick 
> > even when the CPU is in power saving modes, so is the recommend 
> > basis for general purpose interval timing".
> 
> So that might also not be the best choice after all...

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

2.1+ approved.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Henry Chang [:henry] from comment #19)
> > 1. Register with Gonk a callback with an exact timing
> Did you mean we need to call [1] to in the exact time later when
> a wakelock has been acquired by content?

It appears that the Android kernel has a few different APIs for scheduling a callback which wakes the CPU up. Some of these APIs allow the kernel to adjust the time when the callback happens so that it can save battery.

So for example it might delay one callback for 5 minutes if that allows two callbacks to happen at approximately the same time, thus saving the CPU from waking up twice.

Other kernel scheduling API does not do that and instead makes sure to always wake the CPU up and call the callback exactly at the requested time.

See comment 10 as well as the links in that comment.

We want to make sure that the Alarm API *always* uses the kernel APIs which wakes the CPU up at exactly the right time. We do *not* want the battery saving versions.

I'm not sure if we use the correct callbacks or not right now. But please have a look and make sure that we use the right ones.

> > 2. Internally grab a wakelock while firing the system message so that the
> > app has a chance to grab its own
> >    wakelock before we release the internal one.
> 
> Based on comment 16, it seems we don't need to grab an additional wakelock,
> isn't it?

It appears that we are already grabbing a wakelock yes.

https://hg.mozilla.org/mozilla-central/annotate/f15ef701bdf2/dom/messages/SystemMessageInternal.js#l753

Comment 27

[Henry Chang [:hchang]]

After tracing AlarmManagerService.java [1] deeper, I still think the kernel alarm API
remains the same behavior. Instead, the framework (AlarmManagerService) reschedules
the every single alarm to batches of alarms and set alarm based on the
batches. 
 
[1] http://androidxref.com/5.0.0_r2/xref/frameworks/base/services/core/java/com/android/server/AlarmManagerService.java
(In reply to Jonas Sicking (:sicking) from comment #26)
> (In reply to Henry Chang [:henry] from comment #19)
> > > 1. Register with Gonk a callback with an exact timing
> > Did you mean we need to call [1] to in the exact time later when
> > a wakelock has been acquired by content?
> 
> It appears that the Android kernel has a few different APIs for scheduling a
> callback which wakes the CPU up. Some of these APIs allow the kernel to
> adjust the time when the callback happens so that it can save battery.
> 
> So for example it might delay one callback for 5 minutes if that allows two
> callbacks to happen at approximately the same time, thus saving the CPU from
> waking up twice.
> 
> Other kernel scheduling API does not do that and instead makes sure to
> always wake the CPU up and call the callback exactly at the requested time.
> 
> See comment 10 as well as the links in that comment.
> 
> We want to make sure that the Alarm API *always* uses the kernel APIs which
> wakes the CPU up at exactly the right time. We do *not* want the battery
> saving versions.
> 
> I'm not sure if we use the correct callbacks or not right now. But please
> have a look and make sure that we use the right ones.
> 
> > > 2. Internally grab a wakelock while firing the system message so that the
> > > app has a chance to grab its own
> > >    wakelock before we release the internal one.
> > 
> > Based on comment 16, it seems we don't need to grab an additional wakelock,
> > isn't it?
> 
> It appears that we are already grabbing a wakelock yes.
> 
> https://hg.mozilla.org/mozilla-central/annotate/f15ef701bdf2/dom/messages/
> SystemMessageInternal.js#l753

Comment 28

[Henry Chang [:hchang]]

The inaccurate alarm might be due to

"navigator.mozAlarms.add" is executed asynchronously.

See the following explanation,

navigator.mozSetMessageHandler('alarm', function() {
  // A cpu wakelock will be internally locked before calling this handler
  // so there's no worries doing all synchronous tasks here.

  var request = navigator.mozAlarms.add(...);
  // Note that at this line, the alarm may still NOT be set to GonkHal.

  request.onsuccess = function() {
    // At this line, the alarm is guaranteed to have been set to GonkHal.
  }

  // After it returns, the internal cpu wakelock would be released.
});

We can try the following modification:

navigator.mozSetMessageHandler('alarm', function() {
  var cpuWakelock = navigator.requestWakeLock('cpu');
  var request = navigator.mozAlarms.add(...);
  request.onsuccess = request.onerror = function() {
    cpuWakelock.unlock();
  };
});

Later I will post some experimental result to support my arguments
but I think we can give it a quick try first. Myself is also running 
the modified code and it performs very well until now.

Jan,

You can also give a try if you get some chance. Just hold a cpu wakelock
until DOMRequest.onsuccess.

Thanks :)

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Henry, I don't think that can be the problem. If the code in question is:


function scheduleNextEvent() {
  var request = navigator.mozAlarms.add(new Date(Date.now() + 30000), 'honorTimezone', {
    type: 'yolo'
  });

And scheduleNextEvent is called synchronously from the system message handler, then there should already be a wakelock that's active since we hold a wakelock while the system message handler is running.

Comment 30

[Henry Chang [:hchang]]

Hi Jonas,

|scheduleNextEvent| could be executed safely but it doesn't mean GonkHal::SetAlarm
would be called before |scheduleNextEvent| returns. The implementation of
|navigator.mozAlarms.add| is a async call [1]. When |navigator.mozAlarms.add|
returns the only things we guaranteed is |this._cpmm.sendAsyncMessage|
[2] has been called. GonkHal::SetAlarm will be then called asynchronous 
in AlarmService.js [3] which is running in b2g process.

[1] http://hg.mozilla.org/mozilla-central/file/43fb1f92e8d4/dom/alarm/AlarmsManager.js#l36
[2] http://hg.mozilla.org/mozilla-central/file/43fb1f92e8d4/dom/alarm/AlarmsManager.js#l74
[3] http://hg.mozilla.org/mozilla-central/file/43fb1f92e8d4/dom/alarm/AlarmService.jsm#l98

(In reply to Jonas Sicking (:sicking) from comment #29)
> Henry, I don't think that can be the problem. If the code in question is:
> 
> 
> function scheduleNextEvent() {
>   var request = navigator.mozAlarms.add(new Date(Date.now() + 30000),
> 'honorTimezone', {
>     type: 'yolo'
>   });
> 
> And scheduleNextEvent is called synchronously from the system message
> handler, then there should already be a wakelock that's active since we hold
> a wakelock while the system message handler is running.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Interesting. So possibly we should make the alarm API hold a wakelock while it is sending the message to the parent to record the alarm.

But I agree a good way to test this theory would be make the application hold a wakelock like you suggest in comment 28

Comment 32

[Henry Chang [:hchang]]

Attachment: Patch to log all ALARM_SET and ALARM_WAIT

Comment 33

[Henry Chang [:hchang]]

Attachment: alarms with cpu lock until onsuccess

Comment 34

[Henry Chang [:hchang]]

Attachment: alarms with no cpu lock

Comment 35

[Henry Chang [:hchang]]

Running experiment for a whole day, it seems the kernel alarm is
accurate and the drift is due to the newly scheduled alarm not being 
added before cpu goes to sleep.

My testing environment is

1) make reset-gaia and clean alarm database
2) Turn on airplane mode to let the device go to sleep mode as often as possible
3) Run the script below [1] with |ADD_ALARM_WITH_CPU_LOCK| set to 0 and 1 respectively
4) Log "alarm set" and "alarm waited" timestamp in GonkHal.cpp (attachment 8574369 [details] [diff] [review])

attachment 8574370 [details] is the log with ADD_ALARM_WITH_CPU_LOCK = 1
attachment 8574371 [details] is the log with ADD_ALARM_WITH_CPU_LOCK = 0

The last two columns are "now" and the "scheduled alarm time from now" (millisecond for both).

When ADD_ALARM_WITH_CPU_LOCK is 1, everything works perfectly.
When ADD_ALARM_WITH_CPU_LOCK is 0, take the following log for example,

----------------------------------------------------------------------
ALARM_SET    1425612863064  499896
ALARM_WAITed 1425613362960         // Accurate alarm time
ALARM_SET    1425613560867  302113 // New alarm is not added right after the previous one was fired.
                                   // The 200 seconds difference is caused by the unprotected async call
                                   // that I mentioned in comment 30
ALARM_WAITed 1425613863686         // Since the kernel alarm is accurate still, the alarm is fired on time.

.
.
.
.

ALARM_SET    1425616363971  499828
ALARM_WAITed 1425616863799
ALARM_SET    1425620795810  -3432002 // The new alarm is too late to be scheduled in the future.
                                     // So we get a negative interval.
ALARM_WAITed 1425620795810
----------------------------------------------------------------------------------

Maybe we need to add a cpu wakelock to mozAlarm.add or document the alarm API
to let the user know this fact. 

[1]

var ADD_ALARM_WITH_CPU_LOCK = 1;

scheduleNextEvent();

function scheduleNextEvent() {
  var cpuWakeLock;
  if (ADD_ALARM_WITH_CPU_LOCK) {
    cpuWakeLock = navigator.requestWakeLock('cpu');
  }

  var request = navigator.mozAlarms.add(new Date(Date.now() + 500 * 1000), 'honorTimezone', {
    type: 'yolo'
  });

  request.onsuccess = function() {
    if (ADD_ALARM_WITH_CPU_LOCK) {
      cpuWakeLock.unlock();
    }
  };

  request.onerror = function() {
    console.error('Scheduling next alarm failed');
    cpuWakeLock.unlock();
  };
}

navigator.mozSetMessageHandler('alarm', function() {
  scheduleNextEvent();
});

Comment 36

[Henry Chang [:hchang]]

Attachment: Bug1118272.patch

Comment 37

[Henry Chang [:hchang]]

Attachment: Bug1118272.patch

Comment 38

[Henry Chang [:hchang]]

Comment on attachment 8574493 [details] [diff] [review]
Bug1118272.patch

Hi Jonas,

I attached a patch to hold a wakelock until the request for adding alarm replies. By the way, I am thinking if we should add a wakelock for removing a alarm as well.
I tend to 'yes' because removing a alarm is also as time critical as adding a alarm. What do you think? 

Thanks!

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think a wakelock is really needed for when an alarm is removed. I agree that there's a risk that we wake up the CPU unnecessarily, but that risk should be very small, and at worst it means that we waste some battery.

However we should make sure that once an alarm is removed, that we guarantee not to fire it. Even if we're racing between the code that's firing the alarm and the code that's removing the alarm. But a wakelock would not help with that since that can happen even if the CPU never goes to sleep.

Comment 40

[Henry Chang [:hchang]]

(In reply to Jonas Sicking (:sicking) from comment #39)
> However we should make sure that once an alarm is removed, that we guarantee
> not to fire it. Even if we're racing between the code that's firing the
> alarm and the code that's removing the alarm. But a wakelock would not help
> with that since that can happen even if the CPU never goes to sleep.
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Totally agree! So I'll focus the mozAlarm.add in the bug. Thanks!

Comment 41

[Henry Chang [:hchang]]

Hi Jonas,

Not sure if you are available to review the patch at the moment :)
Thanks!

Comment 42

[Henry Chang [:hchang]]

Comment on attachment 8574493 [details] [diff] [review]
Bug1118272.patch

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

Hi Fabrice,

Since we have a feature complete deadline at 4/6 for v2.1, could you please
also help review this patch? The patch adds a wakelock in the implementation
of MozAlarm.add to avoid entering sleep mode before GonkHal::SetCpuSleepAllowed
is actually called.

Thanks!

Comment 43

[[:fabrice] Fabrice Desré]

Comment on attachment 8574493 [details] [diff] [review]
Bug1118272.patch

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

::: dom/alarm/AlarmsManager.js
@@ +182,5 @@
>      debug("uninit()");
>    },
> +
> +  // A <requestId, {cpuLock, timer}> dictionary.
> +  _cpuLockDict: {},

nit: use a Map() instead.

Comment 44

[Henry Chang [:hchang]]

Attachment: Bug1118272_v2 (use Map).patch

Comment 45

[[:fabrice] Fabrice Desré]

Comment on attachment 8585156 [details] [diff] [review]
Bug1118272_v2 (use Map).patch

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

::: dom/alarm/AlarmsManager.js
@@ +26,4 @@
>  function AlarmsManager() {
>    debug("Constructor");
> +
> +  // A <requestId, {cpuLock, timer}> dictionary.

nit: s/dictionary/map

Comment 46

[[:fabrice] Fabrice Desré]

One last thing: the 30s timeout may be a bit long. Can you experiment with shorter ones?

Comment 47

[Henry Chang [:hchang]]

(In reply to Fabrice Desré [:fabrice] from comment #46)
> One last thing: the 30s timeout may be a bit long. Can you experiment with
> shorter ones?

Thanks Fabrice! I'll find a proper one for this value. Thanks!

Comment 48

[Henry Chang [:hchang]]

Attachment: patch v3 (r+'d by Fabrice)(for checkin-needed)

Comment 49

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5770a232c07

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c656feac7f

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e7c656feac7f

Comment 52

[Henry Chang [:hchang]]

Comment on attachment 8586637 [details] [diff] [review]
patch v3 (r+'d by Fabrice)(for checkin-needed)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  alarm API
User impact if declined: Alarm API would be inaccurate
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: No

Comment 53

[Henry Chang [:hchang]]

Comment on attachment 8586637 [details] [diff] [review]
patch v3 (r+'d by Fabrice)(for checkin-needed)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Alarm API
User impact if declined: Alarm API would be inaccurate
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: No

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a27de571e046
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/eedf7cc0a062

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/eedf7cc0a062