From 1b032ecafb29d084a6c13d103cc038b5c50132c8 Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Thu, 14 Feb 2019 15:37:06 +0000 Subject: [PATCH] Bug 1522637 - Part 7: Perform process switches separtely from on-examine-response, r=valentin Issues were occuring where a process swap would be decided upon during on-examine-response, but before the swap could be handled by the channel, the channel was redirected. This new code takes the mildly hacky approach of simply using a separate observer notification which is fired at the correct time. A better solution may be to use a dedicated service for responding to these events, however that was not implemented for this initial patch. Depends on D18607 Differential Revision: https://phabricator.services.mozilla.com/D19691 --HG-- extra : moz-landing-system : lando --- .../components/sessionstore/SessionStore.jsm | 12 ++---- netwerk/protocol/http/nsHttpChannel.cpp | 38 ++++++++++++------- netwerk/protocol/http/nsHttpHandler.h | 4 ++ .../protocol/http/nsIHttpProtocolHandler.idl | 8 ++++ .../browser/browser_cross_process_redirect.js | 12 ++---- 5 files changed, 44 insertions(+), 30 deletions(-) diff --git a/browser/components/sessionstore/SessionStore.jsm b/browser/components/sessionstore/SessionStore.jsm index 3e920df6cee0..c0370612cfc9 100644 --- a/browser/components/sessionstore/SessionStore.jsm +++ b/browser/components/sessionstore/SessionStore.jsm @@ -48,9 +48,7 @@ const OBSERVING = [ "quit-application", "browser:purge-session-history", "browser:purge-session-history-for-domain", "idle-daily", "clear-origin-attributes-data", - "http-on-examine-response", - "http-on-examine-merged-response", - "http-on-examine-cached-response", + "http-on-may-change-process", ]; // XUL Window properties to (re)store @@ -814,10 +812,8 @@ var SessionStoreInternal = { this._forgetTabsWithUserContextId(userContextId); } break; - case "http-on-examine-response": - case "http-on-examine-cached-response": - case "http-on-examine-merged-response": - this.onExamineResponse(aSubject); + case "http-on-may-change-process": + this.onMayChangeProcess(aSubject); break; } }, @@ -2318,7 +2314,7 @@ var SessionStoreInternal = { // Examine the channel response to see if we should change the process // performing the given load. - onExamineResponse(aChannel) { + onMayChangeProcess(aChannel) { if (!E10SUtils.useHttpResponseProcessSelection()) { return; } diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index e48e97e624a8..5fb40d79a1f0 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -2450,15 +2450,20 @@ nsresult nsHttpChannel::ContinueProcessResponse1() { } rv = NS_OK; - if (mRedirectTabPromise && !mCanceled) { - MOZ_ASSERT(!mOnStartRequestCalled); + if (!mCanceled) { + // notify "http-on-may-change-process" observers + gHttpHandler->OnMayChangeProcess(this); - PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2); - rv = StartCrossProcessRedirect(); - if (NS_SUCCEEDED(rv)) { - return NS_OK; + if (mRedirectTabPromise) { + MOZ_ASSERT(!mOnStartRequestCalled); + + PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2); + rv = StartCrossProcessRedirect(); + if (NS_SUCCEEDED(rv)) { + return NS_OK; + } + PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2); } - PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2); } // No process switch needed, continue as normal. @@ -7121,6 +7126,8 @@ NS_IMETHODIMP nsHttpChannel::SwitchProcessTo(dom::Promise *aTabPromise, nsresult nsHttpChannel::StartCrossProcessRedirect() { nsresult rv; + LOG(("nsHttpChannel::StartCrossProcessRedirect [this=%p]", this)); + rv = CheckRedirectLimit(nsIChannelEventSink::REDIRECT_INTERNAL); NS_ENSURE_SUCCESS(rv, rv); @@ -7257,13 +7264,18 @@ nsHttpChannel::OnStartRequest(nsIRequest *request, nsISupports *ctxt) { // before we check for redirects, check if the load should be shifted into a // new process. rv = NS_OK; - if (mRedirectTabPromise && !mCanceled) { - PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1); - rv = StartCrossProcessRedirect(); - if (NS_SUCCEEDED(rv)) { - return NS_OK; + if (!mCanceled) { + // notify "http-on-may-change-process" observers + gHttpHandler->OnMayChangeProcess(this); + + if (mRedirectTabPromise) { + PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1); + rv = StartCrossProcessRedirect(); + if (NS_SUCCEEDED(rv)) { + return NS_OK; + } + PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1); } - PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1); } // No process change is needed, so continue on to ContinueOnStartRequest1. diff --git a/netwerk/protocol/http/nsHttpHandler.h b/netwerk/protocol/http/nsHttpHandler.h index d2eb2e08a90e..1e7dd003bac3 100644 --- a/netwerk/protocol/http/nsHttpHandler.h +++ b/netwerk/protocol/http/nsHttpHandler.h @@ -368,6 +368,10 @@ class nsHttpHandler final : public nsIHttpProtocolHandler, NotifyObservers(chan, NS_HTTP_ON_EXAMINE_CACHED_RESPONSE_TOPIC); } + void OnMayChangeProcess(nsIHttpChannel *chan) { + NotifyObservers(chan, NS_HTTP_ON_MAY_CHANGE_PROCESS_TOPIC); + } + // Generates the host:port string for use in the Host: header as well as the // CONNECT line for proxies. This handles IPv6 literals correctly. static MOZ_MUST_USE nsresult GenerateHostPort(const nsCString &host, diff --git a/netwerk/protocol/http/nsIHttpProtocolHandler.idl b/netwerk/protocol/http/nsIHttpProtocolHandler.idl index 6132608011ae..6abe8e8276bb 100644 --- a/netwerk/protocol/http/nsIHttpProtocolHandler.idl +++ b/netwerk/protocol/http/nsIHttpProtocolHandler.idl @@ -123,6 +123,14 @@ interface nsIHttpProtocolHandler : nsIProxiedProtocolHandler */ #define NS_HTTP_ON_EXAMINE_CACHED_RESPONSE_TOPIC "http-on-examine-cached-response" +/** + * The observer of this topic is notified before before the response for a HTTP + * load is available. The "subject" of the notification is the nsIHttpChannel + * instance. Observers may call "switchProcessTo" to perform a process switch + * while this is being run. + */ +#define NS_HTTP_ON_MAY_CHANGE_PROCESS_TOPIC "http-on-may-change-process" + /** * Before an HTTP request corresponding to a channel with the LOAD_DOCUMENT_URI * flag is sent to the server, this observer topic is notified. The observer of diff --git a/netwerk/test/browser/browser_cross_process_redirect.js b/netwerk/test/browser/browser_cross_process_redirect.js index 28985bdd90c6..602d78f0f904 100644 --- a/netwerk/test/browser/browser_cross_process_redirect.js +++ b/netwerk/test/browser/browser_cross_process_redirect.js @@ -23,9 +23,7 @@ function ProcessChooser(tabParent, from, to, rejectPromise = false) { this.rejectPromise = rejectPromise; this.registered = true; - Services.obs.addObserver(this, "http-on-examine-response"); - Services.obs.addObserver(this, "http-on-examine-merged-response"); - Services.obs.addObserver(this, "http-on-examine-cached-response"); + Services.obs.addObserver(this, "http-on-may-change-process"); } ProcessChooser.prototype = { @@ -34,9 +32,7 @@ ProcessChooser.prototype = { return; } this.registered = false; - Services.obs.removeObserver(this, "http-on-examine-response"); - Services.obs.removeObserver(this, "http-on-examine-merged-response"); - Services.obs.removeObserver(this, "http-on-examine-cached-response"); + Services.obs.removeObserver(this, "http-on-may-change-process"); }, examine(aChannel) { @@ -83,9 +79,7 @@ ProcessChooser.prototype = { observe(aSubject, aTopic, aData) { switch (aTopic) { - case "http-on-examine-response": - case "http-on-examine-cached-response": - case "http-on-examine-merged-response": + case "http-on-may-change-process": this.examine(aSubject.QueryInterface(Ci.nsIHttpChannel)); break; default: