1301340 - [e10s-multi] Force single content process in tests temporarily where multiple content process would fail

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Gabor Krizsanits (INACTIVE)]

To enable 2 content processes on nightly we have to fix some tests, and there are some known issues we will ignore for nightly. Proper fix for these tests will come later see: Bug 1301015.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: Force single content process for some tests

So most of the test failures came from the permit unload message timeout in the tab closing code. There were the few remaining issues that we might want to fix up later properly.

Comment 2

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3876a85aa0cf830855fb02796a40feb30efd73c
Bug 1301340 - Force single process in some tests. r=mrbkap

Comment 3

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/708ea01b2758870b59a3d92e8a2e65e607b96b89
Bug 1301340 - Missing semicolon. r=me

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3876a85aa0c
https://hg.mozilla.org/mozilla-central/rev/708ea01b2758

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3876a85aa0c
https://hg.mozilla.org/mozilla-central/rev/708ea01b2758

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: part2

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8799862 [details] [diff] [review]
part2

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

r=me with browser_mcb_redirect.js reverted to run all of the tests.

::: browser/base/content/test/general/browser_mcb_redirect.js
@@ +191,5 @@
>  
>    var expected = "image loaded"
>    waitForCondition(
>      () => content.document.getElementById('mctestdiv').innerHTML == expected,
> +    cleanUpAfterTests, "Error: Waited too long for status in Test 5!",

Did you mean to change this? It looks like this patch effectively only runs tests 4 and 5 with these changes.

Comment 8

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95bc66b0c8f5fc235ca2ed8c872d9161f10382d0
Bug 1301340 - part2: Force single content process in some tests. r=mrbkap

Comment 9

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/projects/ash/rev/16c972831b9eee68d9eccaf975b6a5dc2ce4b109
Bug 1301340 - part2: Force single content process in some tests. r=mrbkap

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95bc66b0c8f5

Comment 11

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/projects/ash/rev/cfc5704efc4a6e0414d6615c0c03cfcbbd2d82ff
Bug 1301340 - part3: Force single content process for tests that times out. r=me

https://hg.mozilla.org/projects/ash/rev/c4e14ac4bbb1e04124bd0ff82042c8c53edbf30c
Bug 1301340 - part4: Force single content process for failing tests. r=me

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: Force single content process for tests that times out.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: part4: Force single content process for failing tests.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: part5: Turning off some tests temporarily.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Attachment: part5: Turning off some tests temporarily. v2

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8806680 [details] [diff] [review]
part5: Turning off some tests temporarily. v2

r=me with a new bug to re-enable them (and the comments pointing at that new bug). We should also make sure that the module owners of these modules are aware that we're temporarily turning these off.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18ca736c0d1d
part3: Force single content process for tests that times out. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5a2c5cfb32
part4: Force single content process for failing tests. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/f54a717c6fc6
part5: Turning off some tests temporarily to enable e10s-multi. r=mrbkap

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Why was part 5 "Turning off some tests temporarily to enable e10s-multi" pushed to inbound?  It seems surprising to fully disable  a test in all configurations when it's only the e10s-multi configuration causing trouble...  Wouldn't it better to expose e10s-multi as a property you can check in skip-if lines or only land this on ash?

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Why was part 5 "Turning off some tests temporarily to enable e10s-multi"
> pushed to inbound?  It seems surprising to fully disable  a test in all
> configurations when it's only the e10s-multi configuration causing
> trouble...  Wouldn't it better to expose e10s-multi as a property you can
> check in skip-if lines or only land this on ash?

There won't be separate e10s and e10s-multi configurations, however you are
totally right, I have no reason to disable the test for non-e10s. I will
file/land a follow-up patch tomorrow morning, or if you're concerned about it
feel free to back it out as an alternative.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> > Why was part 5 "Turning off some tests temporarily to enable e10s-multi"
> > pushed to inbound?  It seems surprising to fully disable  a test in all
> > configurations when it's only the e10s-multi configuration causing
> > trouble...  Wouldn't it better to expose e10s-multi as a property you can
> > check in skip-if lines or only land this on ash?
> 
> There won't be separate e10s and e10s-multi configurations, however you are
> totally right, I have no reason to disable the test for non-e10s. I will
> file/land a follow-up patch tomorrow morning, or if you're concerned about it
> feel free to back it out as an alternative.

Hmm, I am guessing that won't help for the responsive.html test I am most interested in, since the whole test directory only runs with e10s on anyway...  but maybe that's good enough for the others?

I guess the main path forward is just to do whatever's needed to fix the test anyway.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/18ca736c0d1d
https://hg.mozilla.org/mozilla-central/rev/4a5a2c5cfb32
https://hg.mozilla.org/mozilla-central/rev/f54a717c6fc6

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb45e90f6596
part6: Disabled test should still run in non-e10s mode. r=me

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cb45e90f6596

Comment 24

[Gabor Krizsanits (INACTIVE)]

We have a few fairly regular intermittents left:

dom/cache/test/mochitest/browser_cache_pb_window.js - intermittent leak, we had it before...

toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger_window_open.js intermittent

* browser/base/content/test/general/browser_tabfocus.js linux debug -> almpst perma orange

page-worker/main.test page load - (JP) known intermittent

browser/base/content/test/referrer/browser_referrer_middle_click_in_container.js | A promise chain failed to handle a rejection intermittent (slightly different than before

* browser/base/content/test/general/browser_bug719271.js Assertion failure: !mTimer, at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/widget/windows/nsAppShell.cpp:83 wind7 debug VM intermittent (the test was already an intermittent but now it's more frequent and we hit an assertion)

I've decided to turn off the two marked with '*'. And with the rest I would say we're good to go.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b2458ebae8d&selectedJob=30546785

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: part7: Turning off the last two intermittents.

Comment 26

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f16c7a63ffba51bfdbcf4ddba5b4153792b4b6

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c42cf92851
part7: Turning off some tests temporarily to enable e10s-multi 2. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f3bb541c86
part8: Few more tunr-offs. r=me

Comment 28

[Phil Ringnalda (:philor)]

Backed out part5 through part8 in https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f230913306b5ab63a64438c12f76e8fbde8c62 for the reasons in bug 1303113 comment 9

Comment 29

[Gabor Krizsanits (INACTIVE)]

(In reply to Phil Ringnalda (:philor) from comment #28)
> Backed out part5 through part8 in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e1f230913306b5ab63a64438c12f76e8fbde8c62 for the reasons in bug 1303113
> comment 9

Re-landed under bug 1303113

Comment 30

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.