Help ridding tests of unsafe CPOWs

20 views
Skip to first unread message

Blake Kaplan

unread,
Oct 18, 2016, 6:28:15 PM10/18/16
to dev-pl...@lists.mozilla.org, firef...@mozilla.org
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

unread,
Oct 19, 2016, 10:32:23 AM10/19/16
to Blake Kaplan, dev-pl...@lists.mozilla.org, firef...@mozilla.org
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

signature.asc

Chris Hutten-Czapski

unread,
Oct 19, 2016, 1:19:51 PM10/19/16
to Gabriele Svelto, dev-platform, Blake Kaplan, Firefox Dev
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

unread,
Oct 19, 2016, 1:49:40 PM10/19/16
to Blake Kaplan, dev-pl...@lists.mozilla.org, Firefox Dev
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

unread,
Oct 19, 2016, 8:13:00 PM10/19/16
to Gabriele Svelto, Joel Maher, dev-pl...@lists.mozilla.org, firef...@mozilla.org
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

unread,
Oct 19, 2016, 8:35:26 PM10/19/16
to Chris Hutten-Czapski, Gabriele Svelto, dev-platform, Firefox Dev
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)|.


 
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
Reply all
Reply to author
Forward
0 new messages