1249055 - Calling alarms.getAll() throws an exception: TypeError: alarms.map is not a function

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

JavaScript error: chrome://extensions/content/ext-alarms.js, line 130: TypeError: alarms.map is not a function

This is because `alarms` [1] is a Set not an Array.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-alarms.js#130

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

Review commit: https://reviewboard.mozilla.org/r/35287/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35287/

Comment 2

[Bob Silverberg [:bsilverberg]]

Note that I also changed a bit of the boilerplate in the other tests in `test_ext_alarms.html` to match my chosen syntax for the new test, which I think is a bit cleaner.

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8720390 [details]
MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35287/diff/1-2/

Comment 4

[Bob Silverberg [:bsilverberg]]

I fixed another bug that was in `get()` and added coverage for get(), getAll() and clearAll(). I think there is room for improvement in the test. In particular, I feel like the test depends on the different methods being called in sequence, but I'm not sure that the way the test is written guarantees that will happen.

This is now ready for an initial review, Kris.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8720390 [details]
MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

https://reviewboard.mozilla.org/r/35287/#review32015

::: toolkit/components/extensions/ext-alarms.js:130
(Diff revision 2)
> -        let result = alarms.map(alarm => alarm.data);
> +        let result = Array.from(alarms).map(alarm => alarm.data);

`Array.from(alarms, alarm => alarm.data)`

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:115
(Diff revision 2)
> +    suffixes.map(suffix => {

Please use `.forEach` or a for-of loop rather than `.map`. Same below.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:116
(Diff revision 2)
> +      chrome.alarms.create(ALARM_NAME + suffix, {when: Date.now() + suffix});

0-3 milliseconds is not a lot. Some of these are going to fire before you check them when we switch to OOP extensions.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:117
(Diff revision 2)
> +      return suffix;

This isn't used.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:136
(Diff revision 2)
> +      browser.test.assertEq("undefined", typeof alarm, "non-existent alarm should be undefined");

Please compare against `undefined` rather than using `typeof`

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:140
(Diff revision 2)
> +    chrome.alarms.clearAll(wasCleared => {

We should also test `.clear()` here. I know it's tested above, but it seems strange for it to be the only API method not tested in this task.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:145
(Diff revision 2)
> +      browser.test.sendMessage("clearAll");

This should be sent from the `getAll` callback.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8720390 [details]
MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35287/diff/2-3/

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8720390 [details]
MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35287/diff/3-4/

Comment 8

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/35287/#review32015

> This should be sent from the `getAll` callback.

I nested the various tests in a way that I believe will ensure that they get executed in the correct order. I wonder if it would be better to do this with `then()` instead of all this nesting. What do you think?

Comment 9

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/35287/#review32015

> I nested the various tests in a way that I believe will ensure that they get executed in the correct order. I wonder if it would be better to do this with `then()` instead of all this nesting. What do you think?

I tried to rewrite it using `then()` but I think the alarms API isn't written in a way that allows that. It seems to insist that I pass a callback into methods like `get()` and `clear()`.

Comment 10

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/35287/#review32015

> I tried to rewrite it using `then()` but I think the alarms API isn't written in a way that allows that. It seems to insist that I pass a callback into methods like `get()` and `clear()`.

Yeah, that's going to be a problem until they're updated to use schema bindings.

Comment 11

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/35287/#review32015

> Yeah, that's going to be a problem until they're updated to use schema bindings.

I've started working on some of the other missing coverage, but it's going to depend on this patch because of the bug fixes. Do you think we can land this soon, or would you prefer that I just keep adding more coverage to this patch?

Comment 12

[Kris Maglione [:kmag]]

This is ready to land. I thought I gave it an r+, but mozreview...

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8720390 [details]
MozReview Request: Bug 1249055 - Calling alarms.getAll() throws an exception, r?kmag

https://reviewboard.mozilla.org/r/35287/#review32235

Comment 14

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f6f99d23b57

Comment 15

[Bob Silverberg [:bsilverberg]]

Thanks Kris! Pushing to try and I'll mark it for checkin once try is happy.

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/38e5c1ca1eda

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/9cf43731a81106da80a5ca3e3bd066d551666a9b
Bug 1249055: Follow-up: Fix ESLint errors. r=me

Comment 18

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/38e5c1ca1eda
https://hg.mozilla.org/mozilla-central/rev/9cf43731a811