867868 - Implement alarms for Desktop

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Gene Lian (I already quit Mozilla)]

As title. What we only need to do is to make up the missing parts in HAL. We need to support windows, linux and cocoa.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

How about using nsITimer?

Comment 2

[Justin Lebar (not reading bugmail)]

(In reply to Nikhil Marathe from comment #1)
> How about using nsITimer?

I'm fine with us using nsITimer in fallback-hal, even though it's XPCOM.

I'd like us to check that it doesn't have bad CPU-usage effects on laptops, and that it does the right thing when a machine goes to sleep and then wakes up again.  I suspect it's not a problem.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: First attempt

I think it's a very naive attempt, but it works.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: WIP tests

Just a single test for now.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix alarmHalService memory leak

breaks the cycle in profile teardown.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 744601 [details] [diff] [review]
Fix alarmHalService memory leak

Is this actually necessary?  I'd have thought that JSMs would be torn down at shutdown, and that their refs would be released.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Justin Lebar [:jlebar] from comment #6)
> Comment on attachment 744601 [details] [diff] [review]
> Fix alarmHalService memory leak
> 
> Is this actually necessary?  I'd have thought that JSMs would be torn down
> at shutdown, and that their refs would be released.
It doesn't seem to be happening. Lot's of leaks reported on shutdown by the inclusion of AlarmService and some parts of PushService. This is only one of the fixes.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix alarm service memory leaks on desktop

There were a few components causing leaks.

Closing AlarmDB and breaking the cycle of alarmDB recovered a majority of the leaks.
alarmHalService breaks the direct cycle.
Surprisingly, just using parentprocessmessagemanager causes it to leak unless set to null.

I think I've got all of them. Try running:

Without patch:
https://tbpl.mozilla.org/?tree=Try&rev=9023d1b1a8e0
With patch (ahs-memleak):
https://tbpl.mozilla.org/?tree=Try&rev=717678d5bdda

Comment 9

[Justin Lebar (not reading bugmail)]

> It doesn't seem to be happening.

Ah, I see.  It's because of cycles.

You could make the AlarmService cycle-collected, and that should fix one of your problems.  But this is fine too.

Comment 10

[Justin Lebar (not reading bugmail)]

> You could make the AlarmService cycle-collected

Sorry, this should be AlarmHalService.

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 744759 [details] [diff] [review]
Fix alarm service memory leaks on desktop

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

Nice catch!

::: dom/alarm/AlarmService.jsm
@@ +499,5 @@
> +        this.uninit();
> +    }
> +  },
> +
> +  uninit: function() {

nit: uninit: function uninit()

Since you're here, I think we also need to add:

debug("uninit()");
Services.obs.removeObserver(this, "profile-change-teardown");
this._messages.forEach(function(aMsgName) {
  ppmm.removeMessageListener(aMsgName, this);
}.bind(this));

which sounds more complete to un-initialize everything. Btw, please make |._messages| a member variable instead of a local const in init().

Btw, please help correct the alignment for the following line (my bad). Thanks!

  ppmm.addMessageListener(...)

@@ +501,5 @@
> +  },
> +
> +  uninit: function() {
> +    if (this._db)
> +      this._db.close();

nit: please add braces even for one-line if-block.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 744585 [details] [diff] [review]
First attempt

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

::: hal/fallback/FallbackAlarm.cpp
@@ +14,5 @@
> +{
> +  hal::NotifyAlarmFired();
> +}
> +
> +static nsCOMPtr<nsITimer> sTimer = NULL;

Do you really need to initialize sTimer to NULL?

@@ +15,5 @@
> +  hal::NotifyAlarmFired();
> +}
> +
> +static nsCOMPtr<nsITimer> sTimer = NULL;
> +PRIntervalTime sEpoch;

Should sEpoch be static?

@@ +21,5 @@
>  bool
>  EnableAlarm()
>  {
> +  sEpoch = PR_IntervalNow();
> +  sTimer = do_CreateInstance("@mozilla.org/timer;1");

Can EnableAlarm() be called multiple times? If not, we should MOZ_ASSERT() that sEpoch and sTimer are not already initialized.

@@ +31,5 @@
>  {
> +  if (sTimer)
> +    sTimer->Cancel();
> +
> +  sTimer = NULL;

You can hoist `mTimer = NULL` into the `if (sTimer) {}` block.

@@ +39,5 @@
>  SetAlarm(int32_t aSeconds, int32_t aNanoseconds)
>  {
> +  // aSeconds is an absolute time since the Unix epoch.
> +  // nsITimer requires the delay.
> +  sTimer->InitWithFuncCallback(TimerCallbackFunc, NULL, aSeconds * 1000 - PR_Now() / 1000, nsITimer::TYPE_ONE_SHOT);

GonkHal.cpp's SetAlarm() returns false (instead of crashing) if EnableAlarm() has not been initialized. Adding a MOZ_ASSERT() and return false is probably safer.

Comment 13

[Justin Lebar (not reading bugmail)]

> +static nsCOMPtr<nsITimer> sTimer = NULL;
>
> Do you really need to initialize sTimer to NULL?

Indeed, there are other problems.

First, it should be nullptr, not NULL.

Second, static nsCOMPtr is not allowed, because it creates static constructors, which slow down Gecko startup.  You should use |static StaticRefPtr<nsITimer>|.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 744759 [details] [diff] [review]
Fix alarm service memory leaks on desktop

This patch is now tracked in Bug 871428.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 748770 [details] [diff] [review]
Implement nsITimer based alarm.

s/NULL/nullptr

Please add a ClearOnShutdown to sTimer; I don't think it should be a leak if we don't call DisableAlarm() before shutting down.

Unfortunately the ClearOnShutdown API is a bit clunky to use here; you only want to call ClearOnShutdown once, even if EnableAlarm() is called more than once.  So you'll need a static bool, I think.  (You can make it static inside the function if you want.)  It's OK to ClearOnShutdown on a StaticRefPtr that may be null when we shut down.

> +  sTimer->InitWithFuncCallback(TimerCallbackFunc, NULL, aSeconds * 1000 - PR_Now() / 1000, nsITimer::TYPE_ONE_SHOT);

Please wrap this long line, but more importantly, don't you want to include aNanoseconds in this computation?

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

First I thought nsITimer only supports milliseconds, but I guess someone would just pass a lot of nanoseconds and zero seconds for kicks (or to have more precise milliseconds).

Comment 18

[Justin Lebar (not reading bugmail)]

> First I thought nsITimer only supports milliseconds, but I guess someone would just pass a lot of 
> nanoseconds and zero seconds for kicks (or to have more precise milliseconds).

More precise than zero, you mean!  (aNanoseconds is my only option if I want a second-and-a-half wait.)

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Is it acceptable to have an assertion for sTimer?

Added ClearOnShutdown and nanoseconds support.

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Justin,

On adding ClearOnShutdown(), the MOZ_ASSERT(sTimer) in DisableAlarm() fails sometimes. Considering ClearOnShutdown() is called after everything shutdowns, I don't know why this is happening.

AlarmHalService's destructor calls UnregisterTheOneAlarmObserver() which calls DisableAlarm(). AlarmHalService is freed by AlarmService.uninit() (by breaking the cycle).

Comment 21

[Justin Lebar (not reading bugmail)]

This calls ClearOnShutdown once for every call to EnableAlarm.  But you should only call it once ever for the given sTimer.

Something like

EnableAlarm()
{
  static bool initialized = false;
  if (!initialized) {
    initialized = true;
    ClearOnShutdown(&sTimer);
  }
  ...
}

But that won't fix your problem.

It sounds like the answer is not to have the assertions, and to add a comment indicating that we expect to get calls to DisableTimer after the ClearOnShutdown runs.

Or you could manually register a shutdown observer and ignore calls to Enable/Disable after shutdown, but otherwise assert.

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Removed assertion and manual set to nullptr.

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 751759 [details] [diff] [review]
Implement nsITimer based alarm.

This has the problem with EnableAlarm and ClearOnShutdown identified in comment 21.

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Sorry, didn't realize that MOZ_ASSERT no-ops in release builds.

Just a question. Is ClearOnShutdown() deleting the pointer itself (and not the nsITimer on the heap) because I'm calling it before sTimer has been initialized.

Comment 25

[Justin Lebar (not reading bugmail)]

I guess I wasn't clear in comment 21.  Let me try again:

For a given StaticRefPtr, you must to call ClearOnShutdown exactly once.  (It's not a correctness problem if you call it twice, but it is superfluous, and can lead to excessive memory usage.)

This code fails that test: It calls ClearOnShutdown once on sTimer for every call to EnableAlarm.

I suggested a way you could write the code to ensure that we call ClearOnShutdown only once in comment 21.

ClearOnShutdown simply sets the pointer to null when we shut down.

Comment 26

[Justin Lebar (not reading bugmail)]

Oh, sorry, I looked at the wrong patch!  The patch in comment 24 is much more like it.

Comment 27

[Justin Lebar (not reading bugmail)]

> bool
> EnableAlarm()
> {
>-  return false;
>+  static bool initialized = false;
>+  if (!initialized) {
>+      initialized = true;
>+      ClearOnShutdown(&sTimer);
>+  }

Nit: Two spaces, please.

>+  nsCOMPtr<nsITimer> timer = do_CreateInstance("@mozilla.org/timer;1");
>+  sTimer = timer.get();

This is fine if |sTimer = timer| doesn't work, but otherwise, please do that.

> void
> DisableAlarm()
> {
>+  /**
>+   * DisableAlarm() may be called after sTimer has been set to null by
>+   * ClearOnShutdown().
>+   */

The "/**" comments are "javadocs" for methods and member variables.  Inside
functions we use "/*" or "//".

> bool
> SetAlarm(int32_t aSeconds, int32_t aNanoseconds)
> {
>-  return false;
>+  MOZ_ASSERT(sTimer);
>+
>+  if (!sTimer) {
>+    HAL_LOG(("We should have enabled the alarm"));
>+    return false;
>+  }

Nit: Maybe move the MOZ_ASSERT() into here and make it a MOZ_ASSERT(false)?

>+  // Do the math to convert aSeconds and aNanoseconds into *relative*
>+  // milliseconds for nsITimer.
>+  int32_t milliseconds = aSeconds * 1000 + aNanoseconds / 1000;

Nit: We're not strictly converting to relative ms here; we do that below...

>+  sTimer->InitWithFuncCallback(TimerCallbackFunc, nullptr,
>+                               milliseconds - PR_Now() / 1000,
>+                               nsITimer::TYPE_ONE_SHOT);

Comment 28

[Justin Lebar (not reading bugmail)]

Comment on attachment 754020 [details] [diff] [review]
Implement nsITimer based alarm.

nsm pointed out a bug in this to me.

InitWithFuncCallback() takes an unsigned delay.

So if (milliseconds - PR_Now() / 1000) is negative, we'll never fire the timer.

I think the right thing to do would be to fire the timer immediately.

Comment 29

[Justin Lebar (not reading bugmail)]

Or we could return false if the time is in the past.  But because a consumer cannot guarantee that their time will not be in the past by the time this code runs, erroring out if the time is in the past seems pretty weird.

But note that erroring out is what we do in GonkHal.  Maybe we should fix that, too.

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Add sync notify alarm on past time.

Use int64_t in seconds -> ms conversion to prevent overflow.

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Tests

Comment 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Removed system messages dependency on this bug. This bug tracks the hal implementation and the ability to use AlarmService in Gecko code which is not affected by the availability of system messages. I guess title should be updated to reflect this.

Comment 33

[Justin Lebar (not reading bugmail)]

> I think the right thing to do would be to fire the timer immediately.

I guess I should have been clearer; we should spin the event loop (i.e., use NS_DispatchToMainThread); otherwise, content will be able to tell the difference.

Comment 34

[Justin Lebar (not reading bugmail)]

>+  // Do the math to convert aSeconds and aNanoseconds into milliseconds since
>+  // the epoch.
>+  int64_t milliseconds = ((int64_t)aSeconds) * 1000 + aNanoseconds / 1000;

Nit: We prefer static_cast to C-style casts.

>+  // nsITimer expects relative milliseconds.
>+  int32_t relMilliseconds = milliseconds - PR_Now() / 1000;
>+
>+  // Fire the alarm if its in the past or present.

it's

>+  if (relMilliseconds <= 0) {
>+    HAL_LOG(("In the past!"));

This log message would be pretty confusing to see in isolation...

If you wanted, you could just set

  relMilliseconds = std::max(milliseconds - PR_Now() / 1000, 0)
  
and let nsITimer take care of the rest!  That's probably the simplest thing to
do...

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Let nsITimer do the hard stuff.

Comment 36

[Justin Lebar (not reading bugmail)]

Comment on attachment 755108 [details] [diff] [review]
Implement nsITimer based alarm.

r=me, but could you please add a comment on the std::max?

Comment 37

[Justin Lebar (not reading bugmail)]

Comment on attachment 755088 [details] [diff] [review]
Tests

I think it would be good if you could decrease the timeouts here, so the test
doesn't take so long.  (Our infra is under-provisioned.)

>@@ -0,0 +1,174 @@
>+function add_alarm_future(cb) {
>+  let self = this;
>+  AlarmService.add({
>+    date: new Date(Date.now() + 5000),
>+    ignoreTimezone: true
>+  },
>+  function onAlarmFired(alarm) {
>+    ok(alarm.id == self.alarmId);
>+    cb();
>+  },
>+  function onSuccess(alarmId) {
>+    self.alarmId = alarmId;
>+  }, function onError(error) {
>+    ok(false, "Unexpected error adding alarm for 5 seconds later " + error);
>+    cb();
>+  });
>+}
>+
>+function add_alarm_present(cb) {
>+  let self = this;
>+  AlarmService.add({
>+    date: new Date(Date.now()),
>+    ignoreTimezone: true
>+  },
>+  null,
>+  function onSuccess(alarmId) {
>+    ok(alarmId);
>+    cb();
>+  }, function onError(error) {
>+    ok(false, "Alarm for now should trigger immediately");
>+    cb();
>+  });
>+}

Why don't we have an onAlarmFired listener here?  I think it would be good to
check that onSuccess fires /before/ onAlarmFired.

>+function add_alarm_past(cb) {
>+  let self = this;
>+  // test that a past alarm is fired synchronously.
>+  let fired = false;
>+  AlarmService.add({
>+    date: new Date(Date.now() - 5),
>+    ignoreTimezone: true
>+  },
>+  function onAlarmFired() {
>+    fired = true;
>+  },
>+  function onSuccess(alarmId) {
>+    ok(fired);
>+    cb();
>+  }, function onError(error) {
>+    ok(false, "Alarm for the past should trigger immediately");
>+    cb();
>+  });
>+}

onSuccess fires before onAlarmFired?  That's not good...

If that can happen with a now()-5 timeout, it can also happen with a timeout in
the future (which means that the rest of this test can go orange randomly).
:-/

>+function same_time_alarms(cb) {
>+  var fired = 0;
>+  var twoSec = new Date(Date.now() + 2000);
>+  AlarmService.add({
>+    date: twoSec,
>+    ignoreTimezone: true
>+  }, function() {
>+    fired++;
>+    AlarmService.add({
>+      date: twoSec,
>+      ignoreTimezone: true
>+    }, function() {
>+      fired++;
>+    });
>+  });
>+
>+  AlarmService.add({
>+    date: twoSec,
>+    ignoreTimezone: true
>+  }, function() {
>+    fired++;
>+  });
>+
>+  AlarmService.add({
>+    date: new Date(Date.now() + 5000),
>+    ignoreTimezone: true
>+  }, function() {
>+    fired++;
>+    ok(fired == 4);
>+    cb();
>+  });
>+}

This test isn't safe.  If the 2s alarm doesn't fire until after 5s have passed (the CPU could be busy doing other things, particularly if we run this test on a VM on EC2), then the 5s alarm will fire before the second 2s alarm has a chance to fire.

Comment 38

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I forgot to update the test patch :( Will do it tomorrow.

Comment 39

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

Carrying forward review, but I had made the same ns -> ms calculation mistake in this patch as in bug 876935.

Comment 40

[Justin Lebar (not reading bugmail)]

Hm, should relMilliseconds be 64 bits wide, too?  What if the time passed is 0, 0, for example?  It seems like maybe we should do the math with 64 bits and then fit it into the closest unsigned 32-bit int (on both sides; we're doing only one side right now with the 0).

Comment 41

[Justin Lebar (not reading bugmail)]

Sorry, I should have thought of all of this stuff when I did the initial review.

Comment 42

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Tests

Comment 43

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement nsITimer based alarm.

I hope this is ok, I'm not very good with C types.

Comment 44

[Justin Lebar (not reading bugmail)]

Comment on attachment 755512 [details] [diff] [review]
Implement nsITimer based alarm.

This needs to be clamped<int64_t>, otherwise we implicitly convert the first arg to an int32_t, which is what we're trying to avoid.

Comment 45

[Justin Lebar (not reading bugmail)]

Can we make the timeouts much shorter (like, 100ms, max)?  Or does that break
the test?

>+/*
>+ * Tests for Bug 867868 and related Alarm API bugs.
>+ *
>+ * NOTE: These tests pass the alarm time to AlarmService as a number and not as
>+ * a date. See bug 810973 about Date truncating milliseconds. AlarmService does
>+ * not officially allow a integer, but nor does it disallow it. Of course this
>+ * test will break if AlarmService adds a type check, hence this note.
>+ * FIXME: when bug 810973 is fixed.
>+ */

I don't get why we can't pass Date objects to the alarm service.  Is the alarm
service truncating the ms internally?  It doesn't look like |new
Date(Date.now() + 3000)| truncates milliseconds.

>@@ -21,14 +31,17 @@
> function add_alarm_present(cb) {
>   let self = this;
>+  let alarmId = undefined;
>   AlarmService.add({
>-    date: new Date(Date.now()),
>+    date: Date.now(),
>     ignoreTimezone: true
>   },
>-  null,
>-  function onSuccess(alarmId) {
>-    ok(alarmId);
>+  function onAlarmFired(aAlarm) {
>+    ok(alarmId && aAlarm.id === alarmId);

It's OK if alarmId == 0, right?  I think we should be checking !== undefined
here and elsewhere.

>@@ -117,14 +128,14 @@
> function same_time_alarms(cb) {
>   var fired = 0;
>-  var twoSec = new Date(Date.now() + 2000);
>+  var oneSec = Date.now() + 1000;
>   AlarmService.add({
>-    date: twoSec,
>+    date: oneSec,
>     ignoreTimezone: true
>   }, function() {
>     fired++;
>     AlarmService.add({
>-      date: twoSec,
>+      date: oneSec,
>       ignoreTimezone: true
>     }, function() {
>       fired++;
>
How does changing the timeouts here fix the race condition?

Comment 46

[Justin Lebar (not reading bugmail)]

The diffs above are an interdiff: interdiff <(curl -L https://bug867868.bugzilla.mozilla.org/attachment.cgi?id=755088) <(curl -L https://bug867868.bugzilla.mozilla.org/attachment.cgi?id=755495)

Comment 47

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Justin Lebar [:jlebar] from comment #45)
> Can we make the timeouts much shorter (like, 100ms, max)?  Or does that break
> the test?
> 
> >+/*
> >+ * Tests for Bug 867868 and related Alarm API bugs.
> >+ *
> >+ * NOTE: These tests pass the alarm time to AlarmService as a number and not as
> >+ * a date. See bug 810973 about Date truncating milliseconds. AlarmService does
> >+ * not officially allow a integer, but nor does it disallow it. Of course this
> >+ * test will break if AlarmService adds a type check, hence this note.
> >+ * FIXME: when bug 810973 is fixed.
> >+ */
> 
> I don't get why we can't pass Date objects to the alarm service.  Is the
> alarm
> service truncating the ms internally?  It doesn't look like |new
> Date(Date.now() + 3000)| truncates milliseconds.
http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#330
results in { date: new Date(Date.now() + 3000) } being wrapped in another date. Now it seems you want that line so people can pass in a string or a timestamp instead of a date, but that means it breaks when an actual date is passed.

> 
> >@@ -21,14 +31,17 @@
> > function add_alarm_present(cb) {
> >   let self = this;
> >+  let alarmId = undefined;
> >   AlarmService.add({
> >-    date: new Date(Date.now()),
> >+    date: Date.now(),
> >     ignoreTimezone: true
> >   },
> >-  null,
> >-  function onSuccess(alarmId) {
> >-    ok(alarmId);
> >+  function onAlarmFired(aAlarm) {
> >+    ok(alarmId && aAlarm.id === alarmId);
> 
> It's OK if alarmId == 0, right?  I think we should be checking !== undefined
> here and elsewhere.

Umm, I've never seen an alarm ID = 0 in all the logs ever, but I can add that check.
> 
> >@@ -117,14 +128,14 @@
> > function same_time_alarms(cb) {
> >   var fired = 0;
> >-  var twoSec = new Date(Date.now() + 2000);
> >+  var oneSec = Date.now() + 1000;
> >   AlarmService.add({
> >-    date: twoSec,
> >+    date: oneSec,
> >     ignoreTimezone: true
> >   }, function() {
> >     fired++;
> >     AlarmService.add({
> >-      date: twoSec,
> >+      date: oneSec,
> >       ignoreTimezone: true
> >     }, function() {
> >       fired++;
> >
> How does changing the timeouts here fix the race condition?

It doesn't :) I was just reducing timeouts for the test infra.

Comment 48

[Justin Lebar (not reading bugmail)]

> http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#330

I see.  I'd be happy if we blocked the test on fixing that in alarm service, but if you want to instead fix that in alarm service and then fix the test, that's fine.  I'd say it should be a simple fix, after how complex this bug has turned out to be, I don't want to jinx it!

Comment 49

[Justin Lebar (not reading bugmail)]

Comment on attachment 755495 [details] [diff] [review]
Tests

At the very least, we agree this still has races, so r- for that.

Comment 50

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Tests

Fixes race by making everything cause and effect. All I wanted to do was check that multiple alarms added for the same time get triggered.

Comment 51

[Justin Lebar (not reading bugmail)]

Comment on attachment 755988 [details] [diff] [review]
Tests

Looks good to me; just some nits left.

>diff --git a/hal/tests/browser_alarms.js b/hal/tests/browser_alarms.js
>new file mode 100644
>--- /dev/null
>+++ b/hal/tests/browser_alarms.js

>@@ -0,0 +1,183 @@
>+function add_alarm_future(cb) {
>+  let alarmId = undefined;
>+  AlarmService.add({
>+    date: Date.now() + 143,
>+    ignoreTimezone: true
>+  },
>+  function onAlarmFired(aAlarm) {
>+    ok(alarmId === aAlarm.id);

You may want a message here (the second arg to ok()) so that if the check
fails, you can tell which ok() is failing.

>+    cb();
>+  },
>+  function onSuccess(aAlarmId) {
>+    alarmId = aAlarmId;
>+  }, function onError(error) {

Nit: onError should be on a new line to be consistent with your formatting.

>+    ok(false, "Unexpected error adding alarm for 5 seconds later " + error);

Fix this message.

>+    cb();
>+  });
>+}

>+function add_alarm_present(cb) {
>+  let self = this;
>+  let alarmId = undefined;
>+  AlarmService.add({
>+    date: Date.now(),
>+    ignoreTimezone: true
>+  },
>+  function onAlarmFired(aAlarm) {
>+    ok(alarmId === aAlarm.id);

Add a message?

>+    cb();
>+  },
>+  function onSuccess(aAlarmId) {
>+    alarmId = aAlarmId;
>+  }, function onError(error) {

Newline after comma.

>+    ok(false, "Alarm for now should trigger immediately");

This isn't really the right message...

>+    cb();
>+  });
>+}
>+
>+function add_alarm_past(cb) {
>+  let self = this;
>+  let alarmId = undefined;
>+  AlarmService.add({
>+    date: Date.now() - 5,
>+    ignoreTimezone: true
>+  },
>+  function onAlarmFired(aAlarm) {
>+    ok(alarmId === aAlarm.id);

Message.

>+    cb();
>+  },
>+  function onSuccess(aAlarmId) {
>+    alarmId = aAlarmId;
>+  }, function onError(error) {

Newline.

>+    ok(false, "Alarm for the past should trigger immediately");
>+  });
>+}
>+
>+function trigger_all_alarms(cb) {
>+  let n = 10;
>+  let counter = 0;
>+  let date = Date.now() + 57;
>+  function onAlarmFired() {
>+    counter++;
>+    if (counter == n) {
>+      ok(true);

Add message maybe; if the test hangs here, it would be nice to know that.

>+function multiple_handlers(cb) {
>+  let d = Date.now() + 1000;

Can we reduce this timeout?

Comment 52

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Justin Lebar [:jlebar] from comment #48)
> > http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#330
> 
> I see.  I'd be happy if we blocked the test on fixing that in alarm service,
> but if you want to instead fix that in alarm service and then fix the test,
> that's fine.  I'd say it should be a simple fix, after how complex this bug
> has turned out to be, I don't want to jinx it!

bug 877888

Comment 53

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

implementation - https://hg.mozilla.org/integration/mozilla-inbound/rev/0949c968c398
tests - https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3ac2b99724

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0949c968c398
https://hg.mozilla.org/mozilla-central/rev/4b3ac2b99724