Bug 1522637 - Part 6: 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 D18606

Differential Revision: https://phabricator.services.mozilla.com/D19691

--HG--
extra : moz-landing-system : lando

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;
     }

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.
@@ -7120,6 +7125,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);
 
@@ -7256,13 +7263,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.

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,

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

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: