1277410 - Add a protocol flag to say whether a protocol can load in a child process

Status: RESOLVED WONTFIX

(Core :: Networking, defect, P3)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Right now, protocols created by extensions (even e10s compatible extensions) have no control over whether they load in the parent or child process. We can add a flag to let them control whether or not they want to load in the parent or child.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

The E10SUtils stuff was reviewed by billm in bug 1095484.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 3

[Honza Bambas (:mayhemer)]

Blake, are you aware of bug 590682?  AFAIK (and as I've tested), add-on registered protocol can't be instantiated (are not registered) on content processes.  Unless something has changed recently?

Bug 590682 allows that by automatic (generic) proxying to the parent process.  It doesn't introduce any new protocol flags, since we only start this generic IPC channel when we don't find a handler for a protocol.

That also includes external protocols like mailto: that don't work when redirected to in e10s (bug 1277028).

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Blake, are you aware of bug 590682?  AFAIK (and as I've tested), add-on
> registered protocol can't be instantiated (are not registered) on content
> processes.  Unless something has changed recently?

They can be instantiated, but you have to do it manually (and programmatically) via a frame script in the child process. It isn't pretty, but it does work. My idea for this bug was that an extension that has some very complicated protocol handler that requires the load to be in the parent (so it could interact with some XUL elements or something weird) could fine-tune where it loads.

That being said I don't think this is something we absolutely *must* do. It was an idea and I had a patch lying around, so I figured I'd file a bug.

> Bug 590682 allows that by automatic (generic) proxying to the parent
> process.  It doesn't introduce any new protocol flags, since we only start
> this generic IPC channel when we don't find a handler for a protocol.

Out of curiosity, do we end up with a history entry for e.g. irc: links for link clicks in the child process?

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #4)
> Out of curiosity, do we end up with a history entry for e.g. irc: links for
> link clicks in the child process?

I never tried, but I'd expect so.  With my patch it then behaves like in the non-e10s world.

Comment 6

[Jason Duell]

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

I think Honza is a better reviewers for this at this point.

Comment 7

[Jim Mathies [:jimm]]

blake, does this need to block/get uplifted?

Comment 8

[Honza Bambas (:mayhemer)]

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

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

I'm not deeply familiar with E10SUtils.jsm and how it's being used.  But it seems from looking over into to the code, that this will only affect top-level loads (top level documents and iframes).

Hence, here is a question: how will this affect inline content loads? (<img src=>, <script src=>, <rel link=>)  Also, I don't think it will have any impact when a page (top-level) redirects to a custom URI.

But if there are plans for system of followups, I'll gladly r+ this as a base patch.

::: browser/modules/E10SUtils.jsm
@@ +43,5 @@
>      let mustLoadRemote = true;
>  
> +    let scheme = aURL.substring(0, schemeEnd);
> +    switch (scheme) {
> +      case "javascript":

nit: maybe keep the schemes in this switch alphabetically sorted?  or are you respecting performance here (most used first?)

@@ +49,5 @@
> +        return true;
> +
> +      case "view-source":
> +        // For view-source URIs, try again with the nested URI.
> +        return this.canLoadURIInProcess(aURL.substr("view-source:".length), aProcess);

"view-source:".length == schemeEnd + 1, doesn't it?

::: netwerk/base/nsIProtocolHandler.idl
@@ +302,5 @@
>      const unsigned long ORIGIN_IS_FULL_SPEC = (1 << 20);
> +
> +    /**
> +     * If this flag is not set, then we will refuse to load this protocol
> +     * handler in child processes.

and what happens when it is set? (the line is just an implication)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

This doesn't need to block. It's just an idea that we can take or leave.

Comment 10

[Honza Bambas (:mayhemer)]

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

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

Personally I can give r+ only to the protocol handler changes.  As I said earlier, I'm not aware of how E10SUtils.jsm works, I can't really review it.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Before I land this, I think we should meet to make sure this fits into Necko's plans for addon API.

Comment 12

[Cameron Kaiser [:spectre]]

Bug 1296885 was filed by me unaware of this one to discuss the WebExtensions case.

Comment 13

[Cameron Kaiser [:spectre]]

(However, I'd still like to ask for this or something like it to get landed as an interim step since this is something these addons cannot do for themselves right now under e10s. I'd also be fine with a smaller and less invasive transitional patch where an addon must opt-in to running in the parent, since odds are we'd have to do other plumbing changes to the add-on anyway. I can come up with something like that, or we could go with the one above, or ...)

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

This is jduell's call, I think.

Comment 15

[Jason Duell]

I'm honestly not sure if this patch would be useful or not. Most of the data we've retrieved shows that most custom protocols do work in e10s, as is, and I'm not sure how it will help the ones that don't (overbite).  But Cameron seems to think it would, so I'm not opposed to landing this.  I'm just not sure how it helps, exactly, so I'd like to know more.

> Right now, protocols created by extensions (even e10s compatible extensions) have 
> no control over whether they load in the parent or child process.

So what do we do now?  I've been assuming addons run in the parent, and that the protocol only gets registered in the parent (but that's from looking at only one or two addons).  Is there already some other way for addons to register whether they should run in the parent/child?  If so I'm not sure we need another frob here for that.

Comment 16

[Cameron Kaiser [:spectre]]

I thought the issue was that, yes, the protocol and the add-on are in the parent, but the *load* occurs in the *child* which does *not* have the protocol handler loaded. The idea here was to prevent that bifurcation, which I don't know of any way an add-on can work around presently. Am I mistaken?

> Is there already some other way for addons to register whether they should run
> in the parent/child?  If so I'm not sure we need another frob here for that.

If there is, I'll gladly use it; point me to it.

Comment 17

[Honza Bambas (:mayhemer)]

Extensions are capable of registering a protocol in child processes when they want and then IPC to a job on the parent.  We offer support [1] for that and I've seen this in few addons already (e.g. Blur addon).

(In reply to Cameron Kaiser [:spectre] from comment #13)
> patch where an addon must opt-in to running in the
> parent, 

I don't follow this, addons currently do run on parent.

I'm closing this bug for now, but if I'm mistaken, please reopen with explanation why would need this.


[1]

// For the protocol handler
var child = require("sdk/remote/child");
child.process.port.on("init", function() {
	register_my_protocol();
});

// On the parent
var parent = require("sdk/remote/parent");
parent.processes.forEvery(function(e) {
        e.port.emit("init")
});