1271244 - [e10s] About: shims don't work in all instances

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect, P1)

Description

[Mark Banner (:standard8)]

In bug 1247894 we've been trying to get Loop's about: definitions moved into the add-on. However, these aren't working in e10s mode.

STR

1) Build a browser with attachment 8746763 [details] [diff] [review]
2) Build the loop add-on from the branch on https://github.com/mozilla/loop/pull/360
3) Start Firefox, open the Hello panel, and either "Browse this page with a friend" or open an existing room.

The conversation window opens, but is transparent and there's this error on the console:

[Exception... "newChannel requires at least one of the 'loadingNode', 'loadingPrincipal', or 'loadUsingSystemPrincipal' properties on the options object."  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/RemoteAddonsParent.jsm :: AboutProtocolParent.openChannel :: line 268"  data: no]

We're working around this for Hello at the moment by not moving the about: definitions just yet, and when we do, we can turn off the multiprocess compatibility shims. However, if we can fix this earlier, it'd be good for us to get some additional advance testing.

This could also be affecting other add-ons.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I think this broke as the result of some of the new loadinfo changes. Fixing it the "right" way would require writing C++ code, which we're not willing to do for shims (it requires C++ code because LoadInfo objects can't be structured cloned across IPC boundaries, we'd need to use IPDL). I'll attach a patch to fix it in a dirty way.

That being said, it looks like Loop is already set up to have this work without the shims. The about: protocol handlers are installed in the parent and child processes and the needed chrome: pages are registered. In fact, if I disable the about: protocol handler shim, things seem to somewhat work (though there might be a problem detecting the camera/microphone). Mark, why does the loop add-on still have the shims enabled?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

This is ugly, but it allows us to load the about: page in the parent.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db01a638d94

Comment 3

[Mark Banner (:standard8)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #1)
> That being said, it looks like Loop is already set up to have this work
> without the shims. The about: protocol handlers are installed in the parent
> and child processes and the needed chrome: pages are registered. In fact, if
> I disable the about: protocol handler shim, things seem to somewhat work
> (though there might be a problem detecting the camera/microphone). Mark, why
> does the loop add-on still have the shims enabled?

We're currently compatible with 47 through 49, unfortunately 47 has various leaks related to e10s and if we turn off the shims, then we end up turning a lot of the tests orange.

Hence we're currently waiting for a 3 - 6 weeks, until 47 has gone out the door.

Comment 4

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

Comment on attachment 8753131 [details] [diff] [review]
Patch v1

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

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +260,5 @@
>      }
>  
>      let uri = BrowserUtils.makeURI(msg.data.uri);
>      let contractID = msg.data.contractID;
> +    let loadingPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

Can you explain why the old mechanism doesn't work anymore? Has serializing the nsIPrincipal over broken or something?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #4)
> Can you explain why the old mechanism doesn't work anymore? Has serializing
> the nsIPrincipal over broken or something?

We've changed LoadInfo so that we no longer have a loading principal for top-level (document) loads, so there isn't a loading principal anymore. The correct fix for this would be to write the shim in C++, where we could pass the loadinfo directly to the parent and use nsIIOService.newChannelFromURIWithLoadInfo in the parent. We can't create a top-level loadinfo from JS, so the easiest fix here is just to create an all-powerful loadinfo to slurp the data and pass it to the child (which will continue to use the correct loadinfo).

Comment 6

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

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #5)
> (In reply to Mike Conley (:mconley) - (needinfo me!) from comment #4)
> > Can you explain why the old mechanism doesn't work anymore? Has serializing
> > the nsIPrincipal over broken or something?
> 
> We've changed LoadInfo so that we no longer have a loading principal for
> top-level (document) loads, so there isn't a loading principal anymore. The
> correct fix for this would be to write the shim in C++, where we could pass
> the loadinfo directly to the parent and use
> nsIIOService.newChannelFromURIWithLoadInfo in the parent. We can't create a
> top-level loadinfo from JS, so the easiest fix here is just to create an
> all-powerful loadinfo to slurp the data and pass it to the child (which will
> continue to use the correct loadinfo).

Ah, okay, I understand.

Part of me is worried - though maybe it's unwarranted... add-ons are able to expose their about: URIs so that they're allowed to content. If some web content requests data from that about: URI, they'll end up making the request with the system principal in the parent process. Should that worry me?

If you can dissuade my above fears, the only other thing is that if you're not using loadingPrincipal, securityFlags or contentPolicyType from the message anymore, then the sender of the Addons:AboutProtocol:OpenChannel message should probably stop sending them.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I'm not sure. Jim, would you know? Or is there someone else who knows about plugins on OSX.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, wrong bug :-/

Comment 9

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

mrbkap's needinfo got cleared accidentally.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

After thinking about this, I realized I could do a little better.  TYPE_DOCUMENT loads are impossible to recreate in the parent, but for other types of loads (subdocuments, etc) we can actually create those loadinfos.

To some extent, we have to trust addon authors to not expose things they shouldn't via about: pages but I think with this setup, we're not going to allow anything too bad (at the very least, the documents we create in the child will have the right principals.

Comment 11

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

Comment on attachment 8757044 [details] [diff] [review]
Patch v2

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

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09bc7ad8065f
Hack around loadinfo to make the about: shim work. r=mconley

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09bc7ad8065f

Comment 14

[Chris Peterson [:cpeterson]]

wontfix for Beta 48