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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Bug 762049 - Part 1: Rename test_browserFrame{1,2,3}.html.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Bug 762049 - Part 2: Make remaining tests both in-process and OOP.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Bug 762049 - Part 3: Rename browserFrameHelpers to browserElementTestHelpers.

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

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?

Comment 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Justin Lebar (not reading bugmail)]

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.  :)

Comment 8

[Mounir Lamouri (:mounir)]

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.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Part 4: Add CC0 license to all test JS files.

Comment 10

[Mounir Lamouri (:mounir)]

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.

Comment 11

[Mounir Lamouri (:mounir)]

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?

Comment 12

[Justin Lebar (not reading bugmail)]

> 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?

Comment 13

[Mounir Lamouri (:mounir)]

(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.

Comment 14

[Justin Lebar (not reading bugmail)]

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

Comment 15

[Justin Lebar (not reading bugmail)]

Landed for FF16.

https://hg.mozilla.org/integration/mozilla-inbound/rev/840a820e589c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b40369a421bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e129ca091029
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd157a5da52f

Comment 16

[Justin Lebar (not reading bugmail)]

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

Comment 17

[Gregor Wagner [:gwagner]]

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.

Comment 18

[Justin Lebar (not reading bugmail)]

(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.

Comment 19

[Gregor Wagner [:gwagner]]

(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 :(

Comment 20

[Justin Lebar (not reading bugmail)]

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

You mean, it crashes without this change too?  Could you please file a bug?

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Part 5: Don't use OOP on Windows.

Comment 22

[Justin Lebar (not reading bugmail)]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=20fac72a35ae

Comment 23

[Justin Lebar (not reading bugmail)]

> 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 24

[Mounir Lamouri (:mounir)]

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;

Comment 25

[Justin Lebar (not reading bugmail)]

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

You're right, mine is pretty ugly, isn't it?

Comment 26

[Justin Lebar (not reading bugmail)]

Landed again for FF16:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f138abe4c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a827c12da43
https://hg.mozilla.org/integration/mozilla-inbound/rev/830251d84ccd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a53c96e72649
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3d46a285e6

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9f138abe4c3
https://hg.mozilla.org/mozilla-central/rev/3a827c12da43
https://hg.mozilla.org/mozilla-central/rev/830251d84ccd
https://hg.mozilla.org/mozilla-central/rev/a53c96e72649
https://hg.mozilla.org/mozilla-central/rev/ee3d46a285e6