879308 - null date to mozAlarms.add results in callback error rather than exception.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

mozAlarms.add(null, ...)

The check currently happens in AlarmService and bubbles up into the onerror callback, whereas

mozAlarms.add(null, "footimezone" /* invalid timezone value */)

results in an immediate NOT_IMPLEMENTED error. This should be standardized. Convention seems to be that invalid arguments which can be 'validated' at point of call throw exceptions rather than perform any async error notification.

Fix should:
1) The date typecheck should be moved to AlarmsManager and fail with NS_ERROR_INVALID_ARG.
2) bad timezone value should fail in INVALID_ARG and not in NOT_IMPLEMENTED.

Comment 1

[Mina Almasry]

Hi this sounds like something I can do. I'd like to work on it. Can you please point out where in the source tree are the methods you want me to change. Where is AlarmsManager defined?

Assign?

Comment 2

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

Mina,

https://mxr.mozilla.org is a good way to find where code is located. In this case its in dom/alarm.

Good luck!

Comment 3

[Mina Almasry]

Attachment: Patch

Here is a patch. I've also changed the tests to reflect the new behavior. But I couldn't run the tests though as I don't have Firefox OS. Not sure how to do that. If you want me to, just let me know how to do it.

I also have a couple of lines with a whitespace diff :( I think it's because my editor autodeletes trailing whitespace and it's in the same hunk as actual changes so I didn't want to change it in the patch.

Comment 4

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

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

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

Can you attach the patch again, the review tool isn't processing it correctly. Most things look good, but there seems to be some trailing whitespace in AlarmsManager.

Comment 5

[Mina Almasry]

Attachment: Patch v2

Yes, I was getting fancy editing my patches and they seem to be broken. I'm trying again.

Comment 6

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

Comment on attachment 771937 [details] [diff] [review]
Patch v2

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

The core logic is fine, but tests are broken and some formatting can be improved.

::: dom/alarm/AlarmService.jsm
@@ -403,5 @@
> -    if (!aNewAlarm.date) {
> -      aErrorCb("alarm.date is null");
> -      return;
> -    }
> -

I think you still want to keep this, since direct users of AlarmService from chrome code might make a mistake.

::: dom/alarm/AlarmsManager.js
@@ +67,5 @@
>      }
>  
> +    if (!aDate) {
> +      throw Components.results.NS_ERROR_INVALID_ARG;
> +    }

Move this check above the timezone check to keep it in line with the order of arguments.

@@ +73,3 @@
>      let request = this.createRequest();
>      this._cpmm.sendAsyncMessage(
>        "AlarmsManager:Add", 

Could you also remove this trailing whitespace?

::: dom/alarm/test/test_alarm_add_date.html
@@ +68,1 @@
>      var domRequest;

There is no need to introduce the domRequest variable, since it isn't used anywhere.

@@ +77,3 @@
>  
> +  function testInvalidTimeZone() {
> +    var domRequest;

You can get rid of this one too.

@@ +95,5 @@
>            navigator.appVersion.indexOf("Android") == -1) {
>  
>          testFutureDate();
> +        testNullDate();
> +        testInvaldTimeZone()

typo

@@ +96,5 @@
>  
>          testFutureDate();
> +        testNullDate();
> +        testInvaldTimeZone()
> +        SimpleTest.finish();

This won't work. The tests are async, which means SimpleTest.finish() will be called before the 3 test functions DOMRequests finish. You'll have to go back to the old style of calling the next function in the onsuccess of the first.

Comment 7

[Mina Almasry]

Attachment: patch v3

Yeah it's hard to figure out your tests work just by staring at them :( Anyway here is a patch with the changes you asked for.

Comment 8

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

(In reply to Mina Almasry from comment #7)
> Created attachment 772189 [details] [diff] [review]
> patch v3
> 
> Yeah it's hard to figure out your tests work just by staring at them :(
> Anyway here is a patch with the changes you asked for.

You can run the tests locally using |./mach mochitest-plain dom/alarm/test|

Comment 9

[Mina Almasry]

But I don't have firefox OS so when I do that the tests don't run, I only get "mozalarms on firefox OS only". I'm not sure how to run those tests from my mozilla-central build.

Comment 10

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

Comment on attachment 772189 [details] [diff] [review]
patch v3

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

r=nsm with following changes.

::: dom/alarm/test/test_alarm_add_date.html
@@ +57,5 @@
>      try {
>        domRequest = navigator.mozAlarms.add(yesterday, "honorTimezone", {});
>      } catch (e) {
>        ok(false,
>           "Unexpected exception trying to add alarm for yesterday.");

Should go to the next test even if this test fails.

@@ +84,2 @@
>      } catch(e) {
> +      // Exception thrown. Test passes

Remove this comment.

@@ +105,5 @@
>        if (navigator.userAgent.indexOf("Mobile") != -1 &&
> +          navigator.appVersion.indexOf("Android") == -1)
> +      {
> +        testDate();
> +        testInvalidTimeZone()

testInvalidTimeZone() should be called by testNullDate()

Comment 11

[Mina Almasry]

Attachment: patch v4

- The current set of tests work fine, because that last testNullDate isn't async.
- The comment you wanted removed: I added it because there was one just like it in the same file.

Anyway, here is another patch.

Comment 12

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

Comment on attachment 772278 [details] [diff] [review]
patch v4

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

Almost there.

::: dom/alarm/test/test_alarm_add_date.html
@@ +15,5 @@
>    "use strict";
>  
> +  // Verify passing future, past, null date works correctly, and that passing an
> +  // invalid time zone raises an exception.
> +  function testDate() {

Nit: Keep the function name and description as before, since this test itself only tests future date.

@@ +48,5 @@
>      };
>  
>    }
>  
> +  // Verify passing a Date that's already past doesn't fail (it should fire immediately).

Nit: Please wrap to 80 lines.

@@ +109,5 @@
>        if (navigator.userAgent.indexOf("Mobile") != -1 &&
> +          navigator.appVersion.indexOf("Android") == -1)
> +      {
> +        testDate();
> +        SimpleTest.finish();

Bug: SimpleTest.finish() should be called by testInvalidTimezone()

Comment 13

[Mina Almasry]

Attachment: patch

No problem. I'm right behind you chief.

Comment 14

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

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

Jonas,

This changes API behaviour (although undocumented). sr please.

Comment 15

[Mina Almasry]

Jonas, 

Just a friendly reminder this is still waiting for super review. Thanks!

Comment 16

[Mounir Lamouri (:mounir)]

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

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

The API change looks good. Thanks Mina and sorry for the late review, I am a bit behind my reviews lately.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/49939eb7372e

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49939eb7372e