1014386 - The alarm API accepts nonsense dates

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[reeder_29]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Executed the following code in my app:

			var addRequest = navigator.mozAlarms.add(new Date(0/0), 'ignoreTimezone', { action: "BACKUP" });
			addRequest.onsuccess = function () {
				console.log('A new alarm has been set:' + this.result);
			};
			addRequest.onerror = function () {
				console.error('operation failed: ' + this.error);
                        };


Actual results:

The onsuccess callback was called


Expected results:

The onerror callback should have been called.

Comment 1

[Gene Lian (I already quit Mozilla)]

Yeap, thanks reeder_29 for discovering this and Fabrice for bringing this to me.

This is an interesting question. I just did some tests by JS:

  var date = new Date(1); // |date| is a valid [object Date].
  alert(date.getTime()); // Will return 1.

but 

  var date = new Date(0/0); // |date| is still a valid [object Date].
  alert(date.getTime()); // But it Will return NaN.

We need to do more checks at [1] for |aDate|:

  1. If it's a Date object, check if it has a valid getTime(). If not, throws NS_ERROR_INVALID_ARG.
  2. If it's not a Date object, check it the time is a valid number. If not, throws NS_ERROR_INVALID_ARG.
  3. Otherwsie, throws NS_ERROR_INVALID_ARG.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#56

This a good-first-bug for a newbie. Does reeder_29 want to take it?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why not just define this API to take a double and then all you have to check is whether it's finite?  If someone passes a Date object it will get coerced to a double by calling its getTime(), which is what you want anyway.

Comment 3

[reeder_29]

How much of FxOS would I need to build, to test and fix this?  My old Mac mini is getting creaky.

On a related note, IMNSHO, the alarm API should also reject dates in the past (unless the system sends these as soon as it gets around to them).  Either way, the behavior should be documented.

Comment 4

[Mike Hoye [:mhoye]]

I'm not sure. Gene, can you answer this question?

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to reeder_29 from comment #3)
> How much of FxOS would I need to build, to test and fix this?  My old Mac
> mini is getting creaky.

I think you can start with mochitests on the Mac, which has been done at bug 848617.

> 
> On a related note, IMNSHO, the alarm API should also reject dates in the
> past (unless the system sends these as soon as it gets around to them). 
> Either way, the behavior should be documented.

We used to discuss this issue a lot. Please see bug 876936, comment #9. Eventually, we tend to not reject past dates. Instead, immediately fire them once expired.

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Why not just define this API to take a double and then all you have to check
> is whether it's finite?  If someone passes a Date object it will get coerced
> to a double by calling its getTime(), which is what you want anyway.

Yeap, it's more straight-forward to eat a double, but it's easier for a web developer to prepare a Data object based on a certain set of year/month/day/hour/minute/second... etc, where the absolute time is more difficult to calculate.

Comment 7

[reeder_29]

As a developer, I'm definitely going to be using a Date object to calculate the alarm time(s).  It makes more sense to have the API convert that to a double, than to require each app to do so.

FWIW, my scheduling code is

	scheduleNightly: function () {
		var alarms;
		
		if (!navigator.mozAlarms) {
			console.warn("No alarms; can't schedule backups");
			return;
		}

		var listRequest = navigator.mozAlarms.getAll();
		listRequest.onerror = function () {
			console.error('listing alarms: ' + this.error);
			var userMsg = $L("Can't schedule backup<br>") + (this.error.message || this.error.name || this.error.toString());
			enyo.Signals.send('onShowMessage', {message: userMsg});
		};
		listRequest.onsuccess = function () {
			var nightly;
			console.log('operation successful:' + this.result.length + ' alarms pending');	
			this.result.forEach(function (alarm) {
				console.log(alarm.id + ' : ' + alarm.date.toString() + ' : ' + alarm.respectTimezone);
			});
			alarms = this.result;

			// schedules nightly update, for backup
			nightly = new Date();
			nightly.setHours(4, 30, 0, 0);   // local time
			if (Date.now() >= nightly)
				nightly.setTime(nightly.valueOf() + 86400000);
			addAlarm(nightly);
			
			nightly.setTime(nightly.valueOf() + 86400000);
			addAlarm(nightly);
			
			nightly.setTime(nightly.valueOf() + 6 * 86400000);
			addAlarm(nightly);
		};
	
		function addAlarm(date) {
			console.log('scheduling for ', date.toString());
			
			for (var i=0; i<alarms.length; ++i) {
				console.log(date.toString(), alarms[i].date.toString());
				if (alarms[i].date.valueOf() === date.valueOf()) {
					console.log('already scheduled');
					return;
				}
			}

			var addRequest = navigator.mozAlarms.add(date, 'ignoreTimezone', { action: "BACKUP" });
			addRequest.onsuccess = function () {
				console.log('A new alarm has been set:' + this.result);
			};
			addRequest.onerror = function () {
				console.error('operation failed: ' + this.error);
				var userMsg = $L("Can't schedule backup<br>") + (this.error.message || this.error.name || this.error.toString());
				enyo.Signals.send('onShowMessage', {message: userMsg});
			};
		}
	},


Scheduling a repeating event using the current Alarm API requires a chain: each time an alarm happens, then next one is scheduled.  As many things can go wrong between the system firing an alarm and successfully scheduling the next, my app actually schedules three alarms: for tonight, tomorrow night, and in a week.

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5)
> (In reply to reeder_29 from comment #3)
> > How much of FxOS would I need to build, to test and fix this?  My old Mac
> > mini is getting creaky.
> 
> I think you can start with mochitests on the Mac, which has been done at bug
> 848617.

I just found all the FirefoxOS (B2G) Desktop tests have been turned off [1]. I wonder why we did so. Anyway, I think the FirefoxOS (B2G) Emulator test and other Firefox (non-B2G) Desktop tests are still working. So, you can still run Firefox Desktop Mochitest on the Mac (note what I mean is a Firefox Browser Desktop build, not a FirefoxOS Desktop build).

Vaibhav, would you please comment why we turned off the FirefoxOS (B2G) Desktop tests? Thanks!

[1] http://hg.mozilla.org/mozilla-central/diff/72ffff0fdad2/dom/alarm/test/mochitest.ini

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to reeder_29 from comment #7)
> Scheduling a repeating event using the current Alarm API requires a chain:
> each time an alarm happens, then next one is scheduled.

Yes, that's the current design. We used to think of something like Calendar API which can accept repeating alarm setting. However, we gave up because it's very hard to define how to configure the repeating behavior for the API, so we decide to keep the Alarm API as native as possible.

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #8)
> I just found all the FirefoxOS (B2G) Desktop tests have been turned off [1].
> I wonder why we did so. Anyway, I think the FirefoxOS (B2G) Emulator test
> and other Firefox (non-B2G) Desktop tests are still working. So, you can
> still run Firefox Desktop Mochitest on the Mac (note what I mean is a
> Firefox Browser Desktop build, not a FirefoxOS Desktop build).
> 
> Vaibhav, would you please comment why we turned off the FirefoxOS (B2G)
> Desktop tests? Thanks!

Quoted from Vaibhav in an off-line e-mail:

"Actually the tests in http://mxr.mozilla.org/mozilla-central/source/dom/alarm/test/mochitest.ini are failing frequently and hence they have been disabled. See bug 931116."

> 
> [1]
> http://hg.mozilla.org/mozilla-central/diff/72ffff0fdad2/dom/alarm/test/
> mochitest.ini

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but it's easier for a web developer to prepare a Data object

Sure.  My point is if the API takes a double the web developer can pass in a Date object and the right thing will happen.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #8)

> and other Firefox (non-B2G) Desktop tests are still working.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is wrong. Boris corrected me in an off-line e-mail:

Those tests self-disable on actual Firefox desktop.  For example, test_alarm_add_data.html has:

205       // Currently applicable only on FxOS
206       if (navigator.userAgent.indexOf("Mobile") != -1 &&
207           navigator.appVersion.indexOf("Android") == -1) {
208
209         testEmptyObject();
210
211       } else {
212         ok(true, "mozAlarms on Firefox OS only.");
213         SimpleTest.finish();
214       }

So, in summary, the Mochitest only works on the FirefoxOS (B2G) Emulator for now.

Comment 13

[Mark Van Peteghem [:markvp]]

Can I work on this? I have some time next weekend.

Comment 14

[Mike Hoye [:mhoye]]

Hi, Mark. I'd think so! Gene, what do you think?

Comment 15

[Gene Lian (I already quit Mozilla)]

Sure! Please go ahead! Thank you.

Comment 16

[Mark Van Peteghem [:markvp]]

Some friends stole the time I had this weekend, but I'm looking at it now.

I get the error "navigator.mozAlarms is null" when I test any sample code, in both the regular FF and the nightly build. But the docs say it is available since FF 16.

What am I doing wrong?

Comment 17

[Boris Zbarsky [:bzbarsky]]

You at least need to set the "dom.mozAlarms.enabled" preference to true.  But also there are permissions checks for the "alarms" permission and some stuff like that before the API gets enabled...

Comment 18

[Gene Lian (I already quit Mozilla)]

(In reply to Mark Van Peteghem [:markvp] from comment #16)
> Some friends stole the time I had this weekend, but I'm looking at it now.
> 
> I get the error "navigator.mozAlarms is null" when I test any sample code,
> in both the regular FF and the nightly build. But the docs say it is
> available since FF 16.
> 
> What am I doing wrong?

Sorry for the delayed response. As mentioned at comment #16, that's highly possible either the preference or the permission check makes it return null to you.

The Alarm API is supposed to be available on the following platforms/builds for now:

  - FirefoxOS Device/Emulator (for installed apps only)
  - FirefoxOS Simulator (for installed apps only)
  - Firefox Desktop (for installed apps only)

but not:

  - FirefoxOS Device/Emulator (for webpages)
  - FirefoxOS Simulator (for webpages)
  - Firefox Desktop (for webpages)
  - Firefox Android

You may find some indications and the corresponding bugs in the Alarm API test [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/alarm/test/test_alarm_add_data.html?force=1#207

Comment 19

[Gene Lian (I already quit Mozilla)]

> Sorry for the delayed response. As mentioned at comment #16, that's highly
                                                  ^^^^^^^^^^^ Sorry, I mean comment #17.

Comment 20

[Tyler Smith [:tysmith]]

Attachment: Added additional check for invalid date

Comment 21

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

Comment on attachment 8449089 [details] [diff] [review]
Added additional check for invalid date

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

r=me once the new test is added if we agree to stick only to Date objects.
Dropping review till then.

::: dom/alarm/AlarmsManager.js
@@ +40,5 @@
>        debug("Cannot add alarms for non-installed apps.");
>        throw Components.results.NS_ERROR_FAILURE;
>      }
>  
> +    if (!aDate || !(aDate instanceof Date) || !isNaN(aDate.getTime())) {

Please use Number.isNaN() on getTime()'s return value.

We do currently accept non-Date inputs and convert them to a valid Date in AlarmService.jsm:_getAlarmTime(), so I'm not sure we can add this instanceof check here. The MDN docs for the API say that add expects a Date, but I'm not sure if we'd be breaking apps silently.
If we do add this check, we should track this change in release notes and so on for FxOS v2.1(?)

Gene, Boris, what do you think?

::: dom/alarm/test/test_alarm_add_date.html
@@ +96,5 @@
> +      ok(false, "Expected an exception to be thrown for alarm with invalid date.");
> +    } catch (e) {
> +      ok(true, "Exception thrown for alarm with invalid date.");
> +    }
> +

Since the code added a |instanceof Date| check, please add a test for passing non-Date objects and see if it fails.

Comment 22

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8449089 [details] [diff] [review]
Added additional check for invalid date

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

::: dom/alarm/AlarmsManager.js
@@ +40,5 @@
>        debug("Cannot add alarms for non-installed apps.");
>        throw Components.results.NS_ERROR_FAILURE;
>      }
>  
> +    if (!aDate || !(aDate instanceof Date) || !isNaN(aDate.getTime())) {

I agree with Nikhil. For safety, we shouldn't assume the |aData| has to be a Data object at this moment. It's actually a gap between the MDN document and the real implementation. Some third-party apps might potentially call this API by feeding a real number, though. Therefore, I'd prefer checking the |aDate| by:

  1. If it's a Date object, check the return of getTime(). If it returns NaN, throws NS_ERROR_INVALID_ARG.
  2. If it's not a Date object, check if it's a valid number (i.e. > 0). If it's not valid, throws NS_ERROR_INVALID_ARG.
  3. Otherwsie, throws NS_ERROR_INVALID_ARG.

Does that make sense to you?

Comment 23

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

Tyler, do you want to finish this off quickly? Without affecting your other timelines of course.

Comment 24

[Tyler Smith [:tysmith]]

Oh!  This slipped my mind.  Yeah I'll get it done.

Comment 25

[Josh Matthews [:jdm]]

Any progress? :)

Comment 26

[akurnya]

Hi. Just getting started with contributing here. 
What prerequisites would I need to get to fixing this task. Would it be an FF nightly build?
some links would help.

Comment 27

[Josh Matthews [:jdm]]

You would need the clone and build the code from mozilla-central (https://developer.mozilla.org/en-US/docs/Simple_Firefox_build), then import the patch attached to this bug and address the review comments in comment 21 and comment 22.

Comment 28

[akurnya]

(In reply to Josh Matthews [:jdm] from comment #27)
> You would need the clone and build the code from mozilla-central
> (https://developer.mozilla.org/en-US/docs/Simple_Firefox_build), then import
> the patch attached to this bug and address the review comments in comment 21
> and comment 22.

thanks. 

From the 2 comments What I understand is:


    if ( ( aDate instanceof Date && isNaN(aDate.getTime()) ) || ( !(aDate instanceof Date) && !( aDate > 0 ) ) ) {
        throw Error(NS_ERROR_INVALID_ARG)
    }

Not sure what NS_ERROR_INVALID_ARG is.

it seems the way to throw it in the patch is:
    throw Components.results.NS_ERROR_FAILURE;

Will setup the basic FF first and check it out.

Comment 29

[Josh Matthews [:jdm]]

`throw Components.results.NS_ERROR_INVALID_ARG` is how we can throw other values besides NS_ERROR_FAILURE.

Comment 30

[Michael]

Attachment: 1014386.patch

Comment 31

[anujraajgoel]

Hi
I wish to start with this bug but I am absolutely new to OpenSource Dev. Please help me get started with this.

Comment 32

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

I don't know how relevant this bug is any more considering Firefox OS is end of lifed. That said, I no longer work at Mozilla, so I don't know the exact details. :jdm, is this still worth fixing?

Michael, if this bug is still relevant, please ask me for review again. Please only set either the review or the feedback flag on the attachment. Both are not required. On the bug itself, please set the "Need more information from..." flag and my email.

Comment 33

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

Michael, I haven't cleared the review? flag, so you only need to set the needinfo.

Comment 34

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

"Firefox OS is end of lifed" My apologies. It isn't being distributed via carriers any more.

Comment 35

[Josh Matthews [:jdm]]

My understanding is that this is still desirable. Work is continuing on 2.5 and subsequent releases.

Comment 36

[Rupshree]

I want to work on this bug.

Comment 37

[Josh Matthews [:jdm]]

Michael has already written a patch that is waiting on review, so you'll need to find something else to work on, unfortunately.

Comment 38

[[:fabrice] Fabrice Desré]

Comment on attachment 8697542 [details] [diff] [review]
1014386.patch

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

::: dom/alarm/test/test_alarm_add_date.html
@@ +84,5 @@
>        ok(true, "Exception thrown for alarm with null date.");
>      }
>  
>      // Move on to the next test.
> +    testInvalidDate()

nit: add a `;`

@@ +96,5 @@
> +       ok(false, "Expected an exception to be thrown for alarm with invalid date.");
> +     } catch (e) {
> +       ok(true, "Exception thrown for alarm with invalid date.");
> +     }
> + 

nit: trailing whitespace.
Also, please fix the indentation of the new function and of the } after testInvalidDate();

@@ +102,1 @@
>      testInvalidTimeZone()

let's add a `;` here too

Comment 39

[Matt Brubeck (:mbrubeck)]

Closing this bug because the Alarm API was removed in bug 1300884.