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

RESOLVED FIXED in Firefox 40, Firefox OS v2.1

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: janjongboom, Assigned: hchang)

Tracking

({verifyme})

unspecified
mozilla40
ARM
Gonk (Firefox OS)
verifyme
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

Created attachment 8544616 [details]
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.
(Reporter)

Updated

3 years ago
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Created attachment 8544617 [details]
Gaps in the list due to the bug

Comment 2

3 years ago
Hi! Danny,

Could you help to take a look? Thanks

--
Keven
Flags: needinfo?(dliang)

Updated

3 years ago
See Also: → bug 1086598
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
Flags: needinfo?(dliang) → needinfo?(janjongboom)
(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.
Flags: needinfo?(dliang)
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 :-)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(janjongboom)
Flags: needinfo?(dliang)
Resolution: --- → WORKSFORME
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...
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
s/30 minutes/10 minutes
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.
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...
Flags: needinfo?(dliang)
Also ni Jonas as he is all-knowing
Flags: needinfo?(jonas)
My reply looks a bit weird but I think:

- Replace ALARMS_SET with ALARMS_SETEXACT

And then see if it solves it.
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...
(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.
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?
Flags: needinfo?(jonas) → needinfo?(kchang)
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
(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.
Flags: needinfo?(dliang)

Comment 18

3 years ago
Hi Henry,
  Please take a look at this bug.
Flags: needinfo?(kchang)

Updated

3 years ago
Flags: needinfo?(hchang)
(Assignee)

Comment 19

3 years ago
(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
Flags: needinfo?(hchang)
(Assignee)

Updated

3 years ago
Assignee: nobody → hchang
(Assignee)

Updated

3 years ago
Flags: needinfo?(jonas)
(Assignee)

Comment 20

3 years ago
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
blocking-b2g: --- → 2.2?
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
[Blocking Requested - why for this release]:
We need an alarm clock we can trust.
blocking-b2g: 2.2? → 2.1?
This is also impacting users of v2.2 community builds for Open C: https://bugzilla.frenchmozilla.org/show_bug.cgi?id=598
(Assignee)

Comment 23

3 years ago
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
(Assignee)

Comment 24

3 years ago
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...
2.1+ approved.
blocking-b2g: 2.1? → 2.1+
(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
Flags: needinfo?(jonas)
(Assignee)

Comment 27

3 years ago
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
(Assignee)

Comment 28

3 years ago
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 :)
Flags: needinfo?(janjongboom)
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.
(Assignee)

Comment 30

3 years ago
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.
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
(Assignee)

Comment 32

3 years ago
Created attachment 8574369 [details] [diff] [review]
Patch to log all ALARM_SET and ALARM_WAIT
(Assignee)

Comment 33

3 years ago
Created attachment 8574370 [details]
alarms with cpu lock until onsuccess
(Assignee)

Comment 34

3 years ago
Created attachment 8574371 [details]
alarms with no cpu lock
(Assignee)

Comment 35

3 years ago
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();
});
(Assignee)

Comment 37

3 years ago
Created attachment 8574493 [details] [diff] [review]
Bug1118272.patch
Attachment #8574492 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
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!
Attachment #8574493 - Flags: review?(jonas)
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.
(Assignee)

Comment 40

3 years ago
(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!
(Reporter)

Updated

3 years ago
Flags: needinfo?(janjongboom)
(Assignee)

Comment 41

3 years ago
Hi Jonas,

Not sure if you are available to review the patch at the moment :)
Thanks!
Flags: needinfo?(jonas)
(Assignee)

Comment 42

3 years ago
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!
Attachment #8574493 - Flags: review?(fabrice)
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.
Attachment #8574493 - Flags: review?(fabrice) → feedback+
(Assignee)

Comment 44

3 years ago
Created attachment 8585156 [details] [diff] [review]
Bug1118272_v2 (use Map).patch
Attachment #8585156 - Flags: review?(fabrice)
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
Attachment #8585156 - Flags: review?(fabrice) → review+
One last thing: the 30s timeout may be a bit long. Can you experiment with shorter ones?
(Assignee)

Comment 47

3 years ago
(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!
(Assignee)

Comment 48

3 years ago
Created attachment 8586637 [details] [diff] [review]
patch v3 (r+'d by Fabrice)(for checkin-needed)
(Assignee)

Updated

3 years ago
Attachment #8586637 - Attachment description: patch v3 (r+'d by Fabrice) → patch v3 (r+'d by Fabrice)(for checkin-needed)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7c656feac7f
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 52

3 years ago
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
Attachment #8586637 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 53

3 years ago
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
Attachment #8586637 - Flags: approval-mozilla-b2g34?

Updated

3 years ago
Attachment #8586637 - Flags: approval-mozilla-b2g37?
Attachment #8586637 - Flags: approval-mozilla-b2g37+
Attachment #8586637 - Flags: approval-mozilla-b2g34?
Attachment #8586637 - Flags: approval-mozilla-b2g34+

Updated

3 years ago
Keywords: verifyme
Flags: needinfo?(jonas)
Attachment #8574493 - Flags: review?(jonas)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a27de571e046
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/eedf7cc0a062
status-b2g-v2.1: affected → fixed
status-b2g-v2.2: affected → fixed
status-b2g-master: affected → fixed
status-firefox38: --- → wontfix
status-firefox39: --- → wontfix
You need to log in before you can comment on or make changes to this bug.