Our e10s code doesn't currently handle redirecting from HTTP to a different protocol (say, view-source, or jar, etc.). This is actually going to be a big pain to fix, involving a serious re-architecting of how we do redirects. So let me first ask the big question: can we ban redirects from HTTP to other protocols, or at least limit it to FTP? How much do we really need this feature? The problem in a nutshell: Right now we handle redirects (to http) by having the parent create a new PHttpChannel. We do this because it's convenient abstraction-wise: the e10s code acts as a redirect observer, so nsHttpChannel's redirect logic has already created a new nsHttpChannel for us, and we just wrap it with the PHttpChannel, and we can then call redirect observers in both parent|child, and proceed. To handle non-Http protocols, I can see two approaches: 1) We create a new IPDL protocol for each protocol that http can redirect to, and use the appropriate one to wrap the channel that the nsHttpChannel redirect logic gives us. The redirected-to channel lives in chrome, and we forward its OnStart/Data/Stop to a stub channel in content. 2) We short-circuit nsHttpChannels' redirect logic, and instead pass the new URI to the child, and have it create the channel there. There's no need for a new IPDL protocol (ex: for view-source:http://foo.com, there's no e10s work to do for the view-source channel itself; the http channel it creates and opens will run over e10s like any other HTTP channel). I think #2 is more promising, but each have downsides: Downsides of #1: - Involves writing a lot of IPDL for protocols that have previously lived without needing IPDL. - I can't think of a way to support extension protocols (bug 590682) using this (I don't think there's a way to write a generic IPDL protocol to support arbitrary extension protocols). - Could be hard/impossible to get right if the protocol wraps another channel, like view-source. Many methods in view-source are just passed along to the underlying channel. That would mean forwarding them back to the parent, which could already be in a different, inconsistent state by then. (see http://tinyurl.com/29sbntg ). Downside of #2: - What do we pass to chrome redirect observers as the "new channel" if we don't create a channel on the parent? Can we take the new channel that nsHttpChannel's redirect logic currently gives us, possibly wrap it with a class that records changes to headers, etc. (do we even have headers or other modifiable properties that we care about in protocols other than HTTP/FTP?), pass it along to chrome redirect observers, forward any changes made to the "real" channel in the child, and then essentially throw it away? But I see that many redirect observers do "mChannel = newChannel": we wouldn't break that, per se, but mChannel would point to a channel that never gets opened or updated again after the redirect observer are called. I haven't looked closely enough to see if that's always kosher. I'm hoping all observers that need an ongoing ref to the new channel live in the child (for child-initiated requests) I suspect that we might be able to use this "throwaway channel" method to make #2 work. E10s has already weakened the guarantees we provide about the new channels we pass to observers (see https://bugzilla.mozilla.org/show_bug.cgi?id=536294#c23); this would just add "maybe the new channel isn't really the channel that's going to be opened" to the list. We could also consider changing the redirect observer API to receive just the new URI and some sort of object that implements only the channel attributes that we support getting/setting. This would make the limitations explicit. But some things, like LoadGroups, actually want to keep a valid ref to the actual new channel, so they can cancel it or whatever, and we'd need a way for them to get it. Feedback/ideas would be welcome. (Do we have a list of all our protocols, and/or which ones are legal to redirect to from HTTP? Trying to glean that from the scriptSecurityManager code is non-trivial.)

it sounds like people are doing this in the field: <html> <head> <META HTTP-EQUIV="Refresh" CONTENT="5; URL=custom-protcol://a?b=c"> </head> <body></body> </html> in order to kick off a native application on device. We should figure out if people can live using js.

Yes, that's exactly what they doing. Web cite interface allows to operate only with url, so I tried with following code. <html> <head> <meta HTTP-EQUIV="Refresh" CONTENT="2; URL=javascript:window.location='http://google.com';"> </head> <body></body> </html> But javascript code is not executed in this case even on desktop non-e10s firefox. Though works fine in Opera. Is it a bug or some security restriction?

> Is it a bug or some security restriction? The latter. See bug 475636.

I think it is blocking applications sharing <-> browser handling on meego platform

We are going to use local http server as part of the sign-on procedure where redirect will be made from a remote service upon a successful sign-in. This will get around the restrictions set by XSS fixes. The prototype works well.

jason, any idea on supporting something like this in the 2.0 timeframe. Some device integration requires such support.

A suggestion may be in bug 590682 comment 5, or... ...another way, to support Jason's suggestion #2 from the description, we may use the fact that redirects are asynchronous. So, the channel can be as well created/obtained asynchronously - have an async API to create a channel. There are 4 places in nsHttpChannel which setup a redirect channel, so we need 4 different callbacks which get the channel and do something with it.

jason - who should this bug be assigned to? it is pretty important for platform integration.

-> me Taking the bug. Proposal is outlined in comment 7.

Here's the plan: - the real channel on the parent will be created synchronously as is now - in HttpChannelParentListener we register this new channel in a new service IChannelRegistrar - this registration will give us a unique id by which this new channel can later be found - Redirect1Begin will not take PHttpChannel anymore but will take the id - Redirect1Begin then creates the child channel with NS_NewChannelFromURI (or similar) - all child channel implementations will have to implement some new interface (e.g. IChildChannel), that will have a method (e.g. ConnectParent) enforcing the child to establish the parent IPC side and search for an existing channel using the id, instead of creating a new one - Redirect1Begin will call that method giving it the id - this way the new channel child will join with the already existing channel on the parent side - also will have to register the parent implementing nsIStreamListener under the same id - we can then grab it (in HttpChannelParent::RecvRedirect2Verify) and in case of redirect success forward stream listener notifications to it Opinions? Only disadvantage is to force child side of the protocol to implement IChannelChild interface. I'm also trying to think of bug 590682 here... If we find out that a child channel doesn't implement the IChildChannel interface, we can wrap it to a universal nsIChannel-to-IPC protocol (that implements it) and use its child channel rather then an in-updated implementation on the child side. As I understand, on the content process side we probably only need stream listener notifications and maybe progress notifications for most channels, nothing more.

bug 606104 is showing up on crash-stats as a top fennec crasher

This is my next item in TODO list.

This is a working preview of what's written in comment 10. So far it just fails to redirect to a different protocol, but doesn't crash. Now I have to patch FTP to support the new interfaces.

>if (tabParent) > return NS_NOINTERFACE; > >return tabParent->QueryInterface(aIID, result); That'll cause some problems.

(In reply to comment #16) > That'll cause some problems. What problems exactly?

Ups... missing ! in the condition. Thanks.

First regular patch for this. - allows only regular redirect to FTP, FTP IPC changed - other protocols are not yet supported (=expected not to work properly) - the mechanics of the logic is described in comment 10, except: - there are also new interfaces for the parent side and we grab the parent to forward to in HttpChannelParentListener::OnRedirectResult - if there is no registered parent channel under the id at that moment, we cancel the new real redirect channel since it cannot do it's job --- however this point is something left for discussion - we can grab the parent channel in OnRedirectResult because it's ensured to be called between successful call to AsyncOpen on the new channel and event queue loop (we cannot get callbacks before that) - some GI and http specific code moved from HttpChannelParentListener to HttpChannelParent --- here it seems that HttpChannelParentListener becomes something generic - got rid of mChannelListener member of HttpChannelParent

re: comment 1 > it sounds like people are doing this in the field: > > <> html> > <head> > <META HTTP-EQUIV="Refresh" CONTENT="5; URL=custom-protcol://a?b=c"> > </head> > <body></body> > </html> > > in order to kick off a native application on device. It would be useful to have a test case for this--particularly to see what kind of things they're doing with the protocol. Can anyone point us at a URL that does this?

(In reply to comment #21) > re: comment 1 > > > it sounds like people are doing this in the field: > > > > <> html> > > <head> > > <META HTTP-EQUIV="Refresh" CONTENT="5; URL=custom-protcol://a?b=c"> > > </head> > > <body></body> > > </html> > > > > in order to kick off a native application on device. > > It would be useful to have a test case for this--particularly to see what kind > of things they're doing with the protocol. Can anyone point us at a URL that > does this? It all started from Flickr authentication support needs: http://www.flickr.com/services/api/auth.howto.web.html In order to cut the number of steps where separate confirmations are required by the user as we cannot detect that authentication has happend with desktop auth scheme for Flickr, web authentication is setup with a URL that redirects to a local browser extension/protocol supported by the system. Step 4 (defining callback URL) on that page tells the story.

Comment on attachment 491075 [details] [diff] [review] v1 Nice patch--elegant solution. Note: I've verified that this works with redirects to protocols that are handled by external applications (rtsp://, irc://, etc.). It will not work yet with "wrapper" protocols (like view-source, or various extension protocols that also wrap an HTTP channel). Those will be handed in bug 590682. (Fortunately it sounds like the fennec cases we care about right now are external app protocols.) I'd like to make some doc changes, and go through some of the cancellation cases a little more, but for now this is passing try and just about ready to be checked in (I found a couple things that need fixing). Let's try to land it ASAP so it can get in fennec b3: we can land any doc fixes later if we need to. >+[scriptable, uuid(c45b92ae-4f07-41dd-b0ef-aa044eeabb1e)] >+interface nsIChildChannel : nsISupports Do we want to make these new interfaces scriptable? I don't see why we'd ever want to have JS call these new APIs. >diff --git a/netwerk/base/public/nsIParentChannel.idl b/netwerk/base/public/nsIParentChannel.idl >+interface nsIParentChannel : nsIStreamListener I don't any good reason to have nsIParentChannel inherit from nsIStreamListener. Is there one? Not a big deal, but they seem logically separate to me. I'll leave decision to you. >diff --git a/netwerk/base/src/RedirectChannelRegistrar.cpp b/netwerk/base/src/RedirectChannelRegistrar.cpp >+template<class KeyClass,class T> nit: space after , >+PRBool >+RedirectChannelRegistrar::nsCOMPtrHashtable<KeyClass,T>::Get(KeyType aKey, T** retVal) const >+{ >+ typename base_type::EntryType* ent = this->GetEntry(aKey); >+ >+ if (ent) >+ { In new code let's keep the { on the same line as if/else/for/while/etc >+RedirectChannelRegistrar::RedirectChannelRegistrar() >+ : mId(0) Maybe I'm paranoid, but should we init to 1, so client always gets nonzero id? If so also init HttpChannelParentListener::mRedirectChannelId to 0. >+{ >+ mRealChannels.Init(16); >+ mParentChannels.Init(16); >+} How many redirect requests do we expect we'll ever run into at once? PL_DHashTableInit says the actual capacity used should be .75 or less, so that's 12 simultaneous redirects. Seems like at times (session restore?) we could get higher. Do we want a bigger number here? 32? Not a big deal, since we'll just realloc a larger table as needed. >diff --git a/netwerk/protocol/ftp/FTPChannelParent.cpp b/netwerk/protocol/ftp/FTPChannelParent.cpp >+FTPChannelParent::RecvConnectChannel(const PRUint32& channelId) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIRedirectChannelRegistrar> registrar = >+ do_GetService("@mozilla.org/redirectchannelregistrar;1", &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ LOG(("Looking for a registered channel [this=%p, id=%d]", >+ this, channelId)); >+ >+ // Find the channel by the provided id and join with it >+ nsCOMPtr<nsIChannel> channel; >+ rv = registrar->GetRegisteredChannel(channelId, getter_AddRefs(channel)); >+ if (NS_FAILED(rv) || !channel) { >+ // If the channel was not found it is probably becuase it has already been >+ // removed from the registrar in HttpChannelParentListener::OnRedirectResult >+ // because the redirect has been canceled sooner we got response from the >+ // content process. >+ LOG((" could not find the registered channel")); >+ return true; >+ } We repeat this code (get channel registrar and call GetRegisteredChannel, doing logging) in three places--stick it in a function in nsNetUtil.h? >+NS_IMETHODIMP >+FTPChannelParent::Delete() >+{ >+ // TODO - not sure we need it, but should delete the child or call on the >+ // child to release the parent. >+ return NS_OK; >+} Since you don't delete the PFTP channel in Delete here, when exactly does it get deleted if the redirect is vetoed? HttpChannelParentListener::OnRedirectResult will call Delete on the FTP channel but it will do nothing. I assume we want logic similar to HTTP here, ie send msg to child to tell it to call send__delete. Or maybe we can send_delete direct from the parent-I haven't thought it through. > NS_IMETHODIMP > FTPChannelParent::GetInterface(const nsIID& uuid, void** result) > { >- DROP_DEAD(); >+ return NS_NOINTERFACE; > } Might as well make it return QueryInterface(uuid, result) Doesn't make a difference now--not used--but logically correct. >diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp >+HttpChannelChild::Redirect1Begin(const PRUint32& newChannelId, > const IPC::URI& newURI, > const PRUint32& redirectFlags, > const nsHttpResponseHead& responseHead) > { >+ mRedirectChannelChild = do_QueryInterface(newChannel); >+ if (mRedirectChannelChild) { >+ mRedirectChannelChild->ConnectParent(newChannelId); >+ } >+ else { formatting: } else { >@@ -739,27 +782,38 @@ HttpChannelChild::CompleteRedirectSetup( > NS_IMETHODIMP > HttpChannelChild::OnRedirectVerifyCallback(nsresult result) > { >+ nsCOMPtr<nsIHttpChannel> newHttpChannel = >+ do_QueryInterface(mRedirectChannelChild); > >+ if (newHttpChannel && NS_SUCCEEDED(result)) { >+ // Cookies may have been changed by redirect observers >+ HttpChannelChild* channel = >+ static_cast<HttpChannelChild*>(newHttpChannel.get()); >+ channel->AddCookiesToRequest(); >+ // Must not be called until after redirect observers called. >+ newHttpChannel->SetOriginalURI(mRedirectOriginalURI); > >+ // After we verify redirect, nsHttpChannel may hit the network: must give >+ // "http-on-modify-request" observers the chance to cancel before that. >+ if (NS_SUCCEEDED(result)) >+ gHttpHandler->OnModifyRequest(newHttpChannel); >+ >+ HttpChannelChild* newChild = static_cast<HttpChannelChild*> >+ (newHttpChannel.get()); formatting: HttpChannelChild* newChild = static_cast<HttpChannelChild*>(newHttpChannel.get()); logic: alas, this cast is not safe. There are other classes (nsViewSourceChannel, possibly JS-extensions) that also implement nsIHttpChannel, so QIing to it is not enough to know that this cast is safe. Long-term we probably want to add some IDL method to nsIHttpChannelInternal that allows us to get mResponseHeaders from any channel that impls nsIHttpChannel. Short term we want to not segfault :), so maybe either add a CID for HttpChannelChild that you can QI, or use a gross hack like also test for an IDL that we know is currently only implemented by HttpChannelChild, like nsIApplicationCacheChannel/nsIAssociatedContentSecurity. Or add a new IDL like "nsIHttpChildChannel" that inherits from nsIChildChannel, and is just used to distinguish that it's safe to cast. >diff --git a/netwerk/protocol/http/HttpChannelParentListener.cpp b/netwerk/protocol/http/HttpChannelParentListener.cpp > NS_IMETHODIMP > HttpChannelParentListener::GetInterface(const nsIID& aIID, void **result) > { > >+ nsCOMPtr<nsIInterfaceRequestor> ir; >+ if (mActiveChannel && >+ NS_SUCCEEDED(CallQueryInterface(mActiveChannel.get(), >+ getter_AddRefs(ir)))) >+ { >+ nsresult rv = ir->GetInterface(aIID, result); >+ if (NS_SUCCEEDED(rv)) >+ return rv; > } Just "return ir->GetInterface(...) here? Seems unlikely that any of them would return anything other than NS_NOINTERFACE on failure, but in principle we might as well use whatever error the call gives us. > //----------------------------------------------------------------------------- > // HttpChannelParentListener::nsIChannelEventSink > //----------------------------------------------------------------------------- > > NS_IMETHODIMP > HttpChannelParentListener::AsyncOnChannelRedirect( > nsIChannel *oldChannel, > nsIChannel *newChannel, > PRUint32 redirectFlags, > nsIAsyncVerifyRedirectCallback* callback) > { >+ nsCOMPtr<nsIParentRedirectingChannel> activeRedirectingChannel = >+ do_QueryInterface(mActiveChannel); >+ NS_ABORT_IF_FALSE(activeRedirectingChannel, >+ "Channel got a redirect response, but doesn't implement " >+ "nsIParentRedirectingChannel to handle it."); This would be an unrecoverable error even in an opt, build, no? Perhaps we should use NS_RUNTIMEABORT instead of NS_ABORT_IF_FALSE. +r with the static_cast and FTP Delete issues fixed. I'd be happy to review those if you want me to, or you can just land if you trust your fixes are simple enough.

Comment on attachment 491075 [details] [diff] [review] v1 Ah, one more thing. Try actually shows one bug: test_event_sink_wrap.js is failing with a timeout, which appears to be the result of ###!!! ASSERTION: Already registered channel not found: 'newChannel', file /home/jduell/central/t/netwerk/protocol/http/HttpChannelParentListener.cpp, line 221 Which (from a glance at the test) looks like it's from an attempt to redirect (illegally) to a file:// URI. This is not mission-critical--I'd still rather have the functionality here as-is for fennec b3 than block landing on this. But may be a simple fix (no assert?).

(In reply to comment #24) > Do we want to make these new interfaces scriptable? I don't see why we'd ever > want to have JS call these new APIs. I'd leave it as is, it may help write tests in the future at least. > I don't any good reason to have nsIParentChannel inherit from > nsIStreamListener. Is there one? Not a big deal, but they seem logically > separate to me. I'll leave decision to you. We require something to forward the notifications to. If nsIParentChannel would not inherit it, we would QI for it anyway and what to do in case we don't get it? There would be no way to send notifications to content. This way I force coders to implement the stream listener as well. > Maybe I'm paranoid, but should we init to 1, so client always gets nonzero id? > If so also init HttpChannelParentListener::mRedirectChannelId to 0. Agree. > > >+{ > >+ mRealChannels.Init(16); > >+ mParentChannels.Init(16); > >+} > > How many redirect requests do we expect we'll ever run into at once? > PL_DHashTableInit says the actual capacity used should be .75 or less, so > that's 12 simultaneous redirects. Seems like at times (session restore?) we > could get higher. Do we want a bigger number here? 32? Not a big deal, since > we'll just realloc a larger table as needed. I here there questions quit often. There is no research done how mane redirects may be pending, but as it is async it may be a lot. Let's rise this number to say 64 or even more. This is allocated in a service object, so no need to safe much memory here. > > >diff --git a/netwerk/protocol/ftp/FTPChannelParent.cpp b/netwerk/protocol/ftp/FTPChannelParent.cpp > > >+FTPChannelParent::RecvConnectChannel(const PRUint32& channelId) > >+{ > >+ nsresult rv; > >+ > >+ nsCOMPtr<nsIRedirectChannelRegistrar> registrar = > >+ do_GetService("@mozilla.org/redirectchannelregistrar;1", &rv); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ > >+ LOG(("Looking for a registered channel [this=%p, id=%d]", > >+ this, channelId)); > >+ > >+ // Find the channel by the provided id and join with it > >+ nsCOMPtr<nsIChannel> channel; > >+ rv = registrar->GetRegisteredChannel(channelId, getter_AddRefs(channel)); > >+ if (NS_FAILED(rv) || !channel) { > >+ // If the channel was not found it is probably becuase it has already been > >+ // removed from the registrar in HttpChannelParentListener::OnRedirectResult > >+ // because the redirect has been canceled sooner we got response from the > >+ // content process. > >+ LOG((" could not find the registered channel")); > >+ return true; > >+ } > > We repeat this code (get channel registrar and call GetRegisteredChannel, doing > logging) in three places--stick it in a function in nsNetUtil.h? I was even thinking of a different API of the registrar that would take the parent channel, id, find and return the real channel, and register the parent one in a single method. Not sure of the name for the method, though. CrossoverChannels ? > Since you don't delete the PFTP channel in Delete here, when exactly does it > get deleted if the redirect is vetoed? > HttpChannelParentListener::OnRedirectResult will call Delete on the FTP channel > but it will do nothing. I assume we want logic similar to HTTP here, ie send > msg to child to tell it to call send__delete. Or maybe we can send_delete > direct from the parent-I haven't thought it through. I'll copy the approach from http channel. > > > > NS_IMETHODIMP > > FTPChannelParent::GetInterface(const nsIID& uuid, void** result) > > { > >- DROP_DEAD(); > >+ return NS_NOINTERFACE; > > } > > Might as well make it > > return QueryInterface(uuid, result) I'll check that won't break anything and change it. > >+ else { > > formatting: > > } else { Uggrhhh.. I hate this formatting, since when is it used? It makes using { on the same line even more unreadable IMHO. > HttpChannelChild* newChild = > static_cast<HttpChannelChild*>(newHttpChannel.get()); > > logic: alas, this cast is not safe. There are other classes > (nsViewSourceChannel, possibly JS-extensions) that also implement > nsIHttpChannel, so QIing to it is not enough to know that this cast is safe. > Long-term we probably want to add some IDL method to nsIHttpChannelInternal > that allows us to get mResponseHeaders from any channel that impls > nsIHttpChannel. Short term we want to not segfault :), so maybe either add a > CID for HttpChannelChild that you can QI, or use a gross hack like also test > for an IDL that we know is currently only implemented by HttpChannelChild, like > nsIApplicationCacheChannel/nsIAssociatedContentSecurity. Or add a new IDL like > "nsIHttpChildChannel" that inherits from nsIChildChannel, and is just used to > distinguish that it's safe to cast. Very good point, I'll go the nsIHttpChildChannel way. > >+ nsCOMPtr<nsIParentRedirectingChannel> activeRedirectingChannel = > >+ do_QueryInterface(mActiveChannel); > >+ NS_ABORT_IF_FALSE(activeRedirectingChannel, > >+ "Channel got a redirect response, but doesn't implement " > >+ "nsIParentRedirectingChannel to handle it."); > > This would be an unrecoverable error even in an opt, build, no? Perhaps we > should use NS_RUNTIMEABORT instead of NS_ABORT_IF_FALSE. When this fails, the content won't switch to the new channel and won't call AsyncOnChannelRedirect. But will still be able to accept stream listener notifications. This means, it is not a catastrophic failure. If a channel implemented by an extension or our nsBaseChannel based channel invokes the redirect API, we should probably only fail the redirect, but I'm not sure what is the politic here. Rather hard fail, then let things work in an unexpected/semi-working way. (In reply to comment #25) > Comment on attachment 491075 [details] [diff] [review] > v1 > This is not mission-critical--I'd still rather have the functionality here > as-is for fennec b3 than block landing on this. But may be a simple fix (no > assert?). I think it fails because: - nsHttpChannel tries to redirect - the code path fails sooner we get to nsIChannelEventSink notification - we get OnRedirectResult and look for the channel to cancel it when there is no parent - we should do this only when we have registered the channel I can use your proposal for initiating the id to 0 and always have a positive id assigned by the registrar. If mRedirectChannelId in HttpChannelParentListener is 0 don't look for the registered channel.

This is updated to Jason's comments.

Comment on attachment 492819 [details] [diff] [review] v1.1 [Check in comment 28] http://hg.mozilla.org/mozilla-central/rev/33c8fcb13a99

There should be one more enhancement to make this work even more better, I'll try to make it till the code freeze.

Minor nit: I'd have made nsIHttpChannelChild inherit from nsIChannelChild.

Honza tells me his mysterious "enhancement" (comment 29) should not block the fennec b3 release.