Now that we have deprecated legacy add-ons, we no longer need the add-on interposition code that was added for e10s. This code is very complex and quite slow. The one hitch is that we still use interposition a lot for tests. My plan is to rewrite our tests to make explicit use of CPOWs instead of interposition. So, for example, |content.document.querySelector| becomes |gBrowser.contentDocumentAsCPOW.querySelector|. In general, searching for "AsCPOW" should be sufficient to find all CPOW usage. Once the rewrite is complete, we can turn off the interposition pref and then start removing the code. A related issue is what to do about the compartment-per-addon code we have. This is also complex and slow. Even when interposition is disabled, we'll still rely on it to decide whether CPOWs should be blocked or not (since they're forbidden in browser code). I think it will make sense to individually enable a pref to allow CPOWs in each test that uses them. Once that's done, we can get rid of compartment-per-addon as well. Removal of CPOWs themselves is still a long way off, unfortunately.

Bug 990729 is where we added all this stuff.

(In reply to Bill McCloskey (:billm) from comment #0) > A related issue is what to do about the compartment-per-addon code we have. > This is also complex and slow. Even when interposition is disabled, we'll > still rely on it to decide whether CPOWs should be blocked or not (since > they're forbidden in browser code). I don't think we need most of the compartment-per-addon code at this point. System add-ons and tests shouldn't use overlays, so we shouldn't need the overlay sandboxing code for anything. Tagging sandboxes and module scopes with add-on IDs might still be useful, but is also relatively simple. It might be worth doing a talos run with it turned off to get an idea what the perf cost is, though (although that would have the side-effect of loading system add-on modules into the shared module global, which would definitely make a lot of things faster). As far as tests go, we should probably be able to just rely on Cu.permitCPOWsInScope for the tests that need it, and probably only enable it when running in automation.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2) I think we're in agreement? My plan is to remove compartment-per-addon shortly after this bug is done. However, it will involve adding an annotation of some sort to each test that uses CPOWs. I'm not sure it matters that much whether the annotation is a pref or Cu.permitCPOWsInScope.

(In reply to Bill McCloskey (:billm) from comment #3) > I think we're in agreement? > Sorry, yes, I think we're in agreement. > My plan is to remove compartment-per-addon shortly after this bug is done. > However, it will involve adding an annotation of some sort to each test that > uses CPOWs. I'm not sure it matters that much whether the annotation is a pref > or Cu.permitCPOWsInScope. I think we should just call Cu.permitCPOWsInScope in the tests that need them.

Currently, if you try to use contentDocumentAsCPOW, you'll get an exception saying that browser code is not allowed to use CPOWs. That's because we cleverly tried to get the CPOW via contentWindowAsCPOW.document. However, this property access happens inside remote-browser.xul, where CPOWs are forbidden. So it doesn't work.

Currently the e10s tab switcher doesn't set the primary="true" attribute on a browser element until the tab switcher is destroyed. This means that using the |content| global is unreliable in e10s since it may take as long as the tab switcher's unload delay before the attribute changes. Normally this doesn't matter since |content| isn't useful in e10s. However, tests that use tabs like about:preferences rely on |content|, and there isn't a good reason to fix them. So let's just change the attribute immediately upon changing tabs.

This function makes it possible to listen for multiple events from the content process, even when there are frameloader swaps. This commit also adds a checkFn param to firstBrowserLoaded, which is useful.

This patch has changes to all the devtools tests.

This patch has changes to all the browser/ tests. Trying to spread out these reviews a bit.

This patch has changes to dom, docshell, and toolkit tests.

This test was the most complex by far to change. Asking for a special review just for this one.

I want to allow an add-on (specifically the mochitest add-on) to be able to use CPOWs even when interposition is disabled. This patch makes that possible. I don't remember why these were tied before. Probably an oversight.

Comment on attachment 8934349 [details] [diff] [review] Send document CPOW as well as window CPOW Review of attachment 8934349 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8934355 [details] [diff] [review] onbeforeunload test change Review of attachment 8934355 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, thanks!

Comment on attachment 8934352 [details] [diff] [review] devtools test changes Review of attachment 8934352 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good. The only complication I'm aware of on our end is we are actively working to fix and enable tests for the webconsole/new-console-frontend/test/mochitest/ directory (files that are copied over from webconsole/test/) in Bug 1400847. Since those files were copied before this patch we'll need to make the same changes to them as they get enabled one by one. But we can follow this pattern for those.

Comment on attachment 8934357 [details] [diff] [review] stop running addoncompat tests Review of attachment 8934357 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/addoncompat/moz.build @@ +5,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > with Files('**'): > BUG_COMPONENT = ('Firefox', 'Extension Compatibility') > Should we file a follow-up to remove these tests entirely from the tree? Or can we just get rid of them now?

Comment on attachment 8934358 [details] [diff] [review] disable interposition Review of attachment 8934358 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Along with the tests, should we be filing follow-up bugs to remove the interposition code entirely?

Comment on attachment 8934351 [details] [diff] [review] Add BrowserTestUtils.addContentEventListener Review of attachment 8934351 [details] [diff] [review]: ----------------------------------------------------------------- Neat, thanks! ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +307,3 @@ > * A newly opened window for which we're waiting for the > * first browser load. > + * @param {Boolean} aboutBlank Should add [optional] here. @@ +308,5 @@ > * first browser load. > + * @param {Boolean} aboutBlank > + * If false, about:blank loads are ignored and we continue > + * to wait. > + * @param {function or null} checkFn Should perhaps add [optional] here. @@ +847,5 @@ > + * > + * @param {xul:browser} browser > + * The browser element to listen for events in. > + * @param {string} eventName > + * Name of the event to listen to. We're missing documentation for listener @@ +851,5 @@ > + * Name of the event to listen to. > + * @param {bool} useCapture [optional] > + * Whether to use a capturing listener. > + * @param {function} checkFn [optional] > + * Called with the Event object as argument, should return true if the Maybe we should be explicit here that this function is called within the content process, and has no access to its closure. @@ +913,5 @@ > + } > + } > + mm.addMessageListener("ContentEventListener:Run", runListener); > + > + let needCleanup = true; This doesn't ever appear to be used. Should we be setting this to true the first time unregisterFunction is called?

Comment on attachment 8934353 [details] [diff] [review] browser/ test changes Review of attachment 8934353 [details] [diff] [review]: ----------------------------------------------------------------- wow, what a huge chunk of work!

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #19) > Comment on attachment 8934358 [details] [diff] [review] > disable interposition > > Review of attachment 8934358 [details] [diff] [review]: > ----------------------------------------------------------------- > > \o/ > > Along with the tests, should we be filing follow-up bugs to remove the > interposition code entirely? Don't worry. It will be a joy to remove all this code. I'll get to it as soon as this lands.

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9fb71f1e8e Send document CPOW as well as window CPOW (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/a7aec2ce903b Change browser element's primary attribute immediately when tab change requested (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4506e124ac Add BrowserTestUtils.addContentEventListener (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/e56a1ba26b7c Test changes to no longer use interposition (r=felipe,bgrins,mrbkap) https://hg.mozilla.org/integration/mozilla-inbound/rev/c7698410ddbd Fix onbeforeunload test to no longer rely on interposition (r=Gijs) https://hg.mozilla.org/integration/mozilla-inbound/rev/039e980b7dc6 Allow CPOWs without interposition (r=aswan) https://hg.mozilla.org/integration/mozilla-inbound/rev/49b93f807db0 Stop running addoncompat tests (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/0e88de036c55 Disable add-on interposition (r=mconley)

Backed out 8 changesets (bug 1412456) for ESlint failure on browser_urlbarKeepStateAcrossTabSwitches.js:13:49 r=backout on a CLOSED TREE https://treeherder.mozilla.org/logviewer.html#?job_id=150411155&repo=mozilla-inbound&lineNumber=263 https://hg.mozilla.org/integration/mozilla-inbound/rev/41db161924728390bae3bfdd3d2f8838b267937b https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0e88de036c5572d8e2ea0e646e52341ac71330f8

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13a637602dc2 Send document CPOW as well as window CPOW (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/736f38486832 Change browser element's primary attribute immediately when tab change requested (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/d85af60fe259 Add BrowserTestUtils.addContentEventListener (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/f100d953f9eb Test changes to no longer use interposition (r=felipe,bgrins,mrbkap) https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ff1050c589 Fix onbeforeunload test to no longer rely on interposition (r=Gijs) https://hg.mozilla.org/integration/mozilla-inbound/rev/602b30ac3c69 Allow CPOWs without interposition (r=aswan) https://hg.mozilla.org/integration/mozilla-inbound/rev/f35ec2a884f8 Stop running addoncompat tests (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/27077db47231 Disable add-on interposition (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/0c01a98f4fd5 Fix ESLint failures

Backed out 9 changesets (bug 1412456) for crashing talos g2 and unexpected network connections in browser-chrome's browser_searchEngine_behaviors.js r=backout a=backout on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7223d0316537bdb463a5c5b602cde660532600e5 Failure logs: g2 talos: https://treeherder.mozilla.org/logviewer.html#?job_id=150433729&repo=mozilla-inbound&lineNumber=1633 unexpected network connections in browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=150436154&repo=mozilla-inbound&lineNumber=3343 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2201664e3c6e109936786d6f74f40a28b5cf89d

Sorry, wrong link for the unexpected network connections. Failure log network connections: https://treeherder.mozilla.org/logviewer.html#?job_id=150415594&repo=mozilla-inbound&lineNumber=7034 Failure log redirect: https://treeherder.mozilla.org/logviewer.html#?job_id=150436154&repo=mozilla-inbound&lineNumber=3343

Another possible issue: https://treeherder.mozilla.org/logviewer.html#?job_id=150436084&repo=mozilla-inbound&lineNumber=3339

I'll try to land all the test changes except for the search engine stuff. Then we can disable interposition once I figure out the Talos thing (which appears to be preexisting, but much less frequent without my patches).

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/211c791a4c7f Send document CPOW as well as window CPOW (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/33d4345bf5fb Add BrowserTestUtils.addContentEventListener (r=mconley) https://hg.mozilla.org/integration/mozilla-inbound/rev/e26ae8ac1f0f Test changes to no longer use interposition (r=felipe,bgrins,mrbkap) https://hg.mozilla.org/integration/mozilla-inbound/rev/eafdd98ac04f Fix onbeforeunload test to no longer rely on interposition (r=Gijs) https://hg.mozilla.org/integration/mozilla-inbound/rev/3be45666b1b8 Allow CPOWs without interposition (r=aswan) https://hg.mozilla.org/integration/mozilla-inbound/rev/f3dc5ed08890 Change browser element's primary attribute immediately when tab change requested (r=mconley)

https://hg.mozilla.org/mozilla-central/rev/211c791a4c7f https://hg.mozilla.org/mozilla-central/rev/33d4345bf5fb https://hg.mozilla.org/mozilla-central/rev/e26ae8ac1f0f https://hg.mozilla.org/mozilla-central/rev/eafdd98ac04f https://hg.mozilla.org/mozilla-central/rev/3be45666b1b8 https://hg.mozilla.org/mozilla-central/rev/f3dc5ed08890

It doesn't look like I'll be able to finish this. Assigning to Kris at his request.

Commit pushed to master at https://github.com/mozilla/normandy https://github.com/mozilla/normandy/commit/9234b6949f79405ab17af67a1454ea30ebb7a450 Bug 1412456 - Test changes to no longer use interposition r=felipe