Since this uses callbacks, this will depend on bug 536292. There's also no point in tackling this before Honza is done making redirect notifications async (bug 513086).

I suspect we can actually at least get started on this bug w/o the async redirect API work being completed. We can at least start to map out the design, and possibly write some of the IPDL code that's needed. When nsHttpChannel does a redirect, it will set up a new channel, and call redirect observers with the (new, old) channels. We should make HttpChannelParent a redirect observer, and it should call a new IPDL PHttpChannel constructor: this one will go from chrome -> content, and will take all the state info (requestHead, responseHead, etc) needed to bring the new HttpChannelChild into sync with the new nsHttpChannel. It will also pass in the IPDL actor for the old PHttpChannel, so once we've constructed the new HttpChannelChild, we can notify any redirect observers with the (new, old) HttpChannelChild's. I expect we run into the wrinkle at the point that the redirect API is currently synchronous, and we can't have chrome block waiting for content to call its redirect observers. But a lot of the IPDL scaffolding might be something we can start on. We have existing code for serializing the responseHead across IPDL, but we don't have it for the requestHead, so that at least ought to be something we could do, for instance.

Mayhemer: Any progress on this bug? I'd be willing to pick this one up.

I plan working on this very soon, two days maximum.

First WIP patch, it is missing the async redirect api changes (bug 513086 and bug 546606), so this patch so far doesn't work and pieces of the code that needs to update are marked with TODOs.

Isn't there any plan to take patches for bug 513086 and bug 546606 to the e10s tree? If not, this bug must be finished on mozilla-central after e10s merge and those two bugs landed.

We're planning to merge e10s to m-c very soon. After that I can see taking those patches to the e10s tree (which we're going to keep around) if we still haven't gotten +sr.

Comment on attachment 451548 [details] [diff] [review] WIP 1 Quit wrong patch...

OK, a better patch is getting born. But I have found a design problem with cookies now - the redirect target request doesn't contain cookies header. To explain: - when redirecting, the new nsHttpChannel instance is created on the chrome process, w/o it's parent/child binding first - HttpChannelParent in OnChannelRedirect creates new parent/child pair and calls Redirect (rpc) on the new child - it copies it self (SetupReplacementChannel dup) and check redirect (sinks->OnChannelRedirect) - when redirect is agreed, chrome nsHttpChannel calls AsyncOpen on the new nsHttpChannel instance; nsIStreamListener notifications are directed to the new second child; so far so good The problem is that cookies are normally added in HttpChannelChild::AsyncOpen to the request head being then carried to the parent. Now we do not call AsyncOpen on the child BEFORE we do for the nsHttpChannel on the chrome process. So, no chance for the chrome channel to have cookies at all. To solve this, HttpChannelChild::Redirect might return cookies header with the redirect veto result or we may turn the channel creation and opening up side down (not sure it is really possible). Just making this comment to let you know of this problem, if anyone has ideas to solve this in more elegant way, let me know. I continue working on this.

I'll have to look at the patch (which is high on my list!) to understand this more fully. CC-ing dwitte in case he's got ideas.

So, this looks like a working patch. I'm able to log into gmail, see and open mails (in the simple html version, the standard version could not not be displayed - getting stuck on the "Loading mail for user XXX"; this is probably a different bug). What needs to be finalized is call to AsyncOpen from RecvOnStartRequest. Maybe split the method to few new methods and call them separately from RecvOnStartRequest - followup?

Comment on attachment 455142 [details] [diff] [review] v1 - synchronous BZ, if you get time to look at this it would be great. I'm concerned about any potential security drawbacks caused by this patch. Also I was not testing internal redirects (proxy fail over, app cache fallbacks)

I'm a little lost at this point. What redirect approach are we implementing? Which process (or both?) do we send the redirect notifications, and in which process can listeners that cancel the redirect live?

(In reply to comment #14) > I'm a little lost at this point. What redirect approach are we implementing? This patch is synchronous, it is only a temporary solution. To send an async message from the chrome process and wait for an async message back from the content process w/o blocking the chrome process means to have patches for bug 513086 and bug 546606 landed. So I have created an rpc protocol for redirects. > Which process (or both?) do we send the redirect notifications Both. Redirect response is handled on the chrome process (nsHttpChannel), also the second nsHttpChannel is created there. Then gHttpHandler->OnChannelRedirect is called; this has not been changed. HttpChannelParent (callbacks to nsHttpChannel) implements OnChannelRedirect. It creates a new HttpChannelParent, binds it (now missing in the patch, just one line) to the new channel and sends a rpc message to the old HttpChannelChild that there is a redirect with reference to this new just created HttpChannelChild. Child channel mimics SetupReplacementChannel (initialize the new HttpChannelChild) and invokes gHttpHandler->OnChannelRedirect same way as it is made on the chrome process. The redirect veto result is then synchronously returned to the chrome process. Also, we have to push the response head to the old HttpChannelChild to store cookies (we won't get RecvOnStartRequest on the old child). Then, we have to return the cookie header to the chrome process from the redirect rpc message to correctly pass it to the new nsHttpChannel child on the chrome process. > and in which process can listeners that cancel the redirect live? In both.

Other thing is that I don't know how this is going to behave when the old channel is canceled from a sink. I have to do some testing of this.

We're seeing this quite a bit on Fennec when we try to log into zimbra and facebook.

Various parts of this patch were bitrotted, and I've made a few changes. More review comments coming, but I wanted to put this up in case you want to update to the async version (you might as well: we'll land the async patches before we land this). --- We've renamed PIFrameEmbedding -> PBrowser. Recent changes to refcounting in HttpChannelChild already added 'mIPCOpen', which is same as your 'mIPCActive' variable. > HttpChannelChild(PRBool createdByParent); changed to just bool. Use PRBool only for IDL now. >+ // Allocate the channel and AddRef it to stick with the current >+ // refcounting system, see NeckoParent::AllocPHttpChannel implementation >+ mRedirectChannel = new HttpChannelParent(iframeEmbedding); >+ if (!mRedirectChannel) >+ return NS_ERROR_OUT_OF_MEMORY; >+ mRedirectChannel->AddRef(); >+ // Create the child instance >+ Manager()->SendPHttpChannelConstructor(mRedirectChannel, iframeEmbedding); No need to check infallible new. And you might as well just call the version of SendPHttpChannelConstructor that constructs/addRefs the HttpChannelParent for you: mRedirectChannel = Manager()->SendPHttpChannelConstructor(browser);

Comment on attachment 459741 [details] [diff] [review] v2- synchronous, unbitrotted This patch has one crash problem >+ // No need to store PBrowser. It is only needed by the parent. When channel created from here: >+ HttpChannelChild* httpChannel = new HttpChannelChild(true); then mIPCOpen(createdByParent) = TRUE > , mCacheExpirationTime(nsICache::NO_EXPIRATION_TIME) > , mState(HCC_NEW) >- , mIPCOpen(false) >+ , mIPCOpen(createdByParent) >+ , mIsRedirectChannel(PR_FALSE) But in this case channel AsyncOpen not called >+ SendAsyncOpen(IPC::URI(mURI), IPC::URI(mOriginalURI), And we have case where mIPCOpen = true, and channel not opened... In that case we getting SetPriority call which is sending priority request to HTTPparent (mChannel == null there) And we have crash in HttpChannelParent::RecvSetPriority

I think the best way to fix this problem is to add an mPriority member to HttpChannelParent and store the priority from RecvSetPriority if the channel doesn't exist, then call mChannel->SetPriority in RecvAsyncOpen.

I'd like to wait for the async bugs to land prior to updating this patch. It's very very close to happen, so I can turn this patch to a regular asynchronous one.

> I'd like to wait for the async bugs to land prior to updating this patch That's fine. I'm just in the middle of writing up some architectural issues and more patch comments anyway. By the way, the issue with SetPriority is just that the new/redirected HttpChannelParent has a null ptr for mChannel--we need to set it in HttpChannelParent::OnChannelRedirect.

Comment on attachment 459741 [details] [diff] [review] v2- synchronous, unbitrotted Lots of comments. I've tried putting the one of most general interest first. Patch looks reasonably close. We've got a couple of design issues to figure out. Deciding what can be accessed/modified by redirect observers: ------------------------------------------------------------- We've got some subtle issues with what we want to guarantee about channels as seen and modified by redirect observers. Right now this patch shows redirect observers in the child a potentially imperfect copy of the chrome channel. And I don't think we want to try make a perfect copy. The question is how imperfect it can be, and also which modifications by content observers we want to support relaying back to the chrome channel. I propose that we support getting/setting only cookies and HTTP headers by redirect observers, and reading of things like loadFlags and URIs, AND that we don't guarantee that any cookie/header changes made by other redirect observers (i.e the chrome ones) are seen by others (the content ones). Providing stronger guarantees is hard/inefficient, and I don't think we need them. (We probably want to document this in the onChannelRedirect IDL comments.) Data: Right now the redirect observers that exist in the tree do things like - Reading the new URI - reading/setting cookies - reading/setting HTTP headers - reading/writing hashpropertyBag I don't see any of them doing things like - getting or setting the uploadfile - changing from GET to POST - any other channel changes I'm going to assume we don't care about hashpropertyBag mods, or anything in the second list. I.e., we don't support these things if they are used cross-process (CSPs, which use the hashbag, keep all logic within a single process AFAIK) The patch does the following now: 1) When redirect is hit, nsHttpChannel creates a new channel and sets it up via SetupReplacementChannel (this pre-exists e10s) 2) global redirect observers in chrome are called 2) The HttpChannelParent is the (only?) channel-specific redirect observer, and its OnChannelRedirect sets up a new PHttpChannel and calls the Redirect IPDL message, which then sets up something that resembles the chrome channel in many but not all ways: - the uploadchannel is null even if chrome's isn't (it would only be not-null for preserveMethod redirects (307) which are rare) - It does not include any changes to the HTTP headers by redirect observers that have already run in chrome, or cookies that they have set. 3) The redirect observer(s) on the child are called 4) The child replies to the parent with the status code (i.e. veto the redirect or not), plus info for copying any cookies that were set by child redirect observers into the new channel, which is then AsyncOpened. - we are not relaying any HTTP headers which are set on the child back to the chrome channel. This needs to be fixed (noted in my source comments). - we don't relay any other changes made to the channel to the chrome channel. This is hopefully fine. If we need to allow channel clients to successfully call GetUploadStream (which would only work for 307 redirects anyway), we could copy the upload stream from the old HttpChannelChild, under the assumption that none of the chrome redirect observers are allowed to have modified it. But I'm not sure we need it. Thoughts? ----------------- Lifecycle issues: ----------------- So this patch winds up keeping the original PHttpChannel and all associated objects (the nsHttpChannel, the HttpChannelChild/Parent) around even after the redirect has been approved and started. As near as I can tell, this is because you want/need to keep the HttpChannelParent as the original/same listener for the new nsHttpChannel. We then wind up having a listener chain where the new nsHttpChannel calls the old HttpChannelParent's OnStart/Data/Stop, and it just proxies that call to the new HttpChannelParent. I'm wondering if we can get away with simply setting the new HttpChannelParent as the listener, and delete the old PHttpParent/PHttpChannel. The only issue I can see with this is that we'd need to guarantee that the PHttpChannelParent is the direct and only listener of the nsHttpChannel. Looking over the code, most of the places that insert a new listener are both private and are not called until after the redirect logic occurs (InstallCacheListener, InstallOfflineCacheListener ApplyContentConversions, nsIStreamConverterService installed in CallOnStartRequest). That leaves nsITraceableChannel::SetNewListener. No code in our entire tree (except for some unit tests) appears to call this. Is it even possible for it to be called in chrome for an e10s channel? I don't think so. If not, I think we can simply delete the old PHttpProtocol at redirect time, unless there's something else I'm missing. If we do keep the current listener-chain logic, we need to delete the old PHttpChannel at some point after OnStopRequest. Right now there is no logic to do this. Note that with the fix for bug 572980--see also bug 576698, which the same patch fixed--we now delete the PHttpProtocol and release the PHttpParent once the child has finished OnStopRequest, instead of when the HttpChannelChild's refcount hits zero. Since this patch never calls OnStopRequest on the original HttpChannelChild, we may need to do something like have the new HttpChannelParent tell the old one to initiate destruction? And we'd need to handle this recursively, as we could have multiple redirects for a request. Kind of gross, but I don't have a better idea. It doesn't look like the original HttpChannelChild ever gets removed from its loadGroup when we redirect. For nsHttpChannel, it gets removed in OnStopRequest (but mListener is null by then, so that OnStop does't get forwarded to the HttpChannelParent or the child). I think we need to remove it in AnswerRedirect. > > //------------------------------------------------------------------- >-sync protocol PNecko >+rpc protocol PNecko We should revert both PNecko/PHttpChannel to the default 'async' when you make the changes to call the redirect observers asynchronously. >+bool >+HttpBaseChannel::GetCookiesForRequest(nsIURI *documentURI, >+ nsIURI *originalURI, >+ nsACString &result) As mentioned later, I think you don't need this GetCookiesForRequest function. >diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h >@@ -116,16 +118,23 @@ HttpChannelChild::RecvOnStartRequest(con > const PRBool& useResponseHead, > const PRBool& isFromCache, > const PRBool& cacheEntryAvailable, > const PRUint32& cacheExpirationTime, > const nsCString& cachedCharset) > { > LOG(("HttpChannelChild::RecvOnStartRequest [this=%x]\n", this)); > >+ if (mIsRedirectChannel) { >+ // Indicates we are result of a redirect. >+ mOriginalURI = mRedirectOriginalURI; >+ // XXX Hack a bit, probably split AsyncOpen to separate methods. >+ AsyncOpen(mListener, mListenerContext); >+ } This AsyncOpen hack is really gross, and we should get rid of it. AFAICT you're only calling it to set up some state variables (mIsPending, mWasOpened, mState), and you're then skipping most of the function for redirect channels. Additionally, some of the parts you're not skipping are things you shouldn't be doing (reading the uploadStream into a string, calling OnModifyRequest observers) or might as well do in ReplyRedirect/SetupReplacementchannel (adding request to the loadgroup). You're calling a 100-line function to get 3 lines of code from it! It also confuses people when they see a call to AsyncOpen inside OnStartRequest. In sum: Bad Idea :) >+HttpChannelChild::AnswerRedirect(PHttpChannelChild* newChannel, >+ const URI& newURI, >+ const PRUint32& newLoadFlags, >+ const PRUint32& redirectFlags, >+ const nsHttpResponseHead& responseHead, >+ nsresult* result, >+ nsCString* cookieRequest) >+{ >+ HttpChannelChild* newHttpChannel = static_cast<HttpChannelChild*>(newChannel); >+ nsCOMPtr<nsIURI> uri = newURI; >+ >+ newHttpChannel->mIsRedirectChannel = PR_TRUE; >+ >+ nsresult rv = newHttpChannel->HttpBaseChannel::Init(uri, mCaps, >+ mConnectionInfo->ProxyInfo()); >+ if (NS_FAILED(rv)) >+ return true; // TODO Send error? Yes, we'll need to figure out how to cancel. I'm fine with leaving it as a follow-up bug. >+ >+ newHttpChannel->mURI = uri; >+ newHttpChannel->mListener = mListener; >+ newHttpChannel->mListenerContext = mListenerContext; >+ newHttpChannel->mLoadFlags = newLoadFlags; >+ >+ GetLoadGroup(getter_AddRefs(newHttpChannel->mLoadGroup)); >+ >+ GetNotificationCallbacks(getter_AddRefs(newHttpChannel->mCallbacks)); >+ >+ GetReferrer(getter_AddRefs(newHttpChannel->mReferrer)); >+ >+ PRBool allowPipelining; >+ GetAllowPipelining(&allowPipelining); >+ newHttpChannel->mAllowPipelining = allowPipelining; >+ >+ PRUint32 redirectionLimit; >+ GetRedirectionLimit(&redirectionLimit); >+ newHttpChannel->mRedirectionLimit = (PRUint8)redirectionLimit - 1; >+ >+ PRBool forceAllowThirdPartyCookie; >+ GetForceAllowThirdPartyCookie(&forceAllowThirdPartyCookie); >+ newHttpChannel->mForceAllowThirdPartyCookie = forceAllowThirdPartyCookie; >+ >+ GetOriginalURI(getter_AddRefs(newHttpChannel->mRedirectOriginalURI)); >+ >+ if (uri && (mURI == mDocumentURI)) >+ uri->Clone(getter_AddRefs(newHttpChannel->mDocumentURI)); >+ else >+ newHttpChannel->mDocumentURI = mDocumentURI; >+ >+ // TODO - Offline Application Cache, bug 536295 >+ // >+ // oldHttpChannel->GetApplicationCache(mApplicationCache); >+ // oldHttpChannel->GetInheritApplicationCache(mInheritApplicationCache); >+ >+ mPropertyHash.EnumerateRead(CopyProperties, >+ static_cast<nsIWritablePropertyBag*>(newHttpChannel)); >+ >+ // We won't get OnStartRequest, set cookies here. >+ mResponseHead = new nsHttpResponseHead(responseHead); >+ SetCookie(mResponseHead->PeekHeader(nsHttp::Set_Cookie)); >+ >+ PRBool preserveMethod = (mResponseHead->Status() == 307); >+ if (preserveMethod) >+ { >+ nsCAutoString method; >+ GetRequestMethod(method); >+ newHttpChannel->SetRequestMethod(method); >+ } For clarity, why don't you move all of this code into a new HttpChannelChild::SetupReplacementChannel function (since that's what it basically is). There's a lot of shared logic with nsHttpChannel here: it would be good to create a HttpBaseChannel::SetupReplacementChannel function, and move common logic into there, so it's clearer where the logic differs. >+ *result = gHttpHandler->OnChannelRedirect(this, newHttpChannel, redirectFlags); >+ // TODO - Async redirect API, move following code to the callback >+ // >+ >+ // Extract cookies header, normally we build the request head in AsyncOpen >+ // and send its whole serialization to the parent channel. But in case >+ // of a redirect AsyncOpen is not called on the new channel. (However, >+ // some part of it is called from OnStartRequest, but it is too late) Change comment when you no longer call AsyncOpen from OnStartRequest. >+ newHttpChannel->GetCookiesForRequest(newHttpChannel->mDocumentURI, >+ newHttpChannel->mRedirectOriginalURI, >+ *cookieRequest); >+ >+ return true; >+} We're copying cookies here from the child to the parent nsHttpChannel, but we also need to copy any HTTP headers that child-side redirect observers may have set. This is simple--just send along mRequestHeaders in the reply here instead of cookieRequest (cookie headers are included in mRequestHeaders) and then set the headers like we do in HttpChannel::RecvAsyncOpen. This will also get rid of the need for your new GetCookiesForRequest function. >+ // The socket transport layer in the chrome process now has a logical ref to >+ // us, until either OnStopRequest or OnRedirect is called. >+ AddIPDLReference(); I've updated this comment to: // The socket transport in the chrome process now holds a logical ref to us // until OnStopRequest, or we do a redirect, or we hit an IPDL error. That may need to be re-written if we wind up keeping the "chained HttpChannelParent" approach. >+//----------------------------------------------------------------------------- >+// HttpChannelParent::nsIChannelEventSink >+//----------------------------------------------------------------------------- >+ >+NS_IMETHODIMP >+HttpChannelParent::OnChannelRedirect(nsIChannel *oldChannel, >+ nsIChannel *newChannel, >+ PRUint32 redirectFlags) >+{ >+ nsCOMPtr<nsILoadGroup> loadGroup; >+ oldChannel->GetLoadGroup(getter_AddRefs(loadGroup)); >+ if (loadGroup) { >+ nsCOMPtr<nsIChannelEventSink> loadGroupChannelEventSink; >+ NS_QueryNotificationCallbacks(nsnull, loadGroup, >+ NS_GET_IID(nsIChannelEventSink), >+ getter_AddRefs(loadGroupChannelEventSink)); >+ if (loadGroupChannelEventSink) { >+ nsresult rv = loadGroupChannelEventSink->OnChannelRedirect(oldChannel, >+ newChannel, >+ redirectFlags); >+ // TODO - Convert this to async, bug 546606 >+ if (NS_FAILED(rv)) >+ return rv; >+ } >+ } So right now we never set the loadgroup for the chrome nsHttpChannel, and we have no plans to. Maybe put this code in an #if 0 block with the comment // Right now we never set the loadgroup for the chrome nsHttpChannel, but if // we do we'll need code like this. Within an #if DEBUG, keep the GetLoadGroup call and add an NS_ASSERT(!loadGroup). >+ >+ // Create new PHttpChannel >+ PBrowserParent* browser = mTabParent ? >+ static_cast<TabParent*>(mTabParent.get()) : nsnull; >+ mRedirectChannel = static_cast<HttpChannelParent *> >+ (Manager()->SendPHttpChannelConstructor(browser)); >+ >+ // Update the notification callbacks of the new channel to the new parent >+ // channel instance. >+ newChannel->SetNotificationCallbacks(mRedirectChannel); So this is where I'd also set the listener of the nsHttpChannel to be mRedirectChannel. We'd need to add a public, non-IDL function ("ReplaceListener"?) to nsHttpChannel to do that. And we'd want to add an assert that the current listener == this. You also need to set mChannel in the new HttpChannelParent here to the new nsHttpChannel, or SetPriority blows up because it hits a null ptr (as mentioned already in comment 22). >+ >+ // And finally, let the content process decide to redirect or not. >+ nsCOMPtr<nsIURI> newURI; >+ newChannel->GetURI(getter_AddRefs(newURI)); >+ PRUint32 newLoadFlags; >+ newChannel->GetLoadFlags(&newLoadFlags); >+ >+ nsHttpChannel *oldHttpChannel = static_cast<nsHttpChannel *>(oldChannel); >+ >+ nsresult result; >+ nsCString cookieRequest; >+ CallRedirect(mRedirectChannel, >+ IPC::URI(newURI), >+ newLoadFlags, >+ redirectFlags, >+ *oldHttpChannel->GetResponseHead(), >+ &result, >+ &cookieRequest); Just like for all other IPDL traffic to the child, we need to check mIPCClosed here, and not CallRedirect if true, This is because our lifespan can be different than the IPDL protocol's: we're being kept alive by a ref from nsHttpChannel even if the child (and thus the IPDL connection) has already died by this point. Make sure that if that happens, we delete the old HttpChannelParent at some point (if we can set the new HttpChannelParent as the direct listener of the redirect channel, we'll simply go away when the old nsHttpChannel removes its ref to us at the end of ProcessRedirection. If we keep the "chained HttpChannelParent" approach you need to make sure whatever logic we use to destoy all the HttpChannelParents doesn't rely on the child being alive.) >+ >+ nsHttpChannel* newHttpChannel = static_cast<nsHttpChannel*>(newChannel); >+ newHttpChannel->SetRequestHeader(nsDependentCString(nsHttp::Cookie), cookieRequest, PR_FALSE); Set HTTP headers from child here, as mentioned. >diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl > >+ OnRedirectResult(nsresult result); Add comment for OnRedirectResult: // async report of redirect approval or veto by child process observers >diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp > > // Inform consumers about this fake redirect > PRUint32 flags = nsIChannelEventSink::REDIRECT_INTERNAL; > rv = gHttpHandler->OnChannelRedirect(this, newChannel, flags); >+ CheckAndReportRedirectCanceled(rv); > if (NS_FAILED(rv)) > return rv; >+ >+void >+nsHttpChannel::CheckAndReportRedirectCanceled(nsresult result) >+{ >+ if (NS_SUCCEEDED(result)) >+ return; Every caller of CheckAndReportRedirectCanceled checks rv afterward and returns if it fails, so I removed the check, renamed the function, and changed calls to if (NS_FAILED(rv)) { ReportRedirectCanceled(rv); return rv; } I suppose if we wanted to get all software-design-y, we could create an object in these functions that automatically calls ReportRedirectCanceled it its destructor unless it's been told not to--that would remove the need to remember to call this at each failure return site. But I'm ok with this unless you feel like implementing that. There's not *that* many failure sites. >+ /** >+ * If result is a failure, find the nsIRedirectVetoHook callback >+ * and let it know we do not redirect. Otherwise, do nothig. s/nothig/nothing/ -------------------------------- I've added an e10s version of the 'test_event_sink.js' xpcshell test => test_event_sink_wrap.js. The test simply cancels the redirect in an observer, and then checks to make sure the request was not initiated, i.e that AsyncOpen wasn't called, so OnDataAvailable isn't called. The test currently fails, because we *do* call AsyncOpen in nsHttpChannel::ProcessRedirect after we've launched the rpc CallRedirect, but before we've gotten back the reply. I assume you'll deal with this differently anyway once you change the patch to async. Just make sure the test works (Note: "make check-one" currently is broken and always reports success (see bug 581750): for now grep for TEST-UNEXPECTED-FAIL. and ignore the "Passed" msg at the end of the output. We don't seem to have any test for sucessful redirects, or that test if cookies/headers changes made it. It would be really great to have better tests. If you like, we can split that out into another bug, and give it to someone else. There is another test that check redirects (test_bug482601.js) that we should create a _wrap version for, but it also requires Cancel to work, so skipping for now. Patch with my changes is coming.

Jason, thanks for insights and analyzes. - the patch is fully dependent on the part 2 (bug 546606) and is then fully asynchrnous - all comments addressed (c22 and c23) - I have split HttpChannelParent files to HttpChannelParent and HttpChannelCallbackWrapper (hg cp) - HttpChannelParent now only works with the child and forwards progress notifications - HttpChannelCallbackWrapper works as callbacks and listener of nsHttpChannel, forwarding nsIStreamListener calls (OnStart/OnStop/OnData) to the parent - HttpChannelCallbackWrapper switches to the new parent when redirect happens; this approach should avoid breaking listener chains - some offline application stuff moved to HttpChannelBase, this will happen anyway with patch for application cache, needed for base SetupReplacementChannel - wrapped only a subset of existing redirect tests; fallback and proxy tests need access to permission manager or preferences: not allowed on content process so far - there is no loadgroup->AsyncOnChannelRedirect call in HttpChannelParent::AsyncOnChannelRedirect; the code would be too much complicated, if we want this, let's do it in a followup Google mail login and leaks+lifetime tested manually.

Comment on attachment 461948 [details] [diff] [review] v4 BTW: this patch is also based up on patch for bug 536301, but it's just about merging.

Last qref...

I'll update patch so that it applies on top of mozilla-central.

That's already been done, Honza did that last night but it got too late for him to stick it into the bug, so no need to spend time on that. Since Honza it's way late again where Honza lives I'll attach what Honza merged yesterday (which was non-trivial).

Honza, this should be what you sent me yesterday with the patches for the bugs that this depended on reverted, so this patch should be purely e10s redirects, and it applies to mozilla-central trunk as of a couple of hours ago (1e4f621a084d). This is for testing only, but if it looks good to you feel free to base further work on this. With this patch alone I can log into gmail using fennec.

Comment on attachment 463711 [details] [diff] [review] Merged to tip, and no dependencies on other patches (I think). This merged patch doesn't properly split HttpChannelParent.*.

This does.

Comment on attachment 463979 [details] [diff] [review] Merged to tip, and no dependencies on other patches (I think). Looks good. +r with comments addressed. The general approach here of adding a HttpChannelCallbackWrapper (so we can keep the same listener for the nsHttpChannel, yet delete the old PHttpChannel) is a good one. I talked to bz and my idea of directly replacing the old HttpChannelParent with the new one wasn't safe. Nice work, Honza. I've done some renaming in the patch, but there's a few more things I'd like to have changed (but I need to sleep :) Please rename HttpChannelCallbackWrapper to HttpChannelParentListener. And rename nsIRedirectVetoHook->nsIRedirectResultListener, and have it's function be called onRedirectResult (see note below). There is a potential race with both RecvSetPriority and RecvSetCacheTokenCachedCharset: if the child calls these, and a redirect has happened in chrome, but the child hasn't processed the redirect logic yet, the IPDL msgs will be received by the old channel, and the new values need to be copied to the new redirect channel. For SetCacheTokenCachedCharset, we don't allow calls before OnStartRequest anyway, so no race. I'm less clear about when SetPriority can get called by the DocShell, etc. Since we don't know which channel will wind up proceeding (depending on whether the redirect is cancelled or not), perhaps we should just call SetPriority in the parent on both mActiveChannel and mRedirectChannel? I.e. mActiveChannel->SetPriority(...) if (mRedirectChannel) mRedirectChannel->SetPriority() I don't expect we're going to be able to handle all the new places where we need to cancel in bug 536317. Please open a new followup bug for all the places where we'll need to figure out Cancel logic, and add comments with the bug #. As part of that bug, we need to fix a couple of the SendFoo methods from the parent to the child--right now we do not check mIPCClosed or check to see if the Send returns false. I've marked with comments. I made some changes--will attach patch. >+interface nsIRedirectVetoHook : nsISupports >+{ >+ /** >+ * If an HTTP redirect is vetoed, or the target channel failed to open, >+ * nsIHttpChannel will call this function if its callbacks implement this >+ * interface. >+ * >+ * @param result >+ * The status code from whatever operation caused the redirect to >+ * fail. >+ */ >+ void onRedirectVeto(in PRBool proceeding); Docs are incorrect. We're calling onRedirectVeto on both success or failure, and indicating which by passing a bool. There is no result parameter. I'd like to change the name of the interface to "nsIRedirectResultListener", and have the function be "onRedirectResult". How about this for docs? +interface nsIRedirectResultListener : nsISupports +{ + /** + * When an HTTP redirect has been processed (either successfully or not) + * nsIHttpChannel will call this function if its callbacks implement this + * interface. + * + * @param proceeding + * Indicated whether the redirect will be proceeding, or not (i.e. + * has been canceled, or failed). + */ + void onRedirectResult(in PRBool proceeding); ok? >+}; >diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp >+static PLDHashOperator >+CopyProperties(const nsAString& aKey, nsIVariant *aData, void *aClosure) >+{ >+ nsIWritablePropertyBag* bag = static_cast<nsIWritablePropertyBag*> >+ (aClosure); >+ bag->SetProperty(aKey, aData); >+ return PL_DHASH_NEXT; >+} Convert to 2-space indents, please. >+nsresult >+HttpBaseChannel::SetupReplacementChannel(nsIURI *newURI, >+ nsIChannel *newChannel, >+ PRBool preserveMethod) Convert to 2-space indents, please. >+ uploadChannel2->ExplicitSetUploadStream( >+ mUploadStream, >+ nsDependentCString(ctype), >+ len, >+ nsDependentCString(mRequestHead.Method()), >+ mUploadStreamHasHeaders); >+ } >+ else { Please convert to one line "} else {" style. >+ if (mUploadStreamHasHeaders) >+ uploadChannel->SetUploadStream(mUploadStream, EmptyCString(), >+ -1); >+ else { And add braces for the "if" part of if..else statement >diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h >+ virtual nsresult SetupReplacementChannel(nsIURI *, nsIChannel *, PRBool preserveMethod); 80 char max line. >diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelCallbackWrapper.cpp // C++ file contents // Header file contents Least useful comments ever. Delete them everywhere. >+HttpChannelCallbackWrapper::GetInterface(const nsIID& aIID, void **result) > { >+ >+ if (aIID.Equals(NS_GET_IID(nsIProgressEventSink))) { >+ if (!mActiveChannel) >+ return NS_NOINTERFACE; >+ return mActiveChannel->QueryInterface(aIID, result); I don't understand why you don't handle nsIProgressEventSink the same way as all the other interfaces, i.e. have HttpChannelCallbackWrapper implement OnProgress/OnStatus, and just call mActiveChannel->OnProgress/Status. You mentioned something about callback caching in an earlier version of the patch, but nsHttpChannel caching the pointer to &HttpChannelCallbackWrapper::OnProgress is fine. It's less error-prone than giving it mActiveChannel's function to cache. Am I missing something? >+HttpChannelCallbackWrapper::OnRedirectVeto(PRBool succeeded) > { >+ HttpChannelParent* channelToDelete = nsnull; >+ if (succeeded) { >+ // OK, switch the active channel to the redirect target loading channel and >+ // delete the active one. >+ if (!mActiveChannel->mIPCClosed) >+ channelToDelete = mActiveChannel; >+ mActiveChannel = mRedirectChannel; >+ } >+ else { >+ // Leave all as is, just delete the redirect target channel >+ channelToDelete = mRedirectChannel; >+ } >+ >+ if (channelToDelete) >+ HttpChannelParent::Send__delete__(channelToDelete); You checked mActiveChannel->mIPCClosed before Send__delete__ but not mRedirectChannel. Changed to check both. >diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp > >+static PLDHashOperator >+CopyProperties(const nsAString& aKey, nsIVariant *aData, void *aClosure) >+{ >+ nsIWritablePropertyBag* bag = static_cast<nsIWritablePropertyBag*> >+ (aClosure); >+ bag->SetProperty(aKey, aData); >+ return PL_DHASH_NEXT; >+} Unused copy: removed. >+ >+bool >+HttpChannelChild::RecvPreRedirect(PHttpChannelChild* newChannel, >+ const URI& newURI, >+ const PRUint32& redirectFlags, >+ const nsHttpResponseHead& responseHead) >+{ >+ HttpChannelChild* newHttpChannelChild = static_cast<HttpChannelChild*>(newChannel); >+ nsCOMPtr<nsIURI> uri(newURI); >+ >+ nsresult rv = newHttpChannelChild->HttpBaseChannel::Init(uri, mCaps, >+ mConnectionInfo->ProxyInfo()); >+ if (NS_FAILED(rv)) >+ return true; // TODO Bug 536317 >+ >+ // We won't get OnStartRequest, set cookies here. >+ mResponseHead = new nsHttpResponseHead(responseHead); >+ SetCookie(mResponseHead->PeekHeader(nsHttp::Set_Cookie)); >+ >+ PRBool preserveMethod = (mResponseHead->Status() == 307); >+ rv = SetupReplacementChannel(uri, newHttpChannelChild, preserveMethod); >+ if (NS_FAILED(rv)) >+ return true; // TODO Bug 536317 It doesn't look like SetupReplacementChannel ever returns anything but NS_OK. Change to return void, and remove check? >+ >+bool >+HttpChannelChild::RecvPostRedirect(PHttpChannelChild* newChannel, >+ const bool& succeeded) Renamed to RecvOnRedirect3Complete. Changed to only be called on success, since we do nothing otherwise. Also remove newChannel argument, since we already have mRedirectChannel. >+//----------------------------------------------------------------------------- >+// HttpChannelChild::nsIAsyncVerifyRedirectCallback >+//----------------------------------------------------------------------------- >+ >+NS_IMETHODIMP >+HttpChannelChild::OnRedirectVerifyCallback(nsresult result) >+{ >+ mRedirectChannelChild->AddCookiesToRequest(); >+ mRedirectChannelChild->SetOriginalURI(mRedirectOriginalURI); All the places where this SetOriginalURI call is done have this comment: > // Make sure to do this _after_ calling OnChannelRedirect I assume we want it here, too. But all the comments need to be updated now that we've changed to the async rediret API (is the answer now that it shouldn't be called until onRedirectVerifyCallback has been called?) >@@ -739,10 +821,47 @@ NS_IMETHODIMP >+nsresult >+HttpChannelChild::OpenRedirected(nsIStreamListener *listener, nsISupports *aContext) >+{ Renamed "CompleteRedirectSetup". >+ LOG(("HttpChannelChild::OpenRedirected [this=%x]\n", this)); >+ >+ NS_ENSURE_TRUE(gNeckoChild != nsnull, NS_ERROR_FAILURE); We don't need this check. Removed. >+ NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS); >+ NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED); These could only fail if some rogue redirect observer has called AsyncOpen on the channel. I suppose they're worth keeping, though they're going to complicate error handling. >+ >+ // Port checked in parent, but duplicate here so we can return with error >+ // immediately >+ nsresult rv; >+ rv = NS_CheckPortSafety(mURI); >+ if (NS_FAILED(rv)) >+ return rv; Port correctness was already checked synchronously in AsyncOpen in chrome, and if it failed, this won't be called, so I've removed this. >+ >+ // notify "http-on-modify-request" observers >+ gHttpHandler->OnModifyRequest(this); >+ >+ mIsPending = PR_TRUE; >+ mWasOpened = PR_TRUE; >+ mListener = listener; >+ mListenerContext = aContext; >+ >+ // add ourselves to the load group. >+ if (mLoadGroup) >+ mLoadGroup->AddRequest(this, nsnull); We have to handle case where http-on-modify-request observer has cancelled channel. Added TODO comment. Please add bug # for followup cancel bug. I believe we also need to call AddIPDLReference() here, as in AsyncOpen. Added it. >diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp > HttpChannelParent::OnStopRequest(nsIRequest *aRequest, > nsISupports *aContext, > nsresult aStatusCode) > { > LOG(("HttpChannelParent::OnStopRequest: [this=%x status=%ul]\n", > this, aStatusCode)); > > if (mIPCClosed || !SendOnStopRequest(aStatusCode)) > return NS_ERROR_UNEXPECTED; > return NS_OK; > } Any reason we shouldn't/can't set mCallbackWrapper to null here? I know the Callback wrapper will break the refcount cycle in its OnStop, but seems like we could null our ref here too, to make free happen sooner. >diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl > >+ // async report of redirect approval or veto by child process observers >+ OnRedirectResult(nsresult result, >+ RequestHeaderTuples requestHeaders); >+ >+ // Called to initiate content channel redirect, starts talking to sinks >+ // on the conent process and reports result via OnRedirectResult above >+ PreRedirect(PHttpChannel newChannel, >+ URI newUri, >+ PRUint32 redirectFlags, >+ nsHttpResponseHead responseHead); >+ // Called after chrome channel finished with redirect veto decision as a final >+ // aprove/reject of the redirect, if succeeded this method is responsible for >+ // freeing the old channel child >+ PostRedirect(PHttpChannel newChannel, >+ bool succeeded); Renamed PreRedirect to "Redirect1Begin", OnRedirectResult to "Redirect2Result", and PostRedirect to "Redirect3Complete". >diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp >+class AutoRedirectVetoNotifier Nice class! > // disconnect from our listener > mListener = 0; > mListenerContext = 0; >+ >+ notifier.RedirectSucceeded(); >+ > // and from our callbacks > mCallbacks = nsnull; > mProgressSink = nsnull; > return NS_OK; Can we move RedirectSucceeded call above the mListener=0, just to keep code neater? Don't see why not.

(In reply to comment #34) > Comment on attachment 463979 [details] [diff] [review] > Merged to tip, and no dependencies on other patches (I think). > > Looks good. +r with comments addressed. Thanks for review and your update. > There is a potential race with both RecvSetPriority Good catch, but, this has been probably introduced with the first part of the redirect API. Let's solve this in a differen bug. > RecvSetCacheTokenCachedCharset: I don't think there is any issue with this. > Please open a new followup bug for all the > places where we'll need to figure out Cancel logic, and add comments with the > bug #. Will do that. > I don't understand why you don't handle nsIProgressEventSink the same way as > all the other interfaces, i.e. have HttpChannelCallbackWrapper implement > OnProgress/OnStatus, and just call mActiveChannel->OnProgress/Status. You > mentioned something about callback caching in an earlier version of the patch, > but nsHttpChannel caching the pointer to > &HttpChannelCallbackWrapper::OnProgress is fine. It's less error-prone than > giving it mActiveChannel's function to cache. Am I missing something? > I had some reason for that... cannot remember what, so I'll probably change it. > It doesn't look like SetupReplacementChannel ever returns anything but NS_OK. > Change to return void, and remove check? > No, it may change and it is now a virtual method, so good to have it returning a result. > I assume we want it here, too. But all the comments need to be updated now > that we've changed to the async rediret API (is the answer now that it > shouldn't be called until onRedirectVerifyCallback has been called?) Another good catch - yes, this had to be updated as part of the redirect API patches. Let's do it in a followup. And your answer is correct. > I believe we also need to call AddIPDLReference() here, as in AsyncOpen. Added > it. And one more good catch - I just don't understand why it perfectly worked w/o it. I have to check for leaks with this change. > Any reason we shouldn't/can't set mCallbackWrapper to null here? I know the > Callback wrapper will break the refcount cycle in its OnStop, but seems like we > could null our ref here too, to make free happen sooner. I have to check for leaks, but the patch works w/o it well, so why to push the luck and break something.

(In reply to comment #37) > (In reply to comment #34) > I had some reason for that... cannot remember what, so I'll probably change it. The reason is that with my approach it is ensured that an nsHttpChannel always sends progress notifications to the correct HttpChannelParent/Child pair. What you suggest is IMO more dangerous and not that error prone.

(In reply to comment #37) > > I believe we also need to call AddIPDLReference() here, as in AsyncOpen. Added > > it. > > And one more good catch - I just don't understand why it perfectly worked w/o > it. I have to check for leaks with this change. Taking back: there is actually made what AddIPDLReference() does, so with what Jason suggest we would leak. The HttpChannelChild is instantiated by NeckoChild::AllocPHttpChannel that passes true to the constructor (sets mIPCActive = true) and AddRefs the channel. This is code from times before bug 572980 has landed. I'll change this to be more clear we do AddIPDLReference.

Posting for reference. Full patch sent by email.

Rebased to use buffering, and landed on e10s. Thanks honza! http://hg.mozilla.org/projects/electrolysis/rev/18cd465199df Will be merged to m-c if it behaves.