Closed Bug 993732 Opened 10 years ago Closed 10 years ago

[B2G][Clock] Alarm set with clock app does not fire until user reopens clock app

Categories

(Core :: DOM: Core & HTML, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla31
blocking-b2g 2.0+
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed

People

(Reporter: bzumwalt, Assigned: hchang)

References

Details

(Keywords: regression, smoketest)

Attachments

(5 files, 2 obsolete files)

Attached file Logcat - Combined
Description:
When user has alarm set with clock app, the alarm will not go off if user is on homescreen. No alarm fires at the set time. The user must relaunch clock app for alarm to go off. Alarm will not go off until user opens clock app again.

Repro Steps:
1) Updated Buri to BuildID: 20140408040204
2) Launch Clock app
3) Create new alarm for a later time by pressing button in upper right corner
4) Press home button to return to home screen.
5) At time when alarm is set to fire, observe that alarm does not go off
6) Launch clock app

Actual:
Set alarm will not go off until user reopens clock app.

Expected:
Set alarm fires at appropriate time without further user intervention.

Environmental Variables:
Device: Buri Master M-C Mozilla RIL
BuildID: 20140408040204
Gaia: 1958454595b1fa0e061f0652ae965629993f5708
Gecko: 8883360b1edb
Version: 31.0a1
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
See attached: Youtube video clip & logcat
blocking-b2g: --- → 1.5?
QA Contact: jharvey
confirming, 1.4 is unaffected
Mozilla Inbound Regression Window:

Last Working:
Device: Buri 1.5 MOZ
BuildID: 20140407034726
Gaia: f1a98bfaa3ab2480945bd7018831fd56c61cdc24
Gecko: d459cc1b5a94
Version: 31.0a1
Firmware Version: v1.2-device.cfg

First Broken:
Device: Buri 1.5 MOZ
BuildID: 20140407040527
Gaia: f1a98bfaa3ab2480945bd7018831fd56c61cdc24
Gecko: f94df8c36ab3
Version: 31.0a1
Firmware Verision: V1.2-device.cfg

Gaia is the same on both builds so this seems to be a Gecko issue.

Gecko Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d459cc1b5a94&tochange=f94df8c36ab3

Note: This issue has the same Regression Windows as bug 993462, bug 993470, and bug 993713.
Blocks: 906164
Component: Gaia::Clock → DOM
Product: Firefox OS → Core
Version: unspecified → Trunk
Assignee: nobody → hchang
Attached patch Bug993732.patchSplinter Review
Since some properties used in |SystemMessageManager::receiveMessage()| has been renamed by bug 988129, we should use new names to ack SystemMessageInternal.
Attachment #8403828 - Flags: review?(gene.lian)
Henry: This patch is missing new tests.
Attached file test_system_message.py
Attached test case for gaia-ui-test
Given <https://hg.mozilla.org/mozilla-central/rev/8db31fe79a2d#l3.129>, the patch looks correct to me.  I'm going to land it on mozilla-central pending Gene's review.  Hopefully Henry promises to land the tests and whatever fixes Gene will ask for when he sees the review request.  :-)
https://hg.mozilla.org/mozilla-central/rev/5a5ed08df529
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attachment #8403828 - Flags: review?(fabrice)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> Given <https://hg.mozilla.org/mozilla-central/rev/8db31fe79a2d#l3.129>, the
> patch looks correct to me.  I'm going to land it on mozilla-central pending
> Gene's review.  Hopefully Henry promises to land the tests and whatever
> fixes Gene will ask for when he sees the review request.  :-)

This is the gaia-ui-test PR:

https://github.com/mozilla-b2g/gaia/pull/18122

and the test passed: https://travis-ci.org/mozilla-b2g/gaia/jobs/22604081#L2622
Attachment #8403828 - Flags: review?(gene.lian)
Attachment #8403828 - Flags: review?(fabrice)
Attachment #8403828 - Flags: review+
Attachment #8404031 - Flags: review?(zcampbell)
Comment on attachment 8404031 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18122

Couple of nits about syntax/etc but it's mostly OK.
Attachment #8404031 - Flags: review?(zcampbell) → review-
(In reply to Zac C (:zac) from comment #10)
> Comment on attachment 8404031 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/18122
> 
> Couple of nits about syntax/etc but it's mostly OK.

Zac, Thanks for the review. But errr, I am just aware that the new added test case
shouldn't pass without the gecko patch I provided in this bug.

I should be looking into the detail of the travis ci test like what gecko it uses...
It picks the latest one from TBPL (I think mozilla-central branch)
(In reply to Zac C (:zac) from comment #12)
> It picks the latest one from TBPL (I think mozilla-central branch)

According to the log 

https://s3.amazonaws.com/archive.travis-ci.org/jobs/22604081/log.txt

The gecko gaia-ui-test ran with is http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-linux64_gecko/1397041322/b2g-31.0a1.multi.linux-x86_64.tar.bz2

,which doesn't include the fixes I provided.

I'll re-run the test on my local environment to check what happened.
I guess that the desktop B2G makes the difference.
Is it possible to also have a test in Gecko so that we catch those errors earlier?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> Given <https://hg.mozilla.org/mozilla-central/rev/8db31fe79a2d#l3.129>, the
> patch looks correct to me.  I'm going to land it on mozilla-central pending
> Gene's review.  Hopefully Henry promises to land the tests and whatever
> fixes Gene will ask for when he sees the review request.  :-)

Yeap. The patch is correct. Bug 906164 missed to use the latest notations for manifestURL and pageURL. And we definitely need to provide new tests for that to avoid the same issue.
blocking-b2g: 1.5? → 1.5+
Verified Fixed. 
Set alarm fires at appropriate time without further user intervention.

Buri, master 
BuildID: 20140409130909
Gaia: 9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko: 5a5ed08df529
Version: 31.0a1
Firmware Version: v1.2-device.cfg
Status: RESOLVED → VERIFIED
(In reply to Anthony Ricaud (:rik) from comment #14)
> Is it possible to also have a test in Gecko so that we catch those errors
> earlier?

For general system message test, like if an app is launched by system message, 
I failed to find the appropriate testing framework to do. But for this single 
bug we may be able to write a test in Gecko since this issue is regarding the 
running app. My idea is to just use the existed alarm mochitest or marionette
test.
Attached patch Bug993732.patch (obsolete) — Splinter Review
Test case for gecko. This test case uses alarm API to test if an running app could receive the system message.
(In reply to Henry Chang [:henry] from comment #22)
> Created attachment 8404624 [details] [diff] [review]
> Bug993732.patch
> 
> Test case for gecko. This test case uses alarm API to test if an running app
> could receive the system message.

Try server result:

https://tbpl.mozilla.org/?tree=Try&rev=546be51cb41c @ mochitest chunk 7:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37572461&tree=Try&full=1
(In reply to Henry Chang [:henry] from comment #13)
> (In reply to Zac C (:zac) from comment #12)
> > It picks the latest one from TBPL (I think mozilla-central branch)
> 
> According to the log 
> 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/22604081/log.txt
> 
> The gecko gaia-ui-test ran with is
> http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-
> linux64_gecko/1397041322/b2g-31.0a1.multi.linux-x86_64.tar.bz2
> 
> ,which doesn't include the fixes I provided.
> 
> I'll re-run the test on my local environment to check what happened.
> I guess that the desktop B2G makes the difference.

After investigation, we couldn't reproduce this bug (Bug 993732) on Desktop B2G.
The reason is AssertContainApp("undefined") 
( http://dxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#353 )

will return true on Desktop B2G.

( http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#1661 )

on Desktop B2G


So I wrote the gecko test case on emulator @ Comment 22.
Attached patch Test case for this bug (obsolete) — Splinter Review
Attachment #8404624 - Attachment is obsolete: true
Attachment #8404624 - Flags: review?(fabrice)
Attachment #8405206 - Flags: review?(fabrice)
Attachment #8405206 - Attachment description: Bug993732.patch → Test case for this bug
Attachment #8404624 - Flags: review?(fabrice)
Comment on attachment 8405206 [details] [diff] [review]
Test case for this bug

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

::: dom/messages/test/test_bug_993732.html
@@ +24,5 @@
> +    at.setTime(at.getTime() + aMillisecondsFromNow);
> +
> +    navigator.mozSetMessageHandler('alarm', function(message) {
> +      ok(true, "We got alarm message!");
> +      SimpleTest.finish();      

nit: trailing whitespace

@@ +61,5 @@
> +  }
> +
> +  SimpleTest.expectAssertions(0, 9);
> +  SimpleTest.waitForExplicitFinish();
> +  if (SpecialPowers.hasPermission("alarms", document)) {

please use SpecialPowers.pushPrefEnv() instead of hasPermission/addPermission
Attachment #8405206 - Flags: review?(fabrice) → review-
Thanks for the review!

(In reply to Fabrice Desré [:fabrice] from comment #27)
> Comment on attachment 8405206 [details] [diff] [review]
> Test case for this bug
> 
> Review of attachment 8405206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/messages/test/test_bug_993732.html
> @@ +24,5 @@
> > +    at.setTime(at.getTime() + aMillisecondsFromNow);
> > +
> > +    navigator.mozSetMessageHandler('alarm', function(message) {
> > +      ok(true, "We got alarm message!");
> > +      SimpleTest.finish();      
> 
> nit: trailing whitespace
> 
> @@ +61,5 @@
> > +  }
> > +
> > +  SimpleTest.expectAssertions(0, 9);
> > +  SimpleTest.waitForExplicitFinish();
> > +  if (SpecialPowers.hasPermission("alarms", document)) {
> 
> please use SpecialPowers.pushPrefEnv() instead of hasPermission/addPermission

I guess you're referring to SpecialPowers.pushPermission.

Thanks!
Attachment #8405206 - Attachment is obsolete: true
Moved test case to Bug 997047
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.