Closed Bug 1301340 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

Product:
Core
Component:
DOM: Content Processes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

Details

(Whiteboard: [e10s-multi:M1])

Attachments

(6 files, 1 obsolete file)

Force single content process for some tests
5.35 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
part2
6.94 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
part3: Force single content process for tests that times out.
3.71 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
part4: Force single content process for failing tests.
13.61 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
part5: Turning off some tests temporarily. v2
7.00 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
part7: Turning off the last two intermittents.
1.68 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review 
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: nobody → gkrizsanits
Blocks: e10s-multi
Whiteboard: [e10s-multi:M1]
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)
Attachment #8792947 - Flags: review?(mrbkap) → review+
Keywords: leave-open
Attached patch part2 — — Splinter Review 

        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+

    
    

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

    
    

    
  
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

    
    

    
  


  

        Attachment #8806678 -
        Flags: review?(mrbkap)

    
    

    
  


  

        Attachment #8806678 -
        Attachment is obsolete: true

        Attachment #8806678 -
        Flags: review?(mrbkap)

        Attachment #8806680 -
        Flags: review?(mrbkap)

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

        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+

    
  
Blocks: 1315042

    
    

    
  
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)

    
    

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

    
    

    
  
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

    
    

    
  
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

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

    
    

    
  
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

    
    

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