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.

How about using nsITimer?

(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.

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

Just a single test for now.

breaks the cycle in profile teardown.

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.

(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.

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

> 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.

> You could make the AlarmService cycle-collected Sorry, this should be AlarmHalService.

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 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.

> +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 on attachment 744759 [details] [diff] [review] Fix alarm service memory leaks on desktop This patch is now tracked in Bug 871428.

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?

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).

> 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.)

Is it acceptable to have an assertion for sTimer? Added ClearOnShutdown and nanoseconds support.

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).

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.

Removed assertion and manual set to nullptr.

Comment on attachment 751759 [details] [diff] [review] Implement nsITimer based alarm. This has the problem with EnableAlarm and ClearOnShutdown identified in comment 21.

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.

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.

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

> 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 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.

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.

Add sync notify alarm on past time. Use int64_t in seconds -> ms conversion to prevent overflow.

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.

> 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.

>+ // 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...

Let nsITimer do the hard stuff.

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 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.

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

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

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).

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

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

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.

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?

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)

(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.

> 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 on attachment 755495 [details] [diff] [review] Tests At the very least, we agree this still has races, so r- for that.

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 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?

(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

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

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