Bug 1649349 - Don't delay rejecting DocumentLoadListener's Open promise when we switch process. r=jya

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

netwerk/ipc/DocumentChannelChild.cpp

@@ -123,8 +123,15 @@ IPCResult DocumentChannelChild::RecvFailedAsyncOpen(
 }
 
 IPCResult DocumentChannelChild::RecvDisconnectChildListeners(
-    const nsresult& aStatus, const nsresult& aLoadGroupStatus) {
-  DisconnectChildListeners(aStatus, aLoadGroupStatus);
+    const nsresult& aStatus, const nsresult& aLoadGroupStatus,
+    bool aSwitchedProcess) {
+  // If this is a normal failure, then we want to disconnect our listeners and
+  // notify them of the failure. If this is a process switch, then we can just
+  // ignore it silently, and trust that the switch will shut down our docshell
+  // and cancel us when it's ready.
+  if (!aSwitchedProcess) {
+    DisconnectChildListeners(aStatus, aLoadGroupStatus);
+  }
   return IPC_OK();
 }
 

netwerk/ipc/DocumentChannelChild.h

@@ -38,7 +38,8 @@ class DocumentChannelChild final : public DocumentChannel,
   mozilla::ipc::IPCResult RecvFailedAsyncOpen(const nsresult& aStatusCode);
 
   mozilla::ipc::IPCResult RecvDisconnectChildListeners(
-      const nsresult& aStatus, const nsresult& aLoadGroupStatus);
+      const nsresult& aStatus, const nsresult& aLoadGroupStatus,
+      bool aSwitchedProcess);
 
   mozilla::ipc::IPCResult RecvDeleteSelf();
 

netwerk/ipc/DocumentChannelParent.cpp

@@ -79,11 +79,11 @@ bool DocumentChannelParent::Init(dom::CanonicalBrowsingContext* aContext,
         self->mDocumentLoadListener = nullptr;
       },
       [self](DocumentLoadListener::OpenPromiseFailedType&& aRejectValue) {
-        if (!self->CanSend()) {
-          return;
+        if (self->CanSend()) {
+          Unused << self->SendDisconnectChildListeners(
+              aRejectValue.mStatus, aRejectValue.mLoadGroupStatus,
+              aRejectValue.mSwitchedProcess);
         }
-        Unused << self->SendDisconnectChildListeners(
-            aRejectValue.mStatus, aRejectValue.mLoadGroupStatus);
         self->mDocumentLoadListener = nullptr;
       });
 

netwerk/ipc/DocumentLoadListener.cpp

@@ -14,6 +14,7 @@
 #include "mozilla/StaticPrefs_extensions.h"
 #include "mozilla/StaticPrefs_fission.h"
 #include "mozilla/StaticPrefs_security.h"
+#include "mozilla/dom/BrowserParent.h"
 #include "mozilla/dom/BrowsingContextGroup.h"
 #include "mozilla/dom/CanonicalBrowsingContext.h"
 #include "mozilla/dom/ChildProcessChannelListener.h"
@@ -800,15 +801,9 @@ void DocumentLoadListener::Cancel(const nsresult& aStatusCode) {
       ("DocumentLoadListener Cancel [this=%p, "
        "aStatusCode=%" PRIx32 " ]",
        this, static_cast<uint32_t>(aStatusCode)));
-  mCancelled = true;
-
-  if (mDoingProcessSwitch) {
-    // If we've already initiated process-switching
-    // then we can no longer be cancelled and we'll
-    // disconnect the old listeners when done.
+  if (mOpenPromiseResolved) {
     return;
   }
-
   if (mChannel) {
     mChannel->Cancel(aStatusCode);
   }
@@ -829,12 +824,16 @@ void DocumentLoadListener::DisconnectListeners(nsresult aStatus,
 
   Disconnect();
 
-  // If we're not going to send anything else to the content process, and
-  // we haven't yet consumed a stream filter promise, then we're never going
-  // to.
-  // TODO: This might be because we retargeted the stream to the download
-  // handler or similar. Do we need to attach a stream filter to that?
-  mStreamFilterRequests.Clear();
+  if (!aSwitchedProcess) {
+    // If we're not going to send anything else to the content process, and
+    // we haven't yet consumed a stream filter promise, then we're never going
+    // to. If we're disconnecting the old content process due to a proces
+    // switch, then we can rely on FinishReplacementChannelSetup being called
+    // (even if the switch failed), so we clear at that point instead.
+    // TODO: This might be because we retargeted the stream to the download
+    // handler or similar. Do we need to attach a stream filter to that?
+    mStreamFilterRequests.Clear();
+  }
 }
 
 void DocumentLoadListener::RedirectToRealChannelFinished(nsresult aRv) {
@@ -890,11 +889,7 @@ void DocumentLoadListener::FinishReplacementChannelSetup(nsresult aResult) {
       ctx->EndDocumentLoad(false);
     }
   });
-
-  if (mDoingProcessSwitch) {
-    DisconnectListeners(NS_BINDING_ABORTED, NS_BINDING_ABORTED,
-                        NS_SUCCEEDED(aResult));
-  }
+  mStreamFilterRequests.Clear();
 
   nsCOMPtr<nsIRedirectChannelRegistrar> registrar =
       RedirectChannelRegistrar::GetOrCreate();
@@ -1433,7 +1428,8 @@ bool DocumentLoadListener::MaybeTriggerProcessSwitch(
 
   // Get information about the current document loaded in our BrowsingContext.
   nsCOMPtr<nsIPrincipal> currentPrincipal;
-  if (auto* wgp = browsingContext->GetCurrentWindowGlobal()) {
+  RefPtr<WindowGlobalParent> wgp = browsingContext->GetCurrentWindowGlobal();
+  if (wgp) {
     currentPrincipal = wgp->DocumentPrincipal();
   }
 
@@ -1474,7 +1470,18 @@ bool DocumentLoadListener::MaybeTriggerProcessSwitch(
   LOG(("Process Switch: Changing Remoteness from '%s' to '%s'",
        currentRemoteType.get(), remoteType.get()));
 
+  // We're now committing to a process switch, so we can disconnect from
+  // the listeners in the old process.
   mDoingProcessSwitch = true;
+  if (wgp && wgp->IsProcessRoot()) {
+    if (RefPtr<BrowserParent> browserParent = wgp->GetBrowserParent()) {
+      // This load has already started, so we want to filter out any 'stop'
+      // progress events coming from the old process as a result of us
+      // disconnecting from it.
+      browserParent->SuspendProgressEventsUntilAfterNextLoadStarts();
+    }
+  }
+  DisconnectListeners(NS_BINDING_ABORTED, NS_BINDING_ABORTED, true);
 
   LOG(("Process Switch: Calling ChangeRemoteness"));
   browsingContext

netwerk/ipc/DocumentLoadListener.h

@@ -474,9 +474,6 @@ class DocumentLoadListener : public nsIInterfaceRequestor,
   // passed to the childChannel in order to identify it in the new process.
   uint64_t mLoadIdentifier = 0;
 
-  // True if cancelled.
-  bool mCancelled = false;
-
   Maybe<nsCString> mOriginalUriString;
 
   bool mSupportsRedirectToRealChannel = true;

netwerk/ipc/PDocumentChannel.ipdl

@@ -46,7 +46,7 @@ child:
   // the loadgroup (but aStatus is passed as the parameter to RemoveRequest).
   // We do this so we can remove using NS_BINDING_RETARGETED, but still have the
   // channel not be in an error state.
-  async DisconnectChildListeners(nsresult aStatus, nsresult aLoadGroupReason);
+  async DisconnectChildListeners(nsresult aStatus, nsresult aLoadGroupReason, bool aSwitchedProcess);
 
   // Triggers replacing this DocumentChannel with a 'real' channel (like PHttpChannel),
   // and notifies the listener via a redirect to the new channel.

netwerk/ipc/ParentProcessDocumentChannel.cpp

@@ -204,8 +204,14 @@ NS_IMETHODIMP ParentProcessDocumentChannel::AsyncOpen(
         p->ChainTo(aResolveValue.mPromise.forget(), __func__);
       },
       [self](DocumentLoadListener::OpenPromiseFailedType&& aRejectValue) {
-        self->DisconnectChildListeners(aRejectValue.mStatus,
-                                       aRejectValue.mLoadGroupStatus);
+        // If this is a normal failure, then we want to disconnect our listeners
+        // and notify them of the failure. If this is a process switch, then we
+        // can just ignore it silently, and trust that the switch will shut down
+        // our docshell and cancel us when it's ready.
+        if (!aRejectValue.mSwitchedProcess) {
+          self->DisconnectChildListeners(aRejectValue.mStatus,
+                                         aRejectValue.mLoadGroupStatus);
+        }
         self->RemoveObserver();
       });
   return NS_OK;