Help ridding tests of unsafe CPOWs

[Blake Kaplan]

Hello everybody,

I've been seeing a pattern of "unsafe" CPOWs causing our browser-chrome mochitests to go intermittently orange. Generally, it seems that a test randomly turns orange, gets starred and ignored until RyanVM or another one of our sheriffs gets tired of seeing it and needinfos somebody in the hopes of fixing it.

Rather than continue on this path, it would be good to just fix these unsafe CPOWs. They are already banned (via pref) in browser code proper for this very reason and we'd like to get rid of CPOWs entirely one day. To further that objective, I generated a list [1] of CPOW uses in a Linux mochitest run. The list has the filename of the test followed by a list of line numbers in the test file where we detected CPOW usage. If the CPOW was in a different file (i.e. head.js), that is listed below the test.

In general, most of these tests should be easy to fix either with ContentTask or using an API on <browser> that has already done the correct forwarding. If you would like examples, I've fixed several tests in [2] and [3]. I'm also always happy to answer questions or help out.

So, if you have a few spare cycles, please grab a test and fix it!

Thanks.

[1] https://docs.google.com/document/d/1EUUi3tMuhHsJA9iE9yNvjWcae4Unmqrl4uN2VwYxIqE/edit
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1304531
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1311197

--

Blake

[Gabriele Svelto]

Hi Blake,

On 19/10/2016 00:28, Blake Kaplan wrote:
> I've been seeing a pattern of "unsafe" CPOWs causing our browser-chrome
> mochitests to go intermittently orange. Generally, it seems that a test
> randomly turns orange, gets starred and ignored until RyanVM or another one
> of our sheriffs gets tired of seeing it and needinfos somebody in the hopes
> of fixing it.

I remember we did a big push in the War on Orange maybe three (or four?)
years ago. We could do it again; calling out to everybody to take
intermittent tests in the modules they're familiar with and start fixing
them. Personally I'd happily dedicate a chunk of my time to doing it for
a couple of weeks; having to re-trigger stuff every single time I make a
try run drives me nuts.

Gabriele

[Chris Hutten-Czapski]

Things that would help me help with this endeavour:

1: A bug to file patches against

2: A method for detecting if our fix actually fixes the problem

I presume a skeleton of what we're looking for is:

1) Use DXR/ls -r/whatever to find the test file in the tree

2) On the line number(s) mentioned, replace the existing use of a CPOW with something better. This may involve writing things in terms of ContentTask (see documentation here[X]), or by simply finding a non-CPOW-using alternative (like using browser.webNavigation.sessionHistory instead of browser.sessionHistory)

3) Run the test to make sure it still passes using ./mach test path/to/test.js

4) Write an informative commit message linking back to bug XXXX

5) Based on what kind of test it is, send it to try to make sure it isn't broken on other platforms

6) <INSERT VALIDATION OF CPOW REMOVAL STEP HERE>

7) Get the patch reviewed on bug XXXX

Is this correct?

:chutten

_______________________________________________
dev-platform mailing list
dev-pl...@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

[Dave Townsend]

I am working on a patch that takes care of most of the warnings in toolkit/mozapps/extensions/test/xpinstall in https://bugzilla.mozilla.org/show_bug.cgi?id=1311459

_______________________________________________
firefox-dev mailing list
firef...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev

[Blake Kaplan]

On Wed, Oct 19, 2016 at 2:00 AM, Gabriele Svelto <gsv...@mozilla.com> wrote:

I remember we did a big push in the War on Orange maybe three (or four?)
years ago. We could do it again; calling out to everybody to take
intermittent tests in the modules they're familiar with and start fixing
them. Personally I'd happily dedicate a chunk of my time to doing it for
a couple of weeks; having to re-trigger stuff every single time I make a
try run drives me nuts.

​Hi Gabriele,

Right now seems like an excellent time to mention that Joel Maher is working on this very question. I believe that he'll be hosting a session at the Hawaii all-hands about doing something very much like this.

--

Blake

[Blake Kaplan]

On Wed, Oct 19, 2016 at 10:19 AM, Chris Hutten-Czapski <chu...@mozilla.com> wrote:

Things that would help me help with this endeavour:

1: A bug to file patches against

​I debated this, but given the number of tests involved that seemed like a bunch of overhead. I've been filing bugs as I fix tests.​

 

2: A method for detecting if our fix actually fixes the problem

​I should have included this. What I do (and it isn't perfect but hopefully not too bad, either) is to write my patch and then run |MOZ_CPOW_LOG=stacks mach mochitest <test>.js|. That will give you stacks for all CPOWs, which look like [1]:

INFO -  #0 0x7f06ecc22140 i   chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_bug1025195_switchToTabHavingURI_aOpenParams.js:18 (0x7f06d04dc098 @ 21)
INFO -  #1 0x7fff49c5cab0 b   resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:388 (0x7f06ecfed808 @ 254)
INFO -  #2 0x7f06ecc220a8 i   chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_bug1025195_switchToTabHavingURI_aOpenParams.js:17 (0x7f06d13f6f78 @ 252)
INFO -  #3 0x7f06ecc22020 i   self-hosted:1206 (0x7f06c747dc48 @ 24)

Note that if the stack includes RemoteAddonsParent.jsm, then the CPOW use is almost certainly from a shim. My list explicitly filtered those out since they shouldn't intermittently go orange because the shims use CPOWs safely (they block the content process). That being said, I've been tending to convert shim uses to BrowserTestUtils anyway. One extremely common shim that I've seen used is |browser.addEventListener("load", ...)|. These shim uses can usually be trivially converted to use |BrowserTestUtils.browserLoaded(browser)|.

​[1] ​http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/base/content/test/urlbar/browser_bug1025195_switchToTabHavingURI_aOpenParams.js#18

 

I presume a skeleton of what we're looking for is:

1) Use DXR/ls -r/whatever to find the test file in the tree

​Yeah, test files in my experience are unique in the tree.​ I use |find . name ...|.

2) On the line number(s) mentioned, replace the existing use of a CPOW with something better. This may involve writing things in terms of ContentTask (see documentation here[X]), or by simply finding a non-CPOW-using alternative (like using browser.webNavigation.sessionHistory instead of browser.sessionHistory)

​We don't have a ton of documentation. ​https://wiki.mozilla.org/Electrolysis/e10s_test_tips#Browser-chrome is probably the best we have.

 

3) Run the test to make sure it still passes using ./mach test path/to/test.js

4) Write an informative commit message linking back to bug XXXX

5) Based on what kind of test it is, send it to try to make sure it isn't broken on other platforms

6) <INSERT VALIDATION OF CPOW REMOVAL STEP HERE>

7) Get the patch reviewed on bug XXXX

Is this correct?

Yep.​

--

Blake