1412456 - Remove add-on interposition

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 990729 is where we added all this stuff.

Comment 2

[Kris Maglione [:kmag]]

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

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 4

[Kris Maglione [:kmag]]

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

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Send document CPOW as well as window CPOW

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Change browser element's primary attribute immediately when tab change requested

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Add BrowserTestUtils.addContentEventListener

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.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: devtools test changes

This patch has changes to all the devtools tests.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: browser/ test changes

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

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: misc test changes

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

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: onbeforeunload test change

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

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: allow use of CPOWs without interposition

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 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: stop running addoncompat tests

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: disable interposition

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8934349 [details] [diff] [review]
Send document CPOW as well as window CPOW

Review of attachment 8934349 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 16

[:Gijs (he/him)]

Comment on attachment 8934355 [details] [diff] [review]
onbeforeunload test change

Review of attachment 8934355 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, thanks!

Comment 17

[Brian Grinstead [:bgrins]]

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 18

[Mike Conley (:mconley) (:⚙️)]

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 19

[Mike Conley (:mconley) (:⚙️)]

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 20

[Mike Conley (:mconley) (:⚙️)]

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 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8934353 [details] [diff] [review]
browser/ test changes

Review of attachment 8934353 [details] [diff] [review]:
-----------------------------------------------------------------

wow, what a huge chunk of work!

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 24

[Dorel Luca [:dluca]]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Andreea Pavel [:apavel]]

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

Comment 27

[Andreea Pavel [:apavel]]

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

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 31

[Cosmin Sabou [:CosminS]]

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

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 33

[[github robot]]

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