Closed
Bug 1014386
Opened 11 years ago
Closed 8 years ago
The alarm API accepts nonsense dates
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
INVALID
People
(Reporter: reeder_29, Unassigned, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(2 files)
2.00 KB,
patch
|
Details | Diff | Splinter Review | |
1.95 KB,
patch
|
fabrice
:
feedback+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Component: General → DOM
Product: Firefox OS → Core
Updated•11 years ago
|
Summary: Notification API accepts nonsense dates → The alarm API accepts nonsense dates
Updated•11 years ago
|
Flags: needinfo?(gene.lian)
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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.
![]() |
||
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
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•11 years ago
|
||
I'm not sure. Gene, can you answer this question?
Flags: needinfo?(gene.lian)
Comment 5•11 years ago
|
||
(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.
Flags: needinfo?(gene.lian)
Comment 6•11 years ago
|
||
(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.
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
> 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•11 years ago
|
||
(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•11 years ago
|
||
Can I work on this? I have some time next weekend.
Comment 14•11 years ago
|
||
Hi, Mark. I'd think so! Gene, what do you think?
Flags: needinfo?(gene.lian)
Comment 15•11 years ago
|
||
Sure! Please go ahead! Thank you.
Updated•11 years ago
|
Flags: needinfo?(gene.lian)
Comment 16•11 years ago
|
||
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•11 years ago
|
||
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...
Flags: needinfo?(gene)
Updated•11 years ago
|
Mentor: gene.lian
Whiteboard: [good first bug][mentor=gene] → [good first bug]
Comment 18•11 years ago
|
||
(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
Flags: needinfo?(gene)
Comment 19•11 years ago
|
||
> Sorry for the delayed response. As mentioned at comment #16, that's highly
^^^^^^^^^^^ Sorry, I mean comment #17.
Comment 20•11 years ago
|
||
Attachment #8449089 -
Flags: review?(nsm.nikhil)
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.
Attachment #8449089 -
Flags: review?(nsm.nikhil)
Flags: needinfo?(gene.lian)
Comment 22•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(gene.lian)
Assignee: nobody → tylsmith
Tyler, do you want to finish this off quickly? Without affecting your other timelines of course.
Flags: needinfo?(tylsmith)
Comment 24•10 years ago
|
||
Oh! This slipped my mind. Yeah I'll get it done.
Flags: needinfo?(tylsmith)
Assignee: tylsmith → nobody
Mentor: airpingu → nsm.nikhil
Flags: needinfo?(tylsmith)
Whiteboard: [good first bug] → [good first bug][lang=js]
Comment 26•9 years ago
|
||
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.
Flags: needinfo?(nsm.nikhil)
Comment 27•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
`throw Components.results.NS_ERROR_INVALID_ARG` is how we can throw other values besides NS_ERROR_FAILURE.
Flags: needinfo?(nsm.nikhil)
Comment 30•9 years ago
|
||
Attachment #8697542 -
Flags: review?(nsm.nikhil)
Attachment #8697542 -
Flags: feedback?(nsm.nikhil)
Comment 31•9 years ago
|
||
Hi
I wish to start with this bug but I am absolutely new to OpenSource Dev. Please help me get started with this.
Flags: needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil) → needinfo?(josh)
Michael, I haven't cleared the review? flag, so you only need to set the needinfo.
Attachment #8697542 -
Flags: feedback?(nsm.nikhil)
"Firefox OS is end of lifed" My apologies. It isn't being distributed via carriers any more.
Comment 35•9 years ago
|
||
My understanding is that this is still desirable. Work is continuing on 2.5 and subsequent releases.
Flags: needinfo?(josh)
Comment 36•9 years ago
|
||
I want to work on this bug.
Comment 37•9 years ago
|
||
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•9 years ago
|
||
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
Attachment #8697542 -
Flags: review?(nsm.nikhil) → feedback+
Comment 39•8 years ago
|
||
Closing this bug because the Alarm API was removed in bug 1300884.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•