943152 - SpecialPowers.flushPrefEnv is not called during SimpleTest.finish() for TestRunner mochitest-chrome runs

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Bobby Holley (:bholley)]

I was tearing my hair out trying to figure out why my patch in bug 932906 was causing a test to go orange. Eventually, I discovered that the pref I was munging with pushPrefEnv wasn't being reset.

It turns out that flushPrefEnv isn't always called, which is a royal footgun. I'll work up some testcases.

Comment 1

[Bobby Holley (:bholley)]

Looks like this is specific to mochitest-chrome.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Attaching testcases. Currently the mochitest-plain versions pass, but the
mochitest-chrome ones fail. This means that, currently, mochitest-chrome runs
happen with incorrect prefs. :-(

Comment 3

[Bobby Holley (:bholley)]

I'm guessing that, in the mochitest-chrome case, the SpecialPowers that we reference here isn't the same SpecialPowers that we invoke flushPrefEnv on:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#477

ted, any insight? If you don't have time right now, please just let me know and I'll try to find someone else.

Comment 4

[Bobby Holley (:bholley)]

Or maybe jmaher can help.

Comment 5

[Martijn Wargers (dead)]

alert(SpecialPowers.toString()) returns [SpecialPowers] inside a test.
alert(SpecialPowers.toString()) returns [ChromePowers].
It seems that ChromePowers.js doesn't do its work well.

Comment 6

[Martijn Wargers (dead)]

Attachment: harness.diff

This makes it work by just removing this chromepowers stuff from harness.xul.
There is already a specialpowers extension installed, everything seems to work just fine with that one.

But I guess there is a good reason why there is chromepowers, right?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8340474 [details] [diff] [review]
harness.diff

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

chromepowers was meant as a shim for the chrome layer which runs with full privileges.  I recall some issues running full special powers in chrome code while initially developing specialpowers.

I know that specialpowers has evolved for the better, it could be possible we don't need chrome powers anymore.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :mw22 for looking into this.

Comment 9

[Bobby Holley (:bholley)]

That's great Martijn! Can you run it through try to see if it's all good? If so, let's get this landed.

Comment 10

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=40f048333ab7

Comment 11

[Martijn Wargers (dead)]

Attachment: 943152.diff

Ok, tryserver is green, so this patch needs a formal r+, then it can be checked in.

Comment 12

[Bobby Holley (:bholley)]

I'll check this in, along with the tests.

Comment 13

[Bobby Holley (:bholley)]

Pushed this to try along with the tests and along with bug 932906:

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

Comment 14

[Martijn Wargers (dead)]

Bobby, I don't think the tests are good. At least mochitests can be chunked in several parts, which means that test_SpecialPowersExtension.html and test_SpecialPowersExtension2.html are possible to not run after each other.
I've discussed this before with jgriffin, he thinks every test should be self-containing and standalone.

So what we could do with the mochitest-chrome test and mochitest is use parent.SpecialPowers.flushPrefEnv to test this bug that was occuring.
Let me know if you want me to write test for this.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #14)
> Bobby, I don't think the tests are good. At least mochitests can be chunked
> in several parts, which means that test_SpecialPowersExtension.html and
> test_SpecialPowersExtension2.html are possible to not run after each other.

Do tests from different directories actually run in separate chunks? I'd be somewhat surprised.

> I've discussed this before with jgriffin, he thinks every test should be
> self-containing and standalone.

If run in a different order or configuration, these tests won't fail - they just won't test what they wanted to.

In general, I don't really see how we can write a standalone test for this, because the whole issue here is the behavior when multiple tests run in series.

> So what we could do with the mochitest-chrome test and mochitest is use
> parent.SpecialPowers.flushPrefEnv to test this bug that was occuring.
> Let me know if you want me to write test for this.

But the whole problem here is that flushPrefEnv is supposed to be called automatically. Calling it manually doesn't help us with that.

The tests here aren't perfect, but they'll probably catch regressions with 90% certainty, and won't cause any trouble in the remaining 10% situations.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8338620 [details] [diff] [review]
Tests. v1

Given the controversy, I don't think it's fair to land these with r=me. Flagging Martijn for review.

Comment 17

[Martijn Wargers (dead)]

(In reply to Bobby Holley (:bholley) from comment #15)
> Do tests from different directories actually run in separate chunks? I'd be
> somewhat surprised.

I assume you mean from different directories? I'm not sure, perhaps jgriffin would know that.
 
> But the whole problem here is that flushPrefEnv is supposed to be called
> automatically. Calling it manually doesn't help us with that.

I believe in this case when the bug occurs, parent.SpecialPowers.flushPrefEnv is called, but parent.SpecialPowers is of ChromePowers instance, which apparently has a non-working flushPrefEnv function.
So calling it manually would catch this specific bug.
 
> The tests here aren't perfect, but they'll probably catch regressions with
> 90% certainty, and won't cause any trouble in the remaining 10% situations.

Yeah, I agree.

Comment 18

[Jonathan Griffin (:jgriffin)]

Currently the way that chunking occurs, tests in the same directory are guaranteed to run in the same chunk.

However, that isn't a guarantee for the future..we'd like to be smarter about how we chunk, which may require us to split large directories.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #17)
> (In reply to Bobby Holley (:bholley) from comment #15)
> > Do tests from different directories actually run in separate chunks? I'd be
> > somewhat surprised.
> 
> I assume you mean from different directories?

Er, I meant 'same directories'. Sorry.


> > The tests here aren't perfect, but they'll probably catch regressions with
> > 90% certainty, and won't cause any trouble in the remaining 10% situations.
> 
> Yeah, I agree.

Ok. Can you r+ the tests then, and optionally add any additional coverage you think would be helpful?

Comment 20

[Martijn Wargers (dead)]

Comment on attachment 8338620 [details] [diff] [review]
Tests. v1

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

I'm not comfortable reviewing this, as I was pointed out before (in some bug), that I should write standalone tests, if possible.
I think jgriffin pointed that out to me, so I think he's a better reviewer for this.

Comment 21

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8338620 [details] [diff] [review]
Tests. v1

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

I think this is ok.  In the current chunking mechanism, these tests will be run in the same chunk.  That might not be the case in the future, but these tests won't break in that case, they'll just pass without testing what they think they're testing, which is much better than introducing inter-test dependencies that will break if the tests aren't run in sequence.

Comment 22

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e63758ec19f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae80631738a

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e63758ec19f8
https://hg.mozilla.org/mozilla-central/rev/1ae80631738a

Comment 24

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/267af628d7d6
https://hg.mozilla.org/releases/mozilla-aurora/rev/405c0d551b5e

Comment 25

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-esr24/rev/a31f5601bada
https://hg.mozilla.org/releases/mozilla-esr24/rev/df28ae3ba9eb

Comment 26

[(PTO June 19-July 7) Ryan VanderMeulen]

This had to be backed out for mochitest-other failures.
https://hg.mozilla.org/releases/mozilla-esr24/rev/fc537677f296

https://tbpl.mozilla.org/php/getParsedLog.php?id=33217899&tree=Mozilla-Esr24

Comment 27

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> This had to be backed out for mochitest-other failures.
> https://hg.mozilla.org/releases/mozilla-esr24/rev/fc537677f296

This is the way that bug 932906 fails when this bug isn't present.

Looking at the backport that was pushed, the changes to the harness in this bug look wrong - probably a mismerge? Ryan, does this make sense to you?

Let me know if you need me to come up with the backport. We could also just backport bug 932906 on its own and fix up the test to not depend on this bug.

Comment 28

[(PTO June 19-July 7) Ryan VanderMeulen]

(In reply to Bobby Holley (:bholley) from comment #27)
> Looking at the backport that was pushed, the changes to the harness in this
> bug look wrong - probably a mismerge? Ryan, does this make sense to you?

How so? AFAICT, it looks like the Aurora version (minus some context differences), which is what I used for the esr24 backport. And it obviously stuck on Aurora OK.

Comment 29

[Bobby Holley (:bholley)]

Arg, my mistake, sorry. I'd misremembered what this patch was supposed to look like.

Anyway, I'll just fix up the tests in bug 932906 to not depend on this and we can wontfix this for esr24.

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

Martijn, can you comment as to whether you feel this has been sufficiently fixed or not? Thank you.

Comment 31

[Martijn Wargers (dead)]

Yes.