Closed Bug 843893 Opened 7 years ago Closed 7 years ago

Current Gaia breaks mochitests for alarm and power APIs

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(b2g18 affected)

RESOLVED FIXED
Tracking Status
b2g18 --- affected

People

(Reporter: jgriffin, Assigned: onecyrenus)

References

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

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.
Duplicate of this bug: 842610
Blocks: 843943
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.
Blocks: 845216
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.
Flags: needinfo?(overholt)
Thanks for the needinfo (sorry, I missed the CC).  I'll find someone.
Flags: needinfo?(overholt)
I've asked Andrea to take a look.
Assignee: nobody → amarchesini
David, this sounds like exactly the thing you were working with Luqman on today. Any comments?
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.
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.
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.
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.
footgun only with OOP enabled, that is. If OOP isn't in play here, you can ignore comment 10.
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.
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
Assignee: amarchesini → dclarke
Flags: needinfo?(dclarke)
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.
Also, comment 12 is salient. This change went in awhile back, so I'm also curious what else changed to make the failures surface.
Depends on: 846057
Attachment #720574 - Flags: review?(gene.lian)
Component: Gaia → General
Previous patch did not include the new file under power/test/
Attachment #720574 - Attachment is obsolete: true
Attachment #720574 - Flags: review?(gene.lian)
Attachment #720792 - Flags: review?(gene.lian)
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!
Attachment #720792 - Flags: review?(gene.lian) → review+
Flags: in-testsuite+
Blocks: 848133
Is there a general ETA on this at this point?  It's blocking DOM performance work from going forward...
Boris, does this need to land on the v1 train ?
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.
Looked like things are more or less done, with an r+-with-changes. It's a question of landing now.
Attached patch mercurial patch with fixes. (obsolete) — Splinter Review
Attachment #722450 - Attachment is patch: true
Attachment #722450 - Flags: review+
Attachment #720792 - Attachment is obsolete: true
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:
Attachment #722450 - Flags: review?(gene.lian)
Attachment #722450 - Flags: review+
Attachment #722450 - Flags: approval-mozilla-b2g18?
We're waiting for review before considering for uplift.
Gene - does your r+ carry over from comment 18?
> 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 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.
Attachment #722450 - Flags: review?(gene.lian) → review+
Needs to land on v1-train, trunk
Attachment #722450 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
guys, we need to fix this asap.  It is blocking work.
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 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.
Attachment #722450 - Flags: review-
Attached patch second try (obsolete) — Splinter Review
Attachment #722450 - Attachment is obsolete: true
Attachment #726390 - Flags: review?(jgriffin)
Attachment #726390 - Attachment is patch: true
Comment on attachment 726390 [details] [diff] [review]
second try

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

Thanks, I'll run this through try again.
Attachment #726390 - Flags: review?(jgriffin) → review+
pushed to try with current emulator:  https://tbpl.mozilla.org/?tree=Try&rev=7900b93ec575
And pushed to try with a new emulator: https://tbpl.mozilla.org/?tree=Try&rev=a60c10e052c7
(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.
(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.
Thanks John.
This also needs to land on b2g18 as well.
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:
Attachment #726390 - Flags: approval-mozilla-b2g18?
This bug was already b2g18+, it is on the previous patch.
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().
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 ?
(In reply to dclarke@mozilla.com  [: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.
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.
Attached patch 3rd time is the charm (obsolete) — Splinter Review
Need to verify that b2g.json has been disabled, the tests run on gonk but do not run on linux, mac, windows.
Attachment #726390 - Attachment is obsolete: true
Attachment #726390 - Flags: approval-mozilla-b2g18?
Attached patch updated patch (obsolete) — Splinter Review
The alarm section passed in the previous run, just one test failed for power, fixed. Verified on my machine.
Attachment #727401 - Attachment is obsolete: true
Attachment #727534 - Flags: review?(jgriffin)
https://bugzilla.mozilla.org/show_bug.cgi?id=853206 

still waiting for tryserver access to come back online for the moment
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 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.
Attachment #727534 - Flags: review?(jgriffin) → review?(gene.lian)
https://tbpl.mozilla.org/?tree=Try&rev=3faef0b34234

I had kicked this off last night. Is it sufficient ?
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.
Attachment #727534 - Flags: review?(gene.lian) → review+
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.
https://tbpl.mozilla.org/?tree=Try&rev=46b0a2692145

Finally got the patch right, lots of goofs
Attached patch Correct patchSplinter Review
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.
Attachment #727534 - Attachment is obsolete: true
Attachment #728591 - Flags: review+
Try run was green, but it didn't include any Android runs.  I'm thinking we should check it there too.
No longer blocks: 845216
No longer blocks: 843631
try seems to have passed, but random orange showed up on unrelated testcase.
https://hg.mozilla.org/mozilla-central/rev/0edebdda6d5a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Attachment #722450 - Flags: approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.