Closed Bug 1252303 Opened 5 years ago Closed 5 years ago

Creating an alarm should clear any existing alarms with the same name

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 verified)

VERIFIED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox48 --- verified

People

(Reporter: wbamberg, Assigned: bsilverberg)

Details

(Whiteboard: [alarms])

Attachments

(2 files)

Attached file alarms-test.xpi
In the chrome.alarms API, you give an alarm a name when you create it. But it's not obvious what happens if you create another alarm with the same name.

Since alarm names are kind of used as identifiers (in alarms.get(), for example, as well as onAlarm) it seems like duplicate names should not be allowed.

What seems to happen in Chrome is that if you create an alarm "foo", then create another alarm "foo", effectively the first "foo" is replaced with the second one.

In Firefox, though, you get 2 alarms, both called "foo".

***

I've attached an add-on to illustrate this. The add-on:

* has a browser action, that when clicked, creates a periodic alarm "alarm-test". Its period is 2 seconds.
* whenever "alarm-test" is triggered, it logs "creating alarm called 'ring!'", and creates a one-off alarm called "ring!". Its delay is 6 seconds.
* the alarm "ring!" just logs "ring!" to the console.

So in Chrome, the log output during the test something like:

creating alarm called 'ring!'
creating alarm called 'ring!'
creating alarm called 'ring!'
creating alarm called 'ring!'
creating alarm called 'ring!'

In Firefox, it's something like:

creating alarm called 'ring!'
creating alarm called 'ring!'
creating alarm called 'ring!'
ring!
creating alarm called 'ring!'
ring!
creating alarm called 'ring!'
ring!
creating alarm called 'ring!'
ring!
creating alarm called 'ring!'
ring!
Bob, do you want to take this one, since you've been working on Alarm tests recently?
Flags: needinfo?(bob.silverberg)
Summary: Calling alarms.create() twice with an alarm of the same name is handled differently Firefox, compared to Chrome → Creating an alarm should clear any existing alarms with the same name
Whiteboard: [alarms]
Sure thing, Kris
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Flags: needinfo?(bob.silverberg)
I chose to test two conditions for this bug:
1. Check that getAll() returns the expected number of alarms (i.e., creating an alarm with the same name as an existing alarm does not create a new alarm).
2. Check that the original alarm, which is replaced with an alarm with a duplicate name, is in fact cleared and therefore does not fire.

Review commit: https://reviewboard.mozilla.org/r/38215/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38215/
Attachment #8726736 - Flags: review?(kmaglione+bmo)
Iteration: --- → 47.3 - Mar 7
Flags: blocking-webextensions+
Comment on attachment 8726736 [details]
MozReview Request: Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag

https://reviewboard.mozilla.org/r/38215/#review34705

This looks good, but I think we should change the type of `alarmsMap` at this point, since we're using it more like a map than a set everywhere now.

::: toolkit/components/extensions/ext-alarms.js:98
(Diff revision 1)
> +        for (let alarm of alarmsMap.get(extension)) {

Let's change `alarmsMap` to `WeakMap[Extension -> Map[name -> Alarm]`. I don't think using a `Set` makes sense if we want names to be unique.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:303
(Diff revision 1)
> +          browser.test.assertEq(2, results.length, "exactly two alarms exist");

Maybe also test the names, just to be safe?
Attachment #8726736 - Flags: review?(kmaglione+bmo)
Iteration: 47.3 - Mar 7 → 48.1 - Mar 21
Attachment #8726736 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726736 [details]
MozReview Request: Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38215/diff/1-2/
Comment on attachment 8726736 [details]
MozReview Request: Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag

https://reviewboard.mozilla.org/r/38215/#review35185

::: toolkit/components/extensions/ext-alarms.js:126
(Diff revision 2)
> -            cleared = true;
> +          cleared = true;

`return Promise.resolve(true)` here and then `return Promise.resolve(false)` below would be simpler.

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

No need for `return true`
Attachment #8726736 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8726736 [details]
MozReview Request: Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag

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

> No need for `return true`

As you can tell I'm still trying to figure out when I need a `return` and when I don't. I take it from this that I only need to `return` something from within a `then` if I need to use that value inside the next `then`? If that's the case, would it also be true that I don't need a return at line 300:

`return browser.alarms.clear("master alarm").then(wasCleared => {`

nor at line 303:

`return browser.test.notifyPass("alarms");`?
https://reviewboard.mozilla.org/r/38215/#review35185

> As you can tell I'm still trying to figure out when I need a `return` and when I don't. I take it from this that I only need to `return` something from within a `then` if I need to use that value inside the next `then`? If that's the case, would it also be true that I don't need a return at line 300:
> 
> `return browser.alarms.clear("master alarm").then(wasCleared => {`
> 
> nor at line 303:
> 
> `return browser.test.notifyPass("alarms");`?

Yup.

The return for `notifyPass` is deceptive, I think. It makes it look like it returns a Promise, which it does not.
Comment on attachment 8726736 [details]
MozReview Request: Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38215/diff/3-4/
It seems like the try run for this is stuck on the Linux x64 builds. It's been sitting for nearly 24 hours, and another build that I pushed to try seems to have mostly completed its Linux tests. I'm going to add checkin-needed as I believe this patch is good to land.
Keywords: checkin-needed
This patch shouldn't have any platform-dependent behavior, so I think you're safe.
https://hg.mozilla.org/mozilla-central/rev/3282a189323c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I was able to reproduce the initial issue on Firefox 47.0a1 (2016-02-29) under Windows 10 64-bit.

Verified fixed using https://addons.allizom.org/en-US/firefox/addon/alarms-test/ on 48.0a1 (2016-04-07) under Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.