Status

()

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: billm, Assigned: kmag)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

3.24 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.82 KB, patch
mconley
: review+
Details | Diff | Splinter Review
7.17 KB, patch
mconley
: review+
Details | Diff | Splinter Review
93.39 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
93.44 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
38.44 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.73 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.43 KB, patch
aswan
: review+
Details | Diff | Splinter Review
946 bytes, patch
mconley
: review+
Details | Diff | Splinter Review
1.14 KB, patch
mconley
: review+
Details | Diff | Splinter Review
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.
Assignee

Comment 2

2 years ago
(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.
Assignee

Comment 4

2 years ago
(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.

Updated

2 years ago
See Also: → 1419623
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.
Attachment #8934349 - Flags: review?(mconley)
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.
Attachment #8934350 - Flags: review?(mconley)
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.
Attachment #8934351 - Flags: review?(mconley)
This patch has changes to all the devtools tests.
Attachment #8934352 - Flags: review?(bgrinstead)
This patch has changes to all the browser/ tests. Trying to spread out these reviews a bit.
Attachment #8934353 - Flags: review?(felipc)
This patch has changes to dom, docshell, and toolkit tests.
Attachment #8934354 - Flags: review?(mrbkap)
This test was the most complex by far to change. Asking for a special review just for this one.
Attachment #8934355 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8934356 - Flags: review?(aswan)
Comment on attachment 8934349 [details] [diff] [review]
Send document CPOW as well as window CPOW

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

Thanks!
Attachment #8934349 - Flags: review?(mconley) → review+

Comment 16

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

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

Nice work, thanks!
Attachment #8934355 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8934352 - Flags: review?(bgrinstead) → review+
Attachment #8934350 - Flags: review?(mconley) → review+
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?
Attachment #8934357 - Flags: review?(mconley) → review+
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?
Attachment #8934358 - Flags: review?(mconley) → review+
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?
Attachment #8934351 - Flags: review?(mconley) → review+
Comment on attachment 8934353 [details] [diff] [review]
browser/ test changes

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

wow, what a huge chunk of work!
Attachment #8934353 - Flags: review?(felipc) → review+
(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.
Attachment #8934356 - Flags: review?(aswan) → review+
Attachment #8934354 - Flags: review?(mrbkap) → review+

Comment 23

2 years ago
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)

Comment 25

2 years ago
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
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

2 years ago
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)
It doesn't look like I'll be able to finish this. Assigning to Kris at his request.
Assignee: wmccloskey → kmaglione+bmo
Blocks: 1404885
No longer blocks: 1392352
Assignee

Updated

a year ago
Depends on: 1443964
Assignee

Updated

a year ago
Depends on: 1443983

Updated

a year ago
Blocks: 1444973
Assignee

Updated

a year ago
Depends on: 1445551

Updated

11 months ago
No longer blocks: 1404885
Assignee

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.