Alarm API loses milliseconds due to double Date conversion

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nsm, Assigned: sankha)

Tracking

unspecified
mozilla25
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=nsm])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Interesting topic. Thanks for catching this!
Whiteboard: [good first bug][mentor=nikhil]
Whiteboard: [good first bug][mentor=nikhil] → [good first bug][mentor=nsm]
(Assignee)

Comment 2

5 years ago
Created attachment 756493 [details] [diff] [review]
patch
Assignee: nobody → sankha93
Attachment #756493 - Flags: review?(nsm.nikhil)
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.
Attachment #756493 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 756622 [details] [diff] [review]
patch with comments addressed
Attachment #756493 - Attachment is obsolete: true
Attachment #756622 - Flags: review?(nsm.nikhil)
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 |{}|
Attachment #756622 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 760063 [details] [diff] [review]
working patch

I have addressed your comments in this patch. It also passes all tests in hal/tests
Attachment #756622 - Attachment is obsolete: true
Attachment #760063 - Flags: review?(nsm.nikhil)
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.
Attachment #760063 - Flags: review?(nsm.nikhil) → review+
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.
(Assignee)

Comment 9

5 years ago
Created attachment 760502 [details] [diff] [review]
final patch
Attachment #760063 - Attachment is obsolete: true
Attachment #760502 - Flags: checkin+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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.
Attachment #760502 - Flags: checkin+ → review-
(Assignee)

Comment 11

5 years ago
Created attachment 770289 [details] [diff] [review]
patch
Attachment #760502 - Attachment is obsolete: true
Attachment #770289 - Flags: review?(nsm.nikhil)
Keywords: checkin-needed
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.
Attachment #770289 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 770311 [details] [diff] [review]
patch
Attachment #770289 - Attachment is obsolete: true
Attachment #770311 - Flags: review?(nsm.nikhil)
Attachment #770311 - Flags: review?(nsm.nikhil) → review+
https://hg.mozilla.org/mozilla-central/rev/23b2e4f98591
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 749551
No longer depends on: 749551
You need to log in before you can comment on or make changes to this bug.