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

Status: VERIFIED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: 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

Comment 1

[Natalya Kot [:nkot]]

confirming, 1.4 is unaffected

Comment 2

[Harvey]

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.

Comment 3

[Henry Chang [:hchang]]

Attachment: Bug993732.patch

Since some properties used in |SystemMessageManager::receiveMessage()| has been renamed by bug 988129, we should use new names to ack SystemMessageInternal.

Comment 4

[Anthony Ricaud (:rik)]

Henry: This patch is missing new tests.

Comment 5

[Henry Chang [:hchang]]

Attachment: test_system_message.py

Attached test case for gaia-ui-test

Comment 6

[(no longer active)]

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.  :-)

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5a5ed08df529

Comment 8

[Henry Chang [:hchang]]

(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

Comment 9

[Jason Smith [:jsmith]]

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

Comment 10

[Zac C (:zac)]

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.

Comment 11

[Henry Chang [:hchang]]

(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...

Comment 12

[Zac C (:zac)]

It picks the latest one from TBPL (I think mozilla-central branch)

Comment 13

[Henry Chang [:hchang]]

(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.

Comment 14

[Anthony Ricaud (:rik)]

Is it possible to also have a test in Gecko so that we catch those errors earlier?

Comment 15

[Gene Lian (I already quit Mozilla)]

(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.

Comment 20

[Natalya Kot [:nkot]]

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

Comment 21

[Henry Chang [:hchang]]

(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.

Comment 22

[Henry Chang [:hchang]]

Attachment: Bug993732.patch

Test case for gecko. This test case uses alarm API to test if an running app could receive the system message.

Comment 23

[Henry Chang [:hchang]]

(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

Comment 24

[Henry Chang [:hchang]]

(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.

Comment 25

[Henry Chang [:hchang]]

Attachment: Test case for this bug

Comment 27

[[:fabrice] Fabrice Desré]

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

Comment 28

[Henry Chang [:hchang]]

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!

Comment 29

[Henry Chang [:hchang]]

Attachment: Test case for this bug V2

Comment 30

[Henry Chang [:hchang]]

Moved test case to Bug 997047

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3d749ffa6003

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d749ffa6003