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

RESOLVED FIXED

Status

()

RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s-multi:M1])

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → gkrizsanits
Blocks: 1207306
Whiteboard: [e10s-multi:M1]

Updated

2 years ago
Blocks: 1303113
(Assignee)

Comment 1

2 years ago
Created attachment 8792947 [details] [diff] [review]
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.
Attachment #8792947 - Flags: review?(mrbkap)

Updated

2 years ago
Attachment #8792947 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 6

2 years ago
Created attachment 8799862 [details] [diff] [review]
part2
Attachment #8799862 - Flags: review?(mrbkap)
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.
Attachment #8799862 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 9

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

Comment 11

2 years ago
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
(Assignee)

Comment 12

2 years ago
Created attachment 8806676 [details] [diff] [review]
part3: Force single content process for tests that times out.
Attachment #8806676 - Flags: review?(mrbkap)
(Assignee)

Comment 13

2 years ago
Created attachment 8806677 [details] [diff] [review]
part4: Force single content process for failing tests.
Attachment #8806677 - Flags: review?(mrbkap)
(Assignee)

Comment 14

2 years ago
Created attachment 8806678 [details] [diff] [review]
part5: Turning off some tests temporarily.
Attachment #8806678 - Flags: review?(mrbkap)
(Assignee)

Comment 15

2 years ago
Created attachment 8806680 [details] [diff] [review]
part5: Turning off some tests temporarily. v2
Attachment #8806678 - Attachment is obsolete: true
Attachment #8806678 - Flags: review?(mrbkap)
Attachment #8806680 - Flags: review?(mrbkap)

Updated

2 years ago
Attachment #8806676 - Flags: review?(mrbkap) → review+

Updated

2 years ago
Attachment #8806677 - Flags: review?(mrbkap) → review+
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.
Attachment #8806680 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

2 years ago
Blocks: 1315042

Comment 17

2 years ago
Pushed by gkrizsanits@mozilla.com:
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
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?
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 19

2 years ago
(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.
Flags: needinfo?(gkrizsanits)
(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 22

2 years ago
Pushed by gkrizsanits@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb45e90f6596
part6: Disabled test should still run in non-e10s mode. r=me
(Assignee)

Comment 24

2 years ago
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
(Assignee)

Comment 25

2 years ago
Created attachment 8808101 [details] [diff] [review]
part7: Turning off the last two intermittents.
Attachment #8808101 - Flags: review?(mrbkap)

Updated

2 years ago
Attachment #8808101 - Flags: review?(mrbkap) → review+

Comment 27

2 years ago
Pushed by gkrizsanits@mozilla.com:
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
(Assignee)

Comment 29

2 years ago
(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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.