Closed
Bug 590682
Opened 14 years ago
Closed 8 years ago
e10s: Have a universal child<->parent protocol wrapper for any nsIChannel implementation that works only on parent process
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
e10s | later | --- |
People
(Reporter: jduell.mcbugs, Unassigned)
References
Details
(Keywords: addon-compat, Whiteboard: [necko-backlog])
Attachments
(2 files, 6 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
Apparently it's not uncommon for extensions to create their own protocols (ex: <href="myprotocol:foo>) that wind up using HTTP (or possibly FTP) to do transport. We probably need to support this in e10s.
(Note that this is different that an extension trying to provide its own custom implementation of HTTP, which I doubt anyone ever did, and which we're no longer supporting.)
bz is under the impression that many of these extensions simply wind up re-writing to some HTTP URI, in which case we should have no work to do. But he points out that they could also potentially |wrap| an underlying HTTP/FTP channel (i.e. implement nsIHttpChannel, eventually forwarding network traffic to a regular Mozilla-provided HTTP channel).
In the "wrapper" case we need to make sure we don't do things like a static_cast to a concrete class like HttpChannelChild if we're not positive that the nsIChannel/nsIHttpChannel/etc we're dealing with isn't a custom protocol.
Attached is an IRC chat where we discussed. Thinking about it some more, I don't think we have any problem if the extension is living on chrome (it'll wrap an nsHttpChannel just as before). And on content, we only have one relevant static_cast, in redirect1Begin (to the new PHttpChannel HttpChannelChild), and it shouldn't be possible for that to be a custom protocol channel. So this may all work already.
It would be good to 1) confirm that we care about this bug; and 2) have some examples that we can test to confirm is this already works or need changes.
Hanging off e10s/Firefox meta-bug: move to fennec if we'll need it there sooner.
bz pointed out to me that while we may no longer care about supporting custom HTTP channel implementations, we may still want to support custom protocols that *wrap* Http (i.e. wind up using an HTTP channel to do transport). Apparently there are "a lot of extensions" (bz) that do this (bz: have examples?), putting things like <href="myprotocol:foo> in web pages, then using HTTP underneath.
In e10s we've added a number of static_casts that assume that an nsIHttpChannel passed into OnStart/Data/Stop, etc. is an nsHttpChannel: this will barf if the channel is actually a custom channel that wraps an nsHttpChannel. ggOn IRC bz and I came up with one possible solution: a new nsIChannel readonly attribute ("coreChannel"?) that when read is guaranteed to provide the underlying HTTP (or FTP) channel that's being used. Or I suppose we could require that when custom classes call their underlying channel's
Reporter | ||
Comment 1•14 years ago
|
||
Ignore last two paragraphs in previous comment--churn.
Comment 2•14 years ago
|
||
I'm not sure we care about this bug in its current form. I think that if we want to support custom protocol handlers, we should have an API for actually doing it in JS a-la jetpack.
As it stands, extensions can't really load XPCOM stuff into the content process anyway.
Comment 3•14 years ago
|
||
> I think that if we want to support custom protocol handlers
I think we do; it's a very common thing for extensions to implement.
> And on content, we only have one relevant static_cast
We were going to add more for the helper app + download stuff, I thought.
> As it stands, extensions can't really load XPCOM stuff into the content
> process anyway.
As in, no custom protocols possible? Yeah, we need to fix that somehow (either by proxying to chrome before creating "real" channels for everything or something).
Comment 4•14 years ago
|
||
Note that we ship a protocol handler as part of our browser which wraps HTTP in exactly the way I describe: the view-source protocol handler.
Comment 5•14 years ago
|
||
What about to have a universal protocol handler on the child, that would, in case a protocol implementation had not been found, instantiate a universal implementation of nsIChannel/nsIRequest that would forward AsyncOpen/OnRedirect+OnVerify/OnStart+StopRequest/OnDataAvailable + other properties between content and chrome? URI can be created as nsISimpleURI and carried this way on content, and to chrome as string. And when there is no implementation on the chrome, return NS_NOT_IMPLEMENTED in OnStopRequest. This all has already been made for http protocol, so we know it is doable.
This could also be used to solve some part of bug 591707. The universal protocol can be instantiated in both ways (from content, as well as from chrome). If we want to redirect from http->ftp, we use the universal protocol for ftp.
This probably means to generalize the redirect code we have now, but, as I understand, there is not possible to let a protocol inherit other protocol(s), sad. So, we probably need a kind of an abstraction bridge (a new IPDL protocol, implemented by each protocol) that is passed as a new channel referrer to the redirect callback to carry the real channel to the other side, behaving as an abstract class. Extensions do not have to implement this bridge for their new protocols, because the universal protocol (and its bridge) will be used.
This schema can be used for any protocol we want to refer in content. E.g., I'm not sure of state of ftp - is it networking on the chrome or on the content? If on the content, this schema can be used to make ftp do networking on chrome.
Makes sense?
Comment 6•14 years ago
|
||
- new nsIProtocolHandler flag DONT_OVERRIDE_ON_CHILD_PROCESS
- when exposed by the protocol, a generic channel is not used on the child process to simulate protocol's channel implementation
- when not exposed, we instantiate GenericChannelChild (derived from nsBaseChannel) which is in most ways similar to FTP IPC protocol
- GenericChannelParent is then used to instantiate new or connect existing channel on the parent process and forward notifications to the child process
This is a template how this could work, however for most mozilla protocols this is simply not enough. File protocol e.g., if we'll decide to remote it, needs to have its own IPC protocol.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #493700 -
Flags: feedback?(jduell.mcbugs)
Comment 7•14 years ago
|
||
I'm sure this is a stupid question, but as a potential consumer of this interface (for a future OverbiteFF), would the idea be that I would create an implementation -- hopefully in JavaScript -- of nsIChildChannel in my add-on that this would forward to?
Comment 8•14 years ago
|
||
(In reply to comment #7)
> I'm sure this is a stupid question, but as a potential consumer of this
> interface (for a future OverbiteFF), would the idea be that I would create an
> implementation -- hopefully in JavaScript -- of nsIChildChannel in my add-on
> that this would forward to?
Not exactly. AFAIK, extensions' code (js) will be running only on the parent process in context of the chrome. nsIChildChannel implementation is for the child process. For OverbiteFF and similar extensions you don't need to do anything! Or better say, it is the goal of this bug.
Please keep stick with this bug and keep testing your extensions with the patches. There might be updates as this one is a base only.
In a nut shell this patch does:
- when a content requests access to an URI (load via URL bar or by a link click, js open()), for protocols that do not implement nsIChildChannel creates a generic IPC bridge between content and chrome process
- the protocol (written in JS or C) is then instantiated and run on the chrome process
The generic channel (nsIChildChannel implementation) is currently able only to:
- collect properties (nsIPropertyBag) on content process
- call asyncOpen, cancel, suspend, resume from content to chrome
- return onStartRequest, onDataAvailable and onStopRequest from chrome to content
- carry securityInfo object from chrome to content (in a limited way)
- protocols that support redirecting (like http) may be redirected to it
The generic channel may be enhanced of:
- nsIProgressEventSink implementation
- cache entries access (if that makes sense)
Anything more specific will probably need to have its own (C++) implementation for the particular protocol. I'm not sure how well this is going to be allowed for extension, though.
Comment 9•14 years ago
|
||
Excellent! Gopher is about as simple a protocol as you get, so I would definitely like to test with this. At least for that use case, based on your description this should suffice for me.
Comment 10•13 years ago
|
||
Jason: ping.
There was a long time a big silence here. Do we still need this functionality? I think it is still missing in Fennec, but I'm not entirely sure about the status here.
Reporter | ||
Comment 11•13 years ago
|
||
Yes, I'm pretty sure we still need this (though it seems to be a low priority--no one's bumped into it yet on fennec).
bz: can you point me at any extensions that you know wrap HTTP like this? It'd be nice to have a test case.
Comment 12•13 years ago
|
||
> bz: can you point me at any extensions that you know wrap HTTP like this?
Not offhand, sorry. I just recall people asking questions about it in the newsgroups over the years, I know they were doing it. Whether the results are on AMO or even public, I have no idea.
Comment 14•11 years ago
|
||
This is the first time I've heard of this problem. Would anyone know if this still an issue? Most addons that I've test don't do this.
Comment 15•11 years ago
|
||
Most addons don't implement protocol handlers. A few do.
Comment 16•11 years ago
|
||
That said, the real question is whether any of them implement nsIHttpChannel...
Reporter | ||
Comment 17•11 years ago
|
||
Comment on attachment 493700 [details] [diff] [review]
preview 1
Review of attachment 493700 [details] [diff] [review]:
-----------------------------------------------------------------
We don't currently run any addons out of process (IIUC) so I think this is a latent issue that we may need to solve at some point, but not now.
Attachment #493700 -
Flags: feedback?(jduell.mcbugs)
Comment 18•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #16)
> That said, the real question is whether any of them implement
> nsIHttpChannel...
so in bug 897503 we saw that leethax and cocoon implemented nsIHttpProtocolHandler - and they were popular enough to create a top crasher when the background thumbnail service started using child processeses.. but neither of them implement nsIHttpChannel and I've never (thankfully) seen anything that does.
Updated•11 years ago
|
Summary: e10s: support custom protocols that wrap HTTP/FTP → e10s: Have a universal child<->parent protocol wrapper for any nsIChannel implementation that works only on parent process
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 493700 [details] [diff] [review]
preview 1
Review of attachment 493700 [details] [diff] [review]:
-----------------------------------------------------------------
I didn't look at every line of this, but the general approach here looks smart. The only missing piece I see is that GenericChannelChild does not implement nsIHttpChannel, and from earlier bug comments it sounds like we need that too. That shouldn't be a ton more work (although there's redirectTo() which will involve some code for sure :)
I still don't see this as high-priority until either one of these is imminent: 1) addons in b2g, or 2) e10s desktop.
Attachment #493700 -
Flags: feedback+
Updated•11 years ago
|
tracking-e10s:
--- → +
Updated•11 years ago
|
Updated•9 years ago
|
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Comment 20•9 years ago
|
||
Looks like may want this soon (e10s approaching.)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Updated•9 years ago
|
Whiteboard: [necko-would-take]
Updated•9 years ago
|
Updated•9 years ago
|
Whiteboard: [necko-would-take] → [necko-active]
Comment 21•9 years ago
|
||
Boris: is in an add-on JS-implemented protocol handler (registered via contractid) also visible to the child process?
I'm not sure what this bug is trying to solve right now. Reading it seems like the problems are two:
1. support custom protocols that add-ons implement (like myprotocol://url) to work from the content process ; I presume we want to support this.
2. prevent any miss-static_casts [1][2][3] we may do in a bad hope the nsIChannel we cast is nsHttpChannel ; may crash in case http protocol handler is overwritten by an add-on.
To support (1), we need a bit more then the universal protocol prototype posted here. When a handler is not found (which is probably the case when an add-on handler is registered only on the parent process - is it?) we fall back to the default ext-handler nsExternalProtocolHandler/nsExtProtocolChannel that forwards to the system - all happening on the content process only.
To fix (2) it's a bit more work. One way is to simply prevent reimplementation (overload) of http protocol handler - not support it anymore. I don't see any add-on on mxr/addons that would do that anyway.
[1] HttpChannelParent::DoAsyncOpen casts an http channel (created via IO service)
[2] HttpChannelParent::ConnectChannel casts a redirect registered channel (created via IO service)
[3] HttpChannelParent::OnStartRequest casts aRequest, but we will probably crash sooner anyway..
(static_cast's in HttpChannelChild are IMO safe, since those are results of NS_NewChannelInternal called on the child process where override is not possible, right?)
Flags: needinfo?(bzbarsky)
Comment 22•9 years ago
|
||
> Boris: is in an add-on JS-implemented protocol handler (registered via contractid) also visible to the child process?
I don't know.
Flags: needinfo?(bzbarsky)
Comment 23•9 years ago
|
||
I've checked that a protocol handler registered in bootstrap.js is not visible to the child process. Instead we get nsExternalProtocolHandler. That makes things (a little bit) more complicated.
Comment 24•9 years ago
|
||
- when we don't have a handler to handle the scheme on the child process, we use the generic channel to proxy to the parent (the change in nsExternalProtocolHandler::NewChannel2)
- we do this before we check we have a handler in the OS, since, IMO, we don't want system handlers be spawned in the child process anyway ; this patch effectively proxies them to the parent as well ; or should we do opposite for security reasons?
- as in the preview patch, the generic channel just forwards the data to the child
- we are able to redirect TO the custom protocol
- there is no support for allowing the custom protocol do redirects itself, but can be added later when needed (e.g. when the custom protocol returns an http channel that later redirects) ; already writing a patch
This patch doesn't solve static_casting to nsHttpChannel that may not be nsHttpChannel. We still support http protocol override. This can be easily forbidden or fixed correctly, in a different bug.
nsExternalProtocolHandler impls NewChannel2. If the custom protocol doesn't we wrap it with sec wrapper (nsSecCheckWrapChannel::MaybeWrap). So, everything's peachy :)
Attachment #493700 -
Attachment is obsolete: true
Attachment #8739469 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Updated•9 years ago
|
Attachment #8739469 -
Flags: review?(jduell.mcbugs) → review?(valentin.gosu)
Comment 25•9 years ago
|
||
Comment on attachment 8739469 [details] [diff] [review]
v1
Review of attachment 8739469 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good. It would be great to have a test case for this if possible.
::: netwerk/ipc/GenericChannelChild.cpp
@@ +21,5 @@
> +GenericChannelChild::GenericChannelChild(nsIURI* aURI, nsILoadInfo* aLoadInfo)
> +: mIPCOpen(false)
> +, mCanceled(false)
> +, mSuspendCount(0)
> +, mIsPending(PR_FALSE)
We can just use true/false now for the entire patch.
@@ +129,5 @@
> + OptionalLoadInfoArgs loadInfoArgs;
> + rv = mozilla::ipc::LoadInfoToLoadInfoArgs(mLoadInfo, &loadInfoArgs);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + SendAsyncOpen(uri, loadFlags, loadInfoArgs);
Maybe check if this fails.
@@ +195,5 @@
> +class GenericDataAvailableEvent : public ChannelEvent
> +{
> + public:
> + GenericDataAvailableEvent(GenericChannelChild* aChild, const nsCString& aData,
> + const uint32_t& aOffset, const uint32_t& aCount)
nit: indentation
@@ +201,5 @@
> + void Run() { mChild->DoOnDataAvailable(mData, mOffset, mCount); }
> + private:
> + GenericChannelChild* mChild;
> + nsCString mData;
> + uint32_t mOffset, mCount;
nit: declare each member on a separate line
@@ +216,5 @@
> +
> +void
> +GenericChannelChild::DoOnDataAvailable(const nsCString& data,
> + const uint32_t& offset,
> + const uint32_t& count)
nit: indentation
@@ +301,5 @@
> + GenericCancelEarlyEvent(GenericChannelChild* aChild, nsresult aStatus)
> + : mChild(aChild), mStatus(aStatus) {}
> + void Run() { mChild->DoCancelEarly(mStatus); }
> + private:
> + GenericChannelChild* mChild;
We should probably be using a RefPtr here. Maybe for the other events too?
::: netwerk/ipc/GenericChannelChild.h
@@ +23,5 @@
> +public:
> + typedef ::nsIStreamListener nsIStreamListener;
> +
> + GenericChannelChild(nsIURI* aURI, nsILoadInfo* aLoadInfo);
> + virtual ~GenericChannelChild();
This should be private.
::: netwerk/ipc/GenericChannelParent.h
@@ +26,5 @@
> + NS_DECL_NSIPARENTCHANNEL
> + NS_DECL_NSIINTERFACEREQUESTOR
> +
> + GenericChannelParent();
> + virtual ~GenericChannelParent();
Destructor should be private.
::: netwerk/ipc/NeckoChild.cpp
@@ +69,5 @@
>
> +PGenericChannelChild*
> +NeckoChild::AllocPGenericChannelChild()
> +{
> + MOZ_CRASH("This IPC pair is not designed to be instantiated from the parent side");
... from the child side.
::: netwerk/ipc/NeckoParent.h
@@ +108,5 @@
> const SerializedLoadContext& aSerialized,
> const HttpChannelCreationArgs& aOpenArgs) override;
> virtual bool DeallocPHttpChannelParent(PHttpChannelParent*) override;
> + virtual PGenericChannelParent* AllocPGenericChannelParent();
> + virtual bool DeallocPGenericChannelParent(PGenericChannelParent*);
These need to be marked override.
::: uriloader/exthandler/nsExternalProtocolHandler.cpp
@@ +434,5 @@
> + // Forward this unknown protocol opening fully to the parent process.
> + if (childProcess) {
> + nsCOMPtr<nsIChannel> channel = new GenericChannelChild(aURI, aLoadInfo);
> + channel.forget(aRetval);
> + return NS_OK;
Are there any cases when we _don't_ want to create a generic channel?
Attachment #8739469 -
Flags: review?(valentin.gosu) → review+
Comment 26•9 years ago
|
||
Has the webextensions team indicated that this is valuable for them? I really don't think we should implement this unless we need it for WebExtensions, since old-style addons that implement XPCOM interfaces are being deprecated.
Flags: needinfo?(honzab.moz)
Comment 27•9 years ago
|
||
From my outsider view I'm not aware of anything in WebExtensions so far that even implements raw socket functionality. Without this patch, the ramification with mandatory e10s means anything relying on protocol handlers that actually implement network protocols suddenly ceases to function, WebExtensions or not. Is the time frame for WebExtensions and e10s the same?
Admittedly, I have a dog in this fight since I maintain one of the (presumably few) addons that does actually implement nsIChannel and actually does use it as a network socket, but other addons like FireFTP and possibly Chatzilla could be affected.
Reporter | ||
Comment 28•9 years ago
|
||
We've got a working patch and we know there are existing addons that will break without this. And this is also the only way to support addons that do genuinely different network protocols (vs just wrapping HTTP). So we're going to land this and we can back out if it's obsolete at some point.
Flags: needinfo?(honzab.moz)
Comment 29•9 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #25)
> ::: netwerk/ipc/NeckoChild.cpp
> @@ +69,5 @@
> >
> > +PGenericChannelChild*
> > +NeckoChild::AllocPGenericChannelChild()
> > +{
> > + MOZ_CRASH("This IPC pair is not designed to be instantiated from the parent side");
>
> ... from the child side.
No, this is correct. This method would be called when the parent generic channel would be created first.
> ::: uriloader/exthandler/nsExternalProtocolHandler.cpp
> @@ +434,5 @@
> > + // Forward this unknown protocol opening fully to the parent process.
> > + if (childProcess) {
> > + nsCOMPtr<nsIChannel> channel = new GenericChannelChild(aURI, aLoadInfo);
> > + channel.forget(aRetval);
> > + return NS_OK;
>
> Are there any cases when we _don't_ want to create a generic channel?
I think not. We always create the external protocol when we don't know what to do (no handler). The final decision has to be always made on the parent. I don't know about any case, except some security stuff checking, but that anyway happens in AsyncOpen. If we were no-e10s, the channel would be created and any sec checking would happen in AsyncOpen anyway. I don't know about any other reason not to do this, but we may find some later. Hopefully not that late :D
Comment 30•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f644a76995e3
- includes a test as well
- onStartRequest IPC call carries the content length
If we need to carry down more, we'll find out later.
Attachment #8739469 -
Attachment is obsolete: true
Attachment #8755125 -
Flags: review+
Comment 31•9 years ago
|
||
Attachment #8755125 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8755126 -
Flags: review+
Comment 32•9 years ago
|
||
Currently there are two failing tests:
(Mn-e10s) TEST-UNEXPECTED-FAIL | test_navigation.py TestNavigate.test_should_not_error_if_nonexistent_url_used | AssertionError: The socket shouldn't have timed out when navigating to a non-existent URL
testing\marionette\harness\marionette\tests\unit\test_navigation.py:91
M-e10s(bc) uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js | Test timed out
Updated•8 years ago
|
Updated•8 years ago
|
Comment 34•8 years ago
|
||
Comment on attachment 8755126 [details] [diff] [review]
v1.1 [correct ci message]
So, I found out why this patch breaks the browser_web_protocol_handlers.js test. That test registers a web protocol handler (a remap of a custom schema to a different URL.)
The check whether to use the generic channel is to aggressive - I want to use it every time on child and forward everything to parent, no matter what.
That works in case of protocols like mailto: but not for these content based protocols.
More correct way is to use the generic channel when HaveExternalProtocolHandler returns false. But it will pass (return true) also for e.g. mailto: that although doesn't work on child at all.
One solution would be to deregister (or never register) some external protocols that can never work on the child process, like mailto:. It would lead to fallback to the generic channel only for those that really need it.
Attachment #8755126 -
Flags: review+
Comment 35•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #34)
> One solution would be to deregister (or never register) some external
> protocols that can never work on the child process, like mailto:. It would
> lead to fallback to the generic channel only for those that really need it.
That seems like a good solution.
Comment 36•8 years ago
|
||
Blake, one more question. If you read my comment 34 here, can you think if your patch could help recognize which of the external protocols to register on the content or not? If your patch is irrelevant to this, do you have any idea how to correctly remove (or bypass) registration of certain external handlers on the content process?
Thanks.
Flags: needinfo?(mrbkap)
Comment 37•8 years ago
|
||
The only change from v1.1 is how we decide to use the generic channel. Before we did it unconditionally when on a child process. That was wrong because there could be protocols (web based URI mappings and other types) available that worked well w/o this patch.
Hence, now we only do instantiate generic channel when we don't find any protocol.
Unfortunately, mailto is also registered (and works well when going directly to) in e10s. Unfortunate is that this no longer solves redirs to external protocols (bug 1277028) which need to be solved separately.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8da593806342
Attachment #8755126 -
Attachment is obsolete: true
Attachment #8759746 -
Flags: review?(valentin.gosu)
Comment 38•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #37)
> Created attachment 8759746 [details] [diff] [review]
> v1.2
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8da593806342
Still failing:
TEST-UNEXPECTED-FAIL | test_navigation.py TestNavigate.test_should_not_error_if_nonexistent_url_used | AssertionError: The socket shouldn't have timed out when navigating to a non-existent URL
TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_search.js | Test timed out -
TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_touch_simulation.js | Test timed out - (one instance only, OSX)
Jason, do you know anyone with good knowledge of Marionette tests? I don't fully understand how the load that "times out" actually works.
Blake, I think I no longer need an answer from you. Thanks.
Flags: needinfo?(mrbkap) → needinfo?(jduell.mcbugs)
Comment 39•8 years ago
|
||
Comment on attachment 8759746 [details] [diff] [review]
v1.2
Review of attachment 8759746 [details] [diff] [review]:
-----------------------------------------------------------------
The patch looks good. Thanks for adding tests.
::: netwerk/test/unit_ipc/test_generic_ipc_protocol.js
@@ +20,5 @@
> + Ci.nsIProtocolHandler.URI_NOAUTH |
> + Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,
> + newURI: function(aSpec, aOriginalCharset, aBaseURI) {
> + var uri = Cc["@mozilla.org/network/standard-url;1"].createInstance(Ci.nsIStandardURL);
> + uri.init(Ci.nsIStandardURL.URLTYPE_STANDARD, 666, aSpec, aOriginalCharset, aBaseURI);
Very evil port number >:)
Attachment #8759746 -
Flags: review?(valentin.gosu) → review+
Reporter | ||
Comment 40•8 years ago
|
||
I don't know Marionette. Looks like :jgriffen (Jonathan Griffen) is your best bet.
Flags: needinfo?(jduell.mcbugs)
Comment 41•8 years ago
|
||
I'm going to land this w/o being actually used at the time. First use, when confirmed, will be in bug 1277028 (I will shortly submit it).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=344b494933bd
Attachment #8759746 -
Attachment is obsolete: true
Attachment #8764889 -
Flags: review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 42•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #41)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=344b494933bd
This has test_generic_ipc_protocol xpcshell failures, no?
Keywords: checkin-needed
Comment 43•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> (In reply to Honza Bambas (:mayhemer) from comment #41)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=344b494933bd
>
> This has test_generic_ipc_protocol xpcshell failures, no?
Yes! Forgot to remove it.
Comment 44•8 years ago
|
||
Attachment #8764889 -
Attachment is obsolete: true
Attachment #8765440 -
Flags: review+
Comment 45•8 years ago
|
||
I slowly start believing we won't need this.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Whiteboard: [necko-active] → [necko-backlog]
Comment 46•8 years ago
|
||
What needs to be done here? For those of us who actually have nsIChannel implementations, is there something else we should do?
Comment 47•8 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #46)
> What needs to be done here? For those of us who actually have nsIChannel
> implementations, is there something else we should do?
There is ongoing discussion about the future of custom protocol handler implementations. I think we're still gathering information on precisely what features extensions need.
Andy, is that a decent summary? If so, are you the right person to talk to in order to ensure that we consider Cameron's usecase?
Flags: needinfo?(amckay)
Comment 48•8 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #47)
> Andy, is that a decent summary? If so, are you the right person to talk to
> in order to ensure that we consider Cameron's usecase?
I'm far from an expert here, so not sure what help I can give, Kris Maglione (:kmag) might be able to help more. Certainly if someone wants to propose an API for WebExtensions I would much rather focus on new APIs than trying to fix the past.
Flags: needinfo?(amckay)
Comment 49•8 years ago
|
||
Realistically anything that lets me implement a channel-type object would suffice (raw socket access, first-class citizen, protocol handler). I'm willing to do a rewrite if I can get the same functionality I get now with a new API. It certainly seems like bug 1227839 is really about the same thing.
Comment 50•8 years ago
|
||
Please don't rely on this bug. There are more and more arguments to wontfix it and let add-on developers use different APIs (I cannot at the moment offer a directions how to rewrite, but there are ways.)
Comment 51•8 years ago
|
||
Are there ways right *now*? If so, are there docs?
Comment 52•8 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #51)
> Are there ways right *now*? If so, are there docs?
I don't think there are any docs right now for this stuff. I also don't think that it's possible to have a general-purpose nsIChannel forwarder in JS because we don't have a way of cloning loadinfos. You can take a look at what we did for the about protocol shim [1][2], which works OK. One major caveat is that the protocol handler currently must be registered programmatically through nsIComponentRegistrar and not via a chrome manifest or similar.
A lot of what we do will depend on the strategy and API we want to expose. One possibility that I don't think I've seen mentioned here would be to expose the functionality of the patch via an XPCOM service that is then driven by other code as needed but not do any auto forwarding of protocols.
[1] http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/toolkit/components/addoncompat/RemoteAddonsChild.jsm#314
[2] http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/toolkit/components/addoncompat/RemoteAddonsParent.jsm#206
Comment 53•8 years ago
|
||
As a short-term shim, can we reconsider the CAN_LOAD_IN_CHILD concept (yours) from bug 1095484 comment 36? My suspicion is that the addons who implement their own nsIChannels will mostly "just work" if they can load in the parent.
Alternatively, will being able to register the protocol in both the child and parent suddenly fix this? My suspicion is no, but I don't know the answer.
Comment 54•8 years ago
|
||
This bug will also add support for redirect-TO a custom protocol, which needs a C++ support class, see my private thread post:
"""
we want to allow redirects TO a custom protocol. Then the provided custom channels must implement nsIChildChannel / nsIParentChannel interfaces. It's impossible for a JS code since at least nsIParentChannel is a noscript internal interface and there is no way to change that.
Hence, a C++ IPC proxy is needed. We already have it in bug 590682, actually ready to land (unused).
The missing piece is to make this universal proxy live. We don't know WHEN to use it. To explain, for a custom protocol available (registered only) on the parent we get the external protocol handler's channel on the child these days.
I think an add-on should express how any custom protocol it may register should be handled e.g. in its manifest, like:
<protocolChrome>myproto1,myproto2</>
<protocolContent>myproto3</>
It would tell us to use the universal proxy for myproto1,2,3 when loading URLs referenced from content, directly navigated to or being redirected to. Either Chrome or Content markup would just tell us on which process to have the "real" channel and call AsyncOpen on it.
Or, if there is a way to know which protocol handlers an add-on is registering (i've heard it's possible), we could collect this info from protocol handlers via two protocol flags: "I am fully e10s capable, don't proxy me" and "I want e10s assistance, and want to run on content". The first flag applies to most of our internal protocols like http, ftp, data. If none of the two flags is set, we proxy from child to parent and let the real channel run and be open on the parent - full backward compatibility, add-on devs in most cases don't need to do anything. The second flag just says to run the real channel on content. That also means to register the protocol on content, something one must do manually, which is already a change add-on devs must do anyway. Hence, maybe we need just the first flag.
"""
That has been answered by bsmedberd:
"""
We're moving pretty quickly toward a world where extensions can't use
XPCOM, and so this old method of registering protocol handlers won't make
sense. To the extent that we plan to support custom protocols, we should
have a well-designed webextensions API for this. In the meantime, dropping
support for the old XPCOM API surface makes a lot of sense to me, rather
than doing a whole bunch of work around trying to infer addon authors
intent.
"""
I don't know when this off-xpcom movement is about to happen and what we do offer instead. Ask bsmedberg.
IMHO, e10s will be released way sooner than deCOMtamination of add-ons. So something should happen NOW.
Flags: needinfo?(benjamin)
Comment 55•8 years ago
|
||
Authors are being asked to switch to WebExtensions immediately. If WebExtensions doesn't have the API surface we need, then we should try to design/implement something sane. Of course there will be a grace period over the next year, but since we're talking about a new e10s-compatible API anyway, why not do the right thing instead of perpetuating the necko XPCOM API?
Does Chrome/WebExtensions already have a way to represent this kind of addon-implemented protocol?
Flags: needinfo?(benjamin)
Comment 56•8 years ago
|
||
I don't know of one. I'd be happy to advocate for one but I don't know how to propose its addition or what's required to do so.
Comment 57•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #55)
> Of course there will be a grace period over
> the next year
What does this grace period actually mean? I really need to understand what your plan here is or if you have any.
Flags: needinfo?(benjamin)
Comment 58•8 years ago
|
||
Note that 70% of our top 30 addons implement own protocol handlers. It means that e10s turned on screws almost 10M ADIs.
Comment 59•8 years ago
|
||
I'm not the product manager for Addons. That would be Kev.
The important principles to note here are:
* The end state is going to be webextensions addons only.
* That's going to happen over the next year.
* So if we have a hole in the webextensions API that makes it impossible for these top-30 addons to port, we should immediately start work on a carefully designed long-term API surface.
* Anything we do in XPCOM is technical debt, and so we should only do it if we *have* to.
* The question of whether we have to, to support extensions that are important but haven't switched to webextensions, is a question that you and Kev need to answer.
So if you do land this, it should come with immediate deprecation warnings, and it also needs to come with some plan for a webextensions solution.
Flags: needinfo?(benjamin)
Comment 60•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #59)
> * So if we have a hole in the webextensions API that makes it impossible for
> these top-30 addons to port, we should immediately start work on a carefully
> designed long-term API surface.
That is yet to be answered.
>
> So if you do land this, it should come with immediate deprecation warnings,
> and it also needs to come with some plan for a webextensions solution.
The thing is that there is more (much more) work to do to finish this patch. Once, we don't know whether to run the 'real channel' on child or parent. That is something the addon knows and must somehow tell us. What I want to say is that we still may need some assistance from extension authors anyway or have an overkill amount of changes to support something we soon remove anyway.
I vote for WONTFIX.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Comment 61•8 years ago
|
||
Comment 56 still applies. If this is "WebExtensions or bust" I'm unaware of a WX surface for this and I don't know how to advocate for one. It's certainly beyond my ken presently to write myself.
However, I don't understand why we just wouldn't run the custom protocol channel on the parent, unless the extension specifically advertises it can run on the child, and leave the handler on the parent.
Comment 62•8 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #61)
> Comment 56 still applies. If this is "WebExtensions or bust" I'm unaware of
> a WX surface for this and I don't know how to advocate for one. It's
> certainly beyond my ken presently to write myself.
>
> However, I don't understand why we just wouldn't run the custom protocol
> channel on the parent, unless the extension specifically advertises it can
> run on the child, and leave the handler on the parent.
Probably open a new bug for it?
There are still some technical challenges to do to make the latest patch for this bug do something useful. I think the effort should be directed elsewhere. Hence WONTFIX.
Comment 63•8 years ago
|
||
Okay, created bug 1296885.
Updated•8 years ago
|
Comment 64•8 years ago
|
||
I'm not sure if the impact is as severe as mentioned for most of the add-ons, it shouldn't be assumed they'll all just break. Adding in addon-compat so the issue can be raised with add-on developers.
Keywords: addon-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•