Closed Bug 943152 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox27 fixed, firefox28 fixed, firefox-esr24 wontfix)

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: bholley, Assigned: martijn.martijn)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Looks like this is specific to mochitest-chrome.
Summary: SpecialPowers.flushPrefEnv is not always called during SimpleTest.finish() → SpecialPowers.flushPrefEnv is not called during SimpleTest.finish() for TestRunner mochitest-chrome runs
Attached patch Tests. v1Splinter Review
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. :-(
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.
Flags: needinfo?(ted)
Or maybe jmaher can help.
Flags: needinfo?(jmaher)
alert(SpecialPowers.toString()) returns [SpecialPowers] inside a test.
alert(SpecialPowers.toString()) returns [ChromePowers].
It seems that ChromePowers.js doesn't do its work well.
Attached patch harness.diff (obsolete) — Splinter Review
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?
Attachment #8340474 - Flags: feedback?(jmaher)
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.
Attachment #8340474 - Flags: feedback?(jmaher) → feedback+
thanks :mw22 for looking into this.
Flags: needinfo?(jmaher)
That's great Martijn! Can you run it through try to see if it's all good? If so, let's get this landed.
Assignee: nobody → martijn.martijn
Attached patch 943152.diffSplinter Review
Ok, tryserver is green, so this patch needs a formal r+, then it can be checked in.
Attachment #8340474 - Attachment is obsolete: true
Attachment #8342223 - Flags: review?(jmaher)
Attachment #8342223 - Flags: review?(jmaher) → review+
Keywords: checkin-needed
I'll check this in, along with the tests.
Keywords: checkin-needed
Flags: needinfo?(ted)
Pushed this to try along with the tests and along with bug 932906:

https://tbpl.mozilla.org/?tree=Try&rev=4e1cd6c700ea
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.
(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 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.
Attachment #8338620 - Flags: review?(martijn.martijn)
(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.
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.
(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 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.
Attachment #8338620 - Flags: review?(martijn.martijn) → review?(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.
Attachment #8338620 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/e63758ec19f8
https://hg.mozilla.org/mozilla-central/rev/1ae80631738a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(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.
Flags: needinfo?(ryanvm)
(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.
Flags: needinfo?(ryanvm)
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.
Martijn, can you comment as to whether you feel this has been sufficiently fixed or not? Thank you.
Flags: needinfo?(martijn.martijn)
Yes.
Flags: needinfo?(martijn.martijn)
See Also: → 1197266
You need to log in before you can comment on or make changes to this bug.