843893 - Current Gaia breaks mochitests for alarm and power APIs

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

The current version of Gaia causes some mochitests to fail:

15:50:30 INFO - 5 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/alarm/test/test_alarm_non_permitted_app.html | navigator.mozAlarms should return null - got [xpconnect wrapped nsIDOMMozAlarmsManager], expected null
16:31:52 INFO - 3901 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/power/test/test_power_basics.html | Shouldn't be able to access power manager without permission.
16:38:43 INFO - I/GeckoDump( 826): 5 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/alarm/test/test_alarm_non_permitted_app.html | navigator.mozAlarms should return null - got [xpconnect wrapped nsIDOMMozAlarmsManager], expected null
16:38:45 INFO - I/GeckoDump( 826): 3901 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/power/test/test_power_basics.html | Shouldn't be able to access power manager without permission.
16:38:46 ERROR - F/libc ( 794): Fatal signal 11 (SIGSEGV) at 0x6f72706c (code=1)
16:38:46 ERROR - This usually indicates the B2G process has crashed

https://tbpl.mozilla.org/php/getParsedLog.php?id=19969020&tree=Mozilla-Inbound&full=1

Because of this problem, we can't update the emulator used in TBPL; we're forced to use an old emulator which has a version of Gaia that doesn't cause this conflict (commit 3df466a3a11c898adc1874c59784ce83a3bab300).

This was also seen in bug 841836.

Not being able to update the emulator will prevent other fixes from landing that require new emulator builds.

Comment 2

[Jonathan Griffin (:jgriffin)]

overholt, scravag, I'm not sure if this a Gaia issue or a DOM issue...can you get someone to investigate?  This is blocking a couple of other issues and will likely start blocking more soon.

Comment 3

[Jonathan Griffin (:jgriffin)]

Andrew, is this something you can get someone to look at?  It's blocking more and more things from landing, since it prevents us from updating the emulator that's being used in TBPL.

Comment 4

[Andrew Overholt [:overholt]]

Thanks for the needinfo (sorry, I missed the CC).  I'll find someone.

Comment 5

[Andrew Overholt [:overholt]]

I've asked Andrea to take a look.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

David, this sounds like exactly the thing you were working with Luqman on today. Any comments?

Comment 7

[Luqman Aden [:Luqman]]

So David and I were trying to figure out yesterday why the alarm tests were failing a
und it came down how they were using the SpecialPowers permissions functions:

> SpecialPowers.addPermission
> SpecialPowers.removePermission

So as far as I can tell, once a permission is set with those functions it won't be propagated to the already initialized DOM objects. I know this is true of the AlarmsApi which seems to only check the permission on page load.

http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#149

The approach we took to get around this was to add a way to also check permissions with SpecialPowers and wrote a little helper which would take a callback, wrap it in an if statement for actually testing if you have the permission you want set and if not setting it then reloading the page.

Comment 8

[Luqman Aden [:Luqman]]

Actually, looking at the power API test, it already seems to do the set-then-reload trick. The reason it's probably failing is because it expects to start in a state where the 'power' permission is not set. AFAIK, mochitests on b2g run initially with all permissions enabled. A quick fix could be to just reverse the 2 tests or remove the permission at the start of the test.

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Sounds like this might be related to the decision to have B2G mochitests run with wide-open permissions so that functional tests weren't hamstrung.

At that time, we also discussed the need to have a mode when it ran with closed permissions so that permissions tests had the proper fixture to run from. 

I think the proposal with the most legs was actually two distinct mochitest runs, one under a permissive manifest and one under a conservative manifest.

Comment 10

[Josh Matthews [:jdm]]

This sounds like the exact same problem that's hitting bug 826058, namely that nobody ever implemented bug 685652. addPermission and setXPreference are footgun APIs in SpecialPowers, unfortunately.

Comment 11

[Josh Matthews [:jdm]]

footgun only with OOP enabled, that is. If OOP isn't in play here, you can ignore comment 10.

Comment 12

[Jonathan Griffin (:jgriffin)]

The confusing thing about this is that it is apparently sensitive to Gaia.  With https://github.com/mozilla-b2g/gaia/commit/3df466a3a11c898adc1874c59784ce83a3bab300 (from 25 days ago), this problem doesn't occur, but with current Gaia it does.  There have not been any changes to the mochitest test container app in that timeframe.

Comment 13

[[e-mail] [:onecyrenus]]

geo: all is as described by luqman, would recommend we work on a quick patch to fix this functionality.  

I'll see if i can work on a patch to get these running as is

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

David,

Sure, agreed re: this fix. 

I can spin out the other discussion, but there will be a need to settle what the start-state of a B2G mochitest is and at least communicate it. We'll also need a way to start a mochitest w/o all permissions set, if we're to do valid perms testing.

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Also, comment 12 is salient. This change went in awhile back, so I'm also curious what else changed to make the failures surface.

Comment 16

[[e-mail] [:onecyrenus]]

Attachment: patch to alarm / power to correctly add / remove permissions from a test

Comment 17

[[e-mail] [:onecyrenus]]

Attachment: power & alarm api tests that should work regardless of the state of the mochitest app

Previous patch did not include the new file under power/test/

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment on attachment 720792 [details] [diff] [review]
power & alarm api tests that should work regardless of the state of the mochitest app

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

Thanks David! Looks great!

I don't have strong opinion but just wondering why the file names are slightly different:

  test_power_non_permitted.html
  test_alarm_non_permitted_app.html

Just a picky reminder. It's up to you. :) Btw, please remove the temporal dump() functions and run the test before check-in. Thanks!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Is there a general ETA on this at this point?  It's blocking DOM performance work from going forward...

Comment 20

[[e-mail] [:onecyrenus]]

Boris, does this need to land on the v1 train ?

Comment 21

[Andrew Halberstadt [:ahal]]

If this bug is well under way, can't we just temporarily disable the affected tests? Assuming this is still a while away from completion.

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Looked like things are more or less done, with an r+-with-changes. It's a question of landing now.

Comment 23

[[e-mail] [:onecyrenus]]

Attachment: mercurial patch with fixes.

Comment 24

[[e-mail] [:onecyrenus]]

Comment on attachment 722450 [details] [diff] [review]
mercurial patch with fixes.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 843893
User impact if declined: mochitest run won't pass
Testing completed: mochitest fixes
Risk to taking this patch (and alternatives if risky): 0
String or UUID changes made by this patch:

Comment 25

[Alex Keybl [:akeybl]]

We're waiting for review before considering for uplift.

Comment 26

[Alex Keybl [:akeybl]]

Gene - does your r+ carry over from comment 18?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Boris, does this need to land on the v1 train ?

For my purposes it just needs to land in whatever we run on tbpl for inbound/m-c/try....  I have DOM patches that expose preexisting bugs in gaia that are now fixed but the snapshot we use on tbpl is still buggy and can't be upgraded due to this bug.

Comment 28

[Gene Lian (I already quit Mozilla)]

Comment on attachment 722450 [details] [diff] [review]
mercurial patch with fixes.

Sorry for the delayed response. I'm just aware of this.

Asking for approval-mozilla-b2g-18 doesn't need to get a review+ again if the patch used to get a review+ before. Marking review+ anyway.

Comment 29

[[e-mail] [:onecyrenus]]

Needs to land on v1-train, trunk

Comment 30

[Doug Turner (:dougt)]

guys, we need to fix this asap.  It is blocking work.

Comment 31

[Jonathan Griffin (:jgriffin)]

Pushed to try along with the latest emulator (to verify that the alarm and power API failures go away):

https://tbpl.mozilla.org/?tree=Try&rev=cffc4618bf7f

Pushed a second version to try running against the current emulator to make sure it's harmless in this context (so we can land independently of an emulator update):

https://tbpl.mozilla.org/?tree=Try&rev=70011fc56183

Comment 32

[Jonathan Griffin (:jgriffin)]

Comment on attachment 722450 [details] [diff] [review]
mercurial patch with fixes.

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

This patch adds "test_power_non_permitted_app.html" to Makefile.in, but doesn't actually add the file itself.

Comment 33

[[e-mail] [:onecyrenus]]

Attachment: second try

Comment 34

[Jonathan Griffin (:jgriffin)]

Comment on attachment 726390 [details] [diff] [review]
second try

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

Thanks, I'll run this through try again.

Comment 35

[Jonathan Griffin (:jgriffin)]

pushed to try with current emulator:  https://tbpl.mozilla.org/?tree=Try&rev=7900b93ec575

Comment 36

[Jonathan Griffin (:jgriffin)]

And pushed to try with a new emulator: https://tbpl.mozilla.org/?tree=Try&rev=a60c10e052c7

Comment 37

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #36)
> And pushed to try with a new emulator:
> https://tbpl.mozilla.org/?tree=Try&rev=a60c10e052c7

There are failures related to the new emulator, but none in alarm and power API's.

Comment 38

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #35)
> pushed to try with current emulator: 
> https://tbpl.mozilla.org/?tree=Try&rev=7900b93ec575

This passed too.  This means we can land this patch now, prior to an emulator update.

Comment 39

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8eeacc3c8a

Comment 40

[[e-mail] [:onecyrenus]]

Thanks John.

Comment 41

[[e-mail] [:onecyrenus]]

This also needs to land on b2g18 as well.

Comment 42

[Jonathan Griffin (:jgriffin)]

Comment on attachment 726390 [details] [diff] [review]
second try

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 

We need to take this patch as a precondition for updating the emulator used in TBPL to run tests on mozilla-b2g18.  Without this patch, we are stuck with an old emulator.  This patch only changes some mochitests, so there is no risk.

String or UUID changes made by this patch:

Comment 43

[[e-mail] [:onecyrenus]]

This bug was already b2g18+, it is on the previous patch.

Comment 44

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/42caf4845677 - at least, so far, Linux and Mac agree that the changes to test_power_basics.html make power not ok().

Comment 45

[[e-mail] [:onecyrenus]]

Phil: 

why do power tests get run on linux / mac again ? do you have a view into what those results were so i can try to repro. 

Jgriffin:

i think the last try build did not run the the power and alarm tests.. do you know why ?

Comment 46

[Jonathan Griffin (:jgriffin)]

(In reply to [e-mail]  [:onecyrenus] from comment #45)
> Phil: 
> 
> why do power tests get run on linux / mac again ? do you have a view into
> what those results were so i can try to repro. 
> 

We need to prevent these tests from running on desktop, either via Makefile.in or in the tests themselves.

> Jgriffin:
> 
> i think the last try build did not run the the power and alarm tests.. do
> you know why ?

It seems they are excluded in http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json due to earlier test failures.

Comment 47

[[e-mail] [:onecyrenus]]

jgriffin: remixing a new patch to only run when running as gonk, and will remove from b2g.json file so we actually see the result.

Comment 48

[[e-mail] [:onecyrenus]]

Attachment: 3rd time is the charm

Need to verify that b2g.json has been disabled, the tests run on gonk but do not run on linux, mac, windows.

Comment 49

[Jonathan Griffin (:jgriffin)]

Latest version pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=1cf991e7948e

Comment 50

[[e-mail] [:onecyrenus]]

Attachment: updated patch

The alarm section passed in the previous run, just one test failed for power, fixed. Verified on my machine.

Comment 51

[[e-mail] [:onecyrenus]]

https://bugzilla.mozilla.org/show_bug.cgi?id=853206 

still waiting for tryserver access to come back online for the moment

Comment 52

[Jonathan Griffin (:jgriffin)]

If you want to push this to try yourself, the correct syntax is:

try: -b o -p ics_armv7a_gecko,linux64,macosx64,win32 -u mochitests -t none

Comment 53

[Jonathan Griffin (:jgriffin)]

Comment on attachment 727534 [details] [diff] [review]
updated patch

I don't know the details of how the power API works or should be tested, so I'm punting this review to someone who might.

Comment 54

[[e-mail] [:onecyrenus]]

https://tbpl.mozilla.org/?tree=Try&rev=3faef0b34234

I had kicked this off last night. Is it sufficient ?

Comment 55

[[e-mail] [:onecyrenus]]

Comment on attachment 727534 [details] [diff] [review]
updated patch

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

This doesn't need a re-review, it's a one line change, and doesn't change the semantics of the test.

Comment 56

[Jonathan Griffin (:jgriffin)]

Did you push the right patch to try?  It looks like it can't find test_power_non_permitted_app.html:

02:07:43 ERROR - make[8]: *** No rule to make target `test_power_non_permitted_app.html', needed by `libs'. Stop.

Comment 57

[[e-mail] [:onecyrenus]]

https://tbpl.mozilla.org/?tree=Try&rev=46b0a2692145

Finally got the patch right, lots of goofs

Comment 58

[[e-mail] [:onecyrenus]]

Attachment: Correct patch

Basically it looks like a few days ago moz-central was changed for alarm api. 

I have made it so the power api does not run on desktop, alarm however runs across all, and passes.

Comment 59

[Jonathan Griffin (:jgriffin)]

Try run was green, but it didn't include any Android runs.  I'm thinking we should check it there too.

Comment 60

[[e-mail] [:onecyrenus]]

just kicked off a run:
https://tbpl.mozilla.org/?tree=Try&rev=daaac9ca1159

Comment 61

[[e-mail] [:onecyrenus]]

try seems to have passed, but random orange showed up on unrelated testcase.

Comment 62

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0edebdda6d5a

Comment 63

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/0edebdda6d5a

Comment 64

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 722450 [details] [diff] [review]
mercurial patch with fixes.

Please get approval on the patch that was actually landed if you want this uplifted to b2g18.