877888 - Alarm API loses milliseconds due to double Date conversion

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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

Calling AlarmService.add({date: new Date(timestamp), ...})
When a Date object is passed, the resulting expression is 'new Date(new Date(timestamp))', which leads to loss of precision of milliseconds due to the Date object bug 810973. This will always make an alarm that was set for < 1s in the future fire immediately.

One solution would be to typecheck that the date passed in was a Date and not bother with creating another Date out of it. Seems like a good first bug. 
Fixing this will also involve updating the tests in hal/tests if bug 867868 lands before this.

Gene, do you want to mentor? If not, feel free to mark me as the mentor.

Comment 1

[Gene Lian (I already quit Mozilla)]

Interesting topic. Thanks for catching this!

Comment 2

[Sankha Narayan Guria [:sankha]]

Attachment: patch

Comment 3

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

Comment on attachment 756493 [details] [diff] [review]
patch

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

::: dom/alarm/AlarmService.jsm
@@ +360,5 @@
>      );
>    },
>  
>    _getAlarmTime: function _getAlarmTime(aAlarm) {
> +    let alarmTime = aAlarm.date instanceof Date ? aAlarm.getTime()

aAlarm.date.getTime()?

Considering this is shooting over the 80 column width, I'd recommend using

    if (aAlarm.date instanceof Date) {
    } else {Please use 
    }

Please run the tests in dom/alarm/tests to ensure the change works.

Also add a comment about why this check is present so that no-one changes this in the future in an attempt to refactor the code.

@@ +366,5 @@
>  
>      // For an alarm specified with "ignoreTimezone",
>      // it must be fired respect to the user's timezone.
>      // Supposing an alarm was set at 7:00pm at Tokyo,
>      // it must be gone off at 7:00pm respect to Paris' 

can you remove this trailing space while you're here?

@@ +369,5 @@
>      // Supposing an alarm was set at 7:00pm at Tokyo,
>      // it must be gone off at 7:00pm respect to Paris' 
>      // local time when the user is located at Paris.
>      // We can adjust the alarm UTC time by calculating
>      // the difference of the orginal timezone and the 

this one too.

Comment 4

[Sankha Narayan Guria [:sankha]]

Attachment: patch with comments addressed

Comment 5

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

Comment on attachment 756622 [details] [diff] [review]
patch with comments addressed

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

::: dom/alarm/AlarmService.jsm
@@ +360,5 @@
>      );
>    },
>  
>    _getAlarmTime: function _getAlarmTime(aAlarm) {
> +    if (aAlarm.date instamnceof Date)

typo.

So I realized the existing tests won't work. You want to download and apply bug 867868 patches on top of this and run the mochitests in hal/tests using |./mach mochitest-browser hal/tests|

Some more style fixes. Single statement conditionals should also be wrapped in |{}|

@@ +361,5 @@
>    },
>  
>    _getAlarmTime: function _getAlarmTime(aAlarm) {
> +    if (aAlarm.date instamnceof Date)
> +      let alarmTime = aAlarm.getTime();

Again, |aAlarm.date.getTime()|

@@ +376,3 @@
>      // current timezone.
>      if (aAlarm.ignoreTimezone)
>         alarmTime += (this._currentTimezoneOffset - aAlarm.timezoneOffset) * 60000;

Please also wrap this in |{}|

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: working patch

I have addressed your comments in this patch. It also passes all tests in hal/tests

Comment 7

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

Comment on attachment 760063 [details] [diff] [review]
working patch

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

This patch looks good. Just one tiny nit.

::: dom/alarm/AlarmService.jsm
@@ +331,5 @@
>    },
>  
>    _getAlarmTime: function _getAlarmTime(aAlarm) {
> +    // avoid casting a Date object to a Date again to
> +    // save milliseconds

Start sentences with uppercase, and end with a period. The 'save milliseconds' part is ambiguous. I'd change it to 'preserve milliseconds' and also put in the bug number that causes that Date issue.

Comment 8

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

Since Bug 867868 landed before this, please update the hal/tests to use Date objects rather than timestamps. Date object is the right public API for AlarmService.

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: final patch

Comment 10

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

Comment on attachment 760502 [details] [diff] [review]
final patch

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

Please run the tests to ensure they pass. The type conversions are invalid.

|./mach mochitest-browser hal/tests|

::: hal/tests/browser_alarms.js
@@ +13,5 @@
>  
>  function add_alarm_future(cb) {
>    let alarmId = undefined;
>    AlarmService.add({
> +    date: new Date() + 143,

This will become an invalid string.

@@ +52,5 @@
>  function add_alarm_past(cb) {
>    let self = this;
>    let alarmId = undefined;
>    AlarmService.add({
> +    date: new Date() - 5,

This will work, but will not be a Date object. In addition I'm always worried about relying on JavaScript type conversions. Use
|new Date(Date.now() - 5)|

@@ +71,5 @@
>  
>  function trigger_all_alarms(cb) {
>    let n = 10;
>    let counter = 0;
> +  let date = new Date() + 57;

Again, invalid.

@@ +93,5 @@
>    }
>  }
>  
>  function multiple_handlers(cb) {
> +  let d = new Date() + 100;

invalid.

@@ +131,5 @@
>  }
>  
>  function same_time_alarms(cb) {
>    var fired = 0;
> +  var delay = new Date() + 100;

invalid.

Comment 11

[Sankha Narayan Guria [:sankha]]

Attachment: patch

Comment 12

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

Comment on attachment 770289 [details] [diff] [review]
patch

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

::: dom/apps/src/OfflineCacheInstaller.jsm
@@ +11,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["OfflineCacheInstaller"];
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

This file seems to have been changed accidentally.

::: hal/tests/browser_alarms.js
@@ +71,5 @@
>  
>  function trigger_all_alarms(cb) {
>    let n = 10;
>    let counter = 0;
> +  let date = new Date(Date.now()) + 57;

|new Date(Date.now() + 57)|

@@ +93,5 @@
>    }
>  }
>  
>  function multiple_handlers(cb) {
> +  let d = new Date(Date.now()) + 100;

Same as above.

Comment 13

[Sankha Narayan Guria [:sankha]]

Attachment: patch

Comment 14

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

Thanks for the patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/23b2e4f98591

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23b2e4f98591