Closed Bug 762049 Opened 8 years ago Closed 8 years ago

Run <iframe mozbrowser> tests in- and out-of-process on all platforms except Windows

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(5 files)

At the moment, we run the <iframe mozbrowser> tests OOP on *nix and in-process on Windows.

With bug 761939 fixed, we should be able to run the tests OOP on Windows.  We should rejigger the test framework so we run all the tests both OOP and in-process on both Windows and *nix.
Assignee: nobody → justin.lebar+bug
These patches work, but they rely on bug 757182.  Since the patches will rot as soon as anyone lands a test or test change to mozbrowser, I'm not going to mark for review until all the dependencies are met here.

Although this is a lot of code churn, this should be a simple review; the changes are all mechanical.
Depends on: 757182
Blocks: 744451
Blocks: 742944
No longer blocks: 744451
Blocks: 762329
Comment on attachment 630787 [details] [diff] [review]
Bug 762049 - Part 1: Rename test_browserFrame{1,2,3}.html.

All dependencies landed (\o/).  Mounir, would you like to do the honors?
Attachment #630787 - Flags: review?(mounir)
Comment on attachment 630788 [details] [diff] [review]
Bug 762049 - Part 2: Make remaining tests both in-process and OOP.

The technique here is to create three files for every test:

 1. test_browserElement_oop_Foo.html
 2. test_browserElement_inproc_Foo.html
 3. browserElement_Foo.js

Files (1) and (2) are identical, and all of the test code is in (3).  browserFrameHelpers (renamed to browserElementTestHelpers) sets the in/out-of process pref according to the filename.

I didn't do this for the tests touched in part 1 because those tests are checking the security prefs, which should work the same in and out-of process.

I didn't modify any of the test code, except for:

 * one test which relied on strict mode not being set; I fixed the test so we can run it with strict mode.
 * two tests which relied on an <iframe> being present in the html file.  I want these html files to be mere shells, and it was easy enough to use appendElement to the same effect.

I'm going to post a fourth patch adding PD licenses to all these files.
Attachment #630788 - Flags: review?(mounir)
Comment on attachment 630789 [details] [diff] [review]
Bug 762049 - Part 3: Rename browserFrameHelpers to browserElementTestHelpers.

Slowly getting rid of "browser frame" in favor of "browser element", which I suspect will get a fast r+ from you.  :)
Attachment #630789 - Flags: review?(mounir)
Attachment #630787 - Flags: review?(mounir) → review+
Comment on attachment 630789 [details] [diff] [review]
Bug 762049 - Part 3: Rename browserFrameHelpers to browserElementTestHelpers.

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

r=me, thank you sed.
Attachment #630789 - Flags: review?(mounir) → review+
Comment on attachment 630788 [details] [diff] [review]
Bug 762049 - Part 2: Make remaining tests both in-process and OOP.

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

r=me

You could have given nicer <title> to your tests as long as you were here but that's not very important I believe ;)

::: dom/browser-element/mochitest/test_browserFramePromptConfirm.html
@@ +82,5 @@
>      </scr' + 'ipt></body></html>';
>  }
>  
>  runTest();
> +

nit: remove that empty line.
Attachment #630788 - Flags: review?(mounir) → review+
Comment on attachment 631038 [details] [diff] [review]
Part 4: Add CC0 license to all test JS files.

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

r=me, assuming you didn't forget a file.
BTW, why not html files?
Attachment #631038 - Flags: review?(mounir) → review+
> BTW, why not html files?

Mostly because most of our HTML mochitests don't have licenses.  Is that a thing we're doing these days?
(In reply to Justin Lebar [:jlebar] from comment #12)
> > BTW, why not html files?
> 
> Mostly because most of our HTML mochitests don't have licenses.  Is that a
> thing we're doing these days?

AFAIK, we don't. But I doubt a lot of people take time to put licenses boilerplate in tests in general.
And backed out because of Windows M2 bustage (I didn't notice on try because it manifests as a crash *after* all the browser element tests run).

Windows OOP is apparently not all it's cracked up to be.  But I still want this change, because it makes testing on *nix much better.  I'll disable the OOP tests on Windows and file a follow-up bug for figuring out what's going on there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2982a120fef5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7219414f43ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d64c6f58785
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c8f23e0f49
It seems this also caused a gc crash during shutdown of a debug b2g desktop build.
I just tried with the backout again and it doesn't crash any more.
(In reply to Gregor Wagner [:gwagner] from comment #17)
> It seems this also caused a gc crash during shutdown of a debug b2g desktop
> build.
> I just tried with the backout again and it doesn't crash any more.

Are you sure?  This was a test-only change.
(In reply to Justin Lebar [:jlebar] from comment #18)
> (In reply to Gregor Wagner [:gwagner] from comment #17)
> > It seems this also caused a gc crash during shutdown of a debug b2g desktop
> > build.
> > I just tried with the backout again and it doesn't crash any more.
> 
> Are you sure?  This was a test-only change.

Ah wrong folder... It's still crashing :(
> Ah wrong folder... It's still crashing :(

You mean, it crashes without this change too?  Could you please file a bug?
Blocks: 763081
Summary: Run <iframe mozbrowser> tests in- and out-of-process on all platforms → Run <iframe mozbrowser> tests in- and out-of-process on all platforms except Windows
> And backed out because of Windows M2 bustage

Here's a link to a busted log: https://tbpl.mozilla.org/php/getParsedLog.php?id=12465092&tree=Mozilla-Inbound
Comment on attachment 631537 [details] [diff] [review]
Part 5: Don't use OOP on Windows.

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

r=me even you keep your highly verbose and not clearer version ;)

::: dom/browser-element/mochitest/browserElementTestHelpers.js
@@ +127,3 @@
>  var oop;
> +if (navigator.platform.indexOf('Win') != -1 ||
> +    location.pathname.indexOf("_inproc_") != -1) {

I would have done something like:

// Enable or disable OOP depending on the test's filename.
oop = location.pathname.indexOf("_inproc_") == -1;

// Always disable OOP on Windows (bug 763081).
oop = navigator.platform.indexOf('Win') != -1 ? false : oop;
Attachment #631537 - Flags: review?(mounir) → review+
> r=me even you keep your highly verbose and not clearer version ;)

You're right, mine is pretty ugly, isn't it?
Depends on: 763449
Blocks: 764232
Depends on: 765242
No longer blocks: 766481
Depends on: 779753
You need to log in before you can comment on or make changes to this bug.