876936 - Alarm API should immediately fire alarms set in the 'past' rather than error.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

As discussed between me and jlebar on IRC, with sub-second resolution, what qualifies as 'past' can become racy depending on system load, so that 'future/present' at the point of call of AlarmService.add may become 'past' when the alarm is actually set in the HAL backend. It would be better if HAL backends just fired such an alarm immediately and the AlarmService did not do any check for setting an alarm in the past.

Comment 1

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

Attachment: Patch including gonk fix

Gene dom/alarm please? and Justin can you review the hal code?

The HAL dispatch is being done synchronously in SetAlarm(). Should I queue it up as an event instead using NS_DispatchToMainThread()?

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 755083 [details] [diff] [review]
Patch including gonk fix

What happens if the ANDROID_ALARM_SET happens after |ts| is in the past?  It seems this is still racy.

Comment 3

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> Comment on attachment 755083 [details] [diff] [review]
> Patch including gonk fix
> 
> What happens if the ANDROID_ALARM_SET happens after |ts| is in the past?  It
> seems this is still racy.

If my recall is right, the blocking IO (.ioctl() in WaitForAlarm()) will immediately release when you're setting a |ts| in the past, so I don't think you need to call an extra |hal::NotifyAlarmFired()| to fire that. The current logic is still working.

Nikhil, could you please check that again? My recall might be wrong.

Comment 4

[Gene Lian (I already quit Mozilla)]

Comment on attachment 755083 [details] [diff] [review]
Patch including gonk fix

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

Please see my comment #3. I'm wondering you don't need these codes in the GonkHal.cpp.

::: dom/alarm/AlarmService.jsm
@@ -392,5 @@
> -      debug("Adding a alarm that has past time.");
> -      this._debugCurrentAlarm();
> -      aErrorCb("InvalidStateError");
> -      return;
> -    }

I used to have the same concern and Jonas replied:

----------
> Hi Jonas,
>
> What happened if the app attempts to add an alarm which has a past time?
>
> 1. The back-end needs to return an error event through .add.onerror. That is, fail to add it.
> 2. Add the alarm and return a valid ID through .add.onsuccess. If so, should it be immediately fired with system message (since it has already been expired)?
>
> Which one is the preferred behavior? Thanks for your confirmation in advance!

I would say 1. And set .error of the DOMRequest to an InvalidStateError DOMError.

/ Jonas
----------

I think as long as we have some codes to run, a racy issue always exists. Therefore, firing the alarm in any way when it's being added as expired might be the most consistent return. That is, we won't have random edge-cases that sometimes returning "InvalidStateError" but sometimes firing the alarm.

However, adding an expired alarm is obviously wrong (imaging you're adding an alarm to fire *2 days ago*) so returning "InvalidStateError" still makes sense from the API point of view, though.

I'd think a better solution is to keep the "InvalidStateError" sanity check and move it from AlarmService to AlarmManager to make it as close as possible to the position of calling mozAlarms.add(). In this way, the "InvalidStateError" makes much sense because we're more sure the content site is attempting to add an expired alarm.

Btw, if you really desire to remove the "InvalidStateError" sanity check, you need API peer review because it changes the API behaviours. However, as mentioned above, I'd prefer keeping that because adding expired alarms doesn't really make sense.

Comment 5

[Justin Lebar (not reading bugmail)]

I disagree with Jonas's decision.  I bet he wasn't thinking about how complicated this is.

It is impossible for any code to guarantee that its alarm will be in the future, no matter where we put the check for "the future".

So with the current design, every correct consumer of the alarm API must check onerror.  I'd guess that the vast majority of such consumers will want to fire the alarm themselves in the onerror case, because they didn't have any control over whether the alarm was actually in the future or not.

We should hide this complexity from websites.  If they care about not having alarms that fire immediately, they can check the time before they add the alarm.

I agree that it's weird that the alarm service will accept a date two days ago and fire the alarm immediately.  But the alternative -- even if we move the sanity check to as close to the entry point as possible -- is a clear race condition in our API, which is a far worse alternative.

Comment 6

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

Attachment: Patch

Remove AlarmService error condition.

Based on [1] and a simple test I did on an Unagi device, I can confirm that android alarms have the same behaviour as nsITimer of immediately firing if the time is in the past rendering the changes to Gonk unnecessary.

[1]: http://developer.android.com/reference/android/app/AlarmManager.html#set(int,%20long,%20android.app.PendingIntent), even though this applies at a higher level, that behaviour stems from the ioctl call.

Comment 7

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

I'm going to agree that it is more sensible to fire the alarm immediately when in the past. Developers usually don't intentionally put alarms for the past. Rather it is delays in processing the alarm that might cause the set to be in the past. In this case firing immediately is a good idea, because at least that much time has passed and the developer expects the alarm to fire. Also considering the precedence set by prior art, including nsITimer, kernel RTC alarms (man timer_settime) and the DOM's very own setTimeout(), this change does make sense.

I'm not aware of the steps to be taken to make changes to an API, if someone could point me to those, I can put it into motion.

Comment 8

[Justin Lebar (not reading bugmail)]

> I'm not aware of the steps to be taken to make changes to an API, if someone could point 
> me to those, I can put it into motion.

Jonas's response to my ni? above should be sufficient.  I'm a DOM peer, so I'm not sure that's even necessary, but I guess one could argue that this change requires superreview (inasmuch as we respect the sr rules at all).

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #5)
> I agree that it's weird that the alarm service will accept a date two days
> ago and fire the alarm immediately.  But the alternative -- even if we move
> the sanity check to as close to the entry point as possible -- is a clear
> race condition in our API, which is a far worse alternative.

My original thought is as long as the sanity check is close enough to the entry point (i.e. mozAlarms.add(...)), the race condition would only happen in the code path between the entry point and the sanity check.

After that check passes, all the *expired* alarms (i.e. alarms cannot be added by HAL before its firing time) must be fired as well. The users should be able to understand this phenomenon because time always passes after calling mozAlarms.add(...).

I'm convinced by Justin that moving the sanity check to else where doesn't actually solve this problem because we shouldn't allow any possibilities of race condition existing in our codes. Redefining the API behaviour might be better.

I can give review+ for the dom codes. However, we still need DOM peer's agreement since the API behaviour is going to be redefined. We also need to report this to W3C since they're following the same sanity check we proposed.

Comment 10

[Gene Lian (I already quit Mozilla)]

Comment on attachment 755411 [details] [diff] [review]
Patch

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

Nice catch! Nikhil!

Comment 11

[Justin Lebar (not reading bugmail)]

> However, we still need DOM peer's agreement since the API behaviour is going to be 
> redefined.

You have a DOM peer's agreement (mine), but I'd still like to hear what Jonas thinks.

Comment 12

[Gene Lian (I already quit Mozilla)]

Hi Nikhil,

Wait! One important thing. You also need to fix the test case at dom/alarm/test/test_alarm_add_date.html which is testing the case of adding expired alarms.

Comment 13

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

(In reply to Gene Lian [:gene] from comment #12)
> Hi Nikhil,
> 
> Wait! One important thing. You also need to fix the test case at
> dom/alarm/test/test_alarm_add_date.html which is testing the case of adding
> expired alarms.

Fixed in local patch, not bothering with update here, waiting to land.

Comment 14

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

Jonas, ping?

Comment 15

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

Not sure what I was thinking but it seems obviously better to not treat this as an error :)

So yeah, lets report a success and fire the alarm right away!

Comment 16

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

http://hg.mozilla.org/projects/birch/rev/0cd289fcea16

Comment 17

[Ed Morley [:emorley]]

Test fails:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23755157&tree=Birch

Backed out:
https://hg.mozilla.org/projects/birch/rev/e460f5af4311

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment #12... You need to fix the test case before landing.

Comment 19

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

http://hg.mozilla.org/projects/birch/rev/649ceb19e492

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/649ceb19e492

Comment 21

[Gene Lian (I already quit Mozilla)]

Report this to W3C [1] and modify the wiki documentation [2]. We also need to inform MDN of the correction [3].

[1] https://github.com/sysapps/web-alarms/issues/45
[2] https://wiki.mozilla.org/WebAPI/AlarmAPI
[3] https://developer.mozilla.org/en-US/docs/Web/API/MozAlarmsManager.add