Alarms fizzle if when is in the past

RESOLVED FIXED in Firefox 64

Status

defect
P5
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: freaktechnik, Assigned: spiro, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla64
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 months ago
STR:
 - create a new alarm, with a when that is in the past

Expected result:
Creation fails/is rejected, or potentially if a period is specified the when is just ignored.

Actual result:
Alarm is created and nothing ever happens.

Updated

9 months ago
Keywords: good-first-bug
Priority: -- → P5
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: :rpl
Mentor: lgreco
(Reporter)

Comment 2

9 months ago
I have since tested how chrome behaves:

Alarms that have a when or delayInMinutes that is in the past are fired immediately. The info object passed to the onAlarm listener contains the time set in created, no adjustments to reflect reality are done.
(Assignee)

Comment 3

8 months ago
I am attempting this bug :)
Assignee: nobody → sharma.divyansh.501
(Assignee)

Comment 4

8 months ago
Just a check - Do I have to modify this file https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/parent/ext-alarms.js ?
Flags: needinfo?(lgreco)

Comment 5

8 months ago
(In reply to Divyansh Sharma [:spiro] from comment #4)
> Just a check - Do I have to modify this file
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/
> parent/ext-alarms.js ?

Hi :spiro,
Yes, you are definitely on the right track!

In particular, the `Alarm` constructor defined in the first part of that file is likely to be one of the pieces that are going to be changed to fix this bug:
when `alarmInfo.when` is in the past, the `delay` computed in the `Alarm` constructor function is going to become a negative number and the alarm will never be fired.

About the additional tests that this fix is going to need, the alarm API doesn't depend on any UI and so it is completely tested by the xpcshell unit tests, in particular you should take a look in the following 4 test files (and run them locally to double-check that your changes are not breaking any of the existing tests):

- https://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=test_ext_alarms

To run an xpcshell test [1]:

./mach xpcshell-test toolkit/components/extensions/test/xpcshell/test_ext_alarms.js

after you have built Firefox using the artifacts builds (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds, the artifact builds allow to avoid rebuilding Firefox from scratch when the changes applied doesn't need a full build like in this case).

You may need to take a look to the following MDN doc page to learn more about the xpcshell tests:

- https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

(also, reading and playing a bit with them locally helps a lot on growing a first-hand experience on the test suites used in mozilla-central, e.g. by applying some changes to the existing tests for the API you are working on and see how the tests react to your changes).

Let me know how it proceeds (especially if there are issues on which I can help you with, or if you have some initial draft patch and you would like me to take an initial look at).
Flags: needinfo?(lgreco)
(Assignee)

Comment 6

8 months ago
Posted file Bug 1478694 Alarm extension (obsolete) —
Bug 1478694 - Alarm ignored if when is in past.
Attachment #9008036 - Attachment description: user: divyansh <sharma.divyansh.501@iitg.ernet.in> branch 'default' changed toolkit/components/extensions/parent/ext-alarms.js → Bug 1478694 Alarm extension
Flags: needinfo?(lgreco)

Comment 7

7 months ago
I'm clearing the needinfo, as the attached patch is being discussed on phabricator (last round of review comments from phabricator: https://phabricator.services.mozilla.com/D5520#122868, unfortunately the activity on phabricator isn't reflected into bugzilla, as it used to be for the mozreview comments).

Nevertheless, :spiro, if you have any doubts or questions about the last review comments on phabricator, or issues that I can help you with, feel free to add back a needinfo assigned to me.
Flags: needinfo?(lgreco)
(Assignee)

Comment 8

7 months ago
Bug 1478694 - Alarm fires immediately if when is in past.
(Assignee)

Comment 9

7 months ago
Luca

It would be really helpful if you can guide me some more about how to add the required test case.

Best.
Flags: needinfo?(lgreco)
(Assignee)

Comment 10

7 months ago
Luca

Please ignore latest diff.

Comment 11

7 months ago
(In reply to Divyansh Sharma [:spiro] from comment #9)
> Luca
> 
> It would be really helpful if you can guide me some more about how to add
> the required test case.
> 
> Best.

The "step zero" of "adding a new test case" is to be sure that you can run the existing tests related to the changes you applied (or you are going to apply)", in this case the test file that you are going to be interested in is `toolkit/components/extensions/test/xpcshell/test_ext_alarms.js`.

In Comment 5 there is the `mach` command that you should run to execute this test file, and a link to the MDN doc pages that describes how the xpcshell tests works and how to write new ones.

After that, a reasonable next step is to read the MDN doc page and then immediately start to practice what you are learning on the test_ext_alarms.js test file, e.g. try to change an assertion to force the test to fail on purpose, and look into what kind of errors you get and how they correlate with the changes you applied. 

This way you "train" yourself to more easily/quickly recognize the reasons behind the errors that you will met when the tests fail in a way that you didn't expect, and how to read and understand the existing tests. 
Then you can start to use that knowledge to write a new one.

Work on it without worrying too much that the test may not be working yet, I will definitely help you to figure out why, but in my opinion it is better to try on your own a bunch of times first. 
I know that it may be frustrating in the start, but making "your own mistakes" is an invaluable learning experience, I can guarantee based on my experience that "it is worth the frustration" ;-).

If after a couple of days of trying you are still stuck, don't worry:
submit on phabricator the changes applied to the tests like they are, I will take a look to it and help you to figure out what is wrong.
Flags: needinfo?(lgreco)
Attachment #9008036 - Attachment is obsolete: true
Attachment #9013309 - Attachment description: Bug 1478694 [FIXED] → Bug 1478695 - Ensure that WebExtensions alarms set in the past are fired immediately.
Attachment #9013309 - Attachment description: Bug 1478695 - Ensure that WebExtensions alarms set in the past are fired immediately. → Bug 1478694 - Ensure that WebExtensions alarms set in the past are fired immediately.
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 12

7 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c9456c0ba9a
Ensure that WebExtensions alarms set in the past are fired immediately. r=rpl
Keywords: checkin-needed

Comment 13

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c9456c0ba9a
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 14

6 months ago
Will manual testing be required on this bug? if so please provide some info onto how to validate, otherwise please set the qe-validate- flag, thanks!
Flags: needinfo?(sharma.divyansh.501)

Comment 15

6 months ago
I'm marking this as qe-verify- (the actual change is minimal and it is tested in the automated tests included in the same commit).
Flags: needinfo?(sharma.divyansh.501) → qe-verify-
(Assignee)

Comment 16

6 months ago
Luca

Didn't we created test case for validation?
Flags: needinfo?(lgreco)

Comment 17

6 months ago
(In reply to Divyansh Sharma [:spiro] from comment #16)
> Luca
> 
> Didn't we created test case for validation?

Yes, that's the reason why I mentioned it in comment 15 to motivate the qe-verify- (and cleared the needinfo).
Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.