Bug 1620875 - Suppress extra onStateChange(STATE_START) notifications from RemoteWebProgress when switching DocumentChannel with a real channel. r=Gijs,nika

Note that this also suppresses notifications from the initial about:blank in the new process, and updates the tabbrowser to not expect those.

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

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

browser/base/content/tabbrowser.js

@@ -1872,6 +1872,7 @@
       let listener = this._tabListeners.get(tab);
       aBrowser.webProgress.removeProgressListener(filter);
       filter.removeProgressListener(listener);
+      let stateFlags = listener.mStateFlags;
 
       // We'll be creating a new listener, so destroy the old one.
       listener.destroy();
@@ -1925,6 +1926,7 @@
         aBrowser.removeAttribute("remoteType");
       }
 
+      let switchingInProgressLoad = !!redirectLoadSwitchId;
       if (!rebuildFrameLoaders) {
         parent.appendChild(aBrowser);
       } else {
@@ -1934,7 +1936,7 @@
         aBrowser.changeRemoteness({
           remoteType,
           replaceBrowsingContext,
-          switchingInProgressLoad: redirectLoadSwitchId != null,
+          switchingInProgressLoad,
         });
         // Once we have new frameloaders, this call sets the browser back up.
         //
@@ -1956,10 +1958,24 @@
       // browser window is minimized.
       aBrowser.docShellIsActive = this.shouldActivateDocShell(aBrowser);
 
+      // If we're switching an in-progress load, then we shouldn't expect
+      // notification for a new initial about:blank and we should preserve
+      // the existing stateFlags on the TabProgressListener.
+      let expectInitialAboutBlank = !switchingInProgressLoad;
+      if (expectInitialAboutBlank) {
+        stateFlags = 0;
+      }
+
       // Create a new tab progress listener for the new browser we just injected,
       // since tab progress listeners have logic for handling the initial about:blank
       // load
-      listener = new TabProgressListener(tab, aBrowser, true, false);
+      listener = new TabProgressListener(
+        tab,
+        aBrowser,
+        expectInitialAboutBlank,
+        false,
+        stateFlags
+      );
       this._tabListeners.set(tab, listener);
       filter.addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_ALL);
 

browser/base/content/test/general/browser_alltabslistener.js

@@ -186,7 +186,6 @@ var gTestPage =
 const kBasePage =
   "http://mochi.test:8888/browser/browser/base/content/test/general/dummy_page.html";
 var gNextTest;
-var gUsingDocumentChannel;
 
 function setExpectationForCrossDomainFrontBrowserLoad() {
   // In fission, we swap remoteness for this load, and we'll get sent a
@@ -196,7 +195,6 @@ function setExpectationForCrossDomainFrontBrowserLoad() {
     gFrontNotifications = [
       "onStateChange",
       "onSecurityChange",
-      "onStateChange",
       "onLocationChange",
       "onSecurityChange",
       "onStateChange",
@@ -215,10 +213,6 @@ async function test() {
   gForegroundBrowser = gBrowser.getBrowserForTab(gForegroundTab);
   gBrowser.selectedTab = gForegroundTab;
 
-  gUsingDocumentChannel = Services.prefs.getBoolPref(
-    "browser.tabs.documentchannel"
-  );
-
   gAllNotifications = [
     "onStateChange",
     "onLocationChange",
@@ -226,12 +220,6 @@ async function test() {
     "onStateChange",
   ];
 
-  // DocumentChannel issues an extra onStateChange at the
-  // start, when it switches over to the real channel
-  if (gUsingDocumentChannel) {
-    gAllNotifications.unshift("onStateChange");
-  }
-
   // We must wait until a page has completed loading before
   // starting tests or we get notifications from that
   let promises = [

dom/base/nsFrameLoader.cpp

@@ -1807,7 +1807,9 @@ void nsFrameLoader::StartDestroy(bool aForProcessSwitch) {
   if (auto* browserParent = GetBrowserParent()) {
     browserParent->RemoveWindowListeners();
     if (aForProcessSwitch) {
-      browserParent->SetDestroyingForProcessSwitch();
+      // This should suspend all future progress events from this BrowserParent,
+      // since we're going to tear it down after stopping the docshell in it.
+      browserParent->SuspendProgressEventsUntilAfterNextLoadStarts();
     }
   }
 
@@ -2657,6 +2659,7 @@ bool nsFrameLoader::TryRemoteBrowserInternal() {
   if (!mRemoteBrowser) {
     return false;
   }
+
   // If we were given a remote tab ID, we may be attaching to an existing remote
   // browser, which already has its own BrowsingContext. If so, we need to
   // detach our original BC and take ownership of the one from the remote

dom/base/nsFrameLoaderOwner.cpp

@@ -127,6 +127,14 @@ void nsFrameLoaderOwner::ChangeRemotenessCommon(
     }
   }
 
+  // If we're switching process for an in progress load, then suppress
+  // progress events from the new BrowserParent to prevent duplicate
+  // events for the new initial about:blank and the new 'start' event.
+  if (aSwitchingInProgressLoad && mFrameLoader->GetBrowserParent()) {
+    mFrameLoader->GetBrowserParent()
+        ->SuspendProgressEventsUntilAfterNextLoadStarts();
+  }
+
   // Now that we've got a new FrameLoader, we need to reset our
   // nsSubDocumentFrame to use the new FrameLoader.
   if (nsSubDocumentFrame* ourFrame = do_QueryFrame(owner->GetPrimaryFrame())) {

dom/ipc/BrowserChild.cpp

@@ -3608,6 +3608,13 @@ NS_IMETHODIMP BrowserChild::OnStateChange(nsIWebProgress* aWebProgress,
     return NS_OK;
   }
 
+  // We shouldn't need to notify the parent of redirect state changes, since
+  // with DocumentChannel that only happens when we switch to the real channel,
+  // and that's an implementation detail that we can hide.
+  if (aStateFlags & nsIWebProgressListener::STATE_IS_REDIRECTED_DOCUMENT) {
+    return NS_OK;
+  }
+
   RefPtr<Document> document;
   if (nsCOMPtr<nsPIDOMWindowOuter> outerWindow = do_GetInterface(docShell)) {
     document = outerWindow->GetExtantDoc();

dom/ipc/BrowserParent.cpp

@@ -224,7 +224,7 @@ BrowserParent::BrowserParent(ContentParent* aManager, const TabId& aTabId,
       mHasPresented(false),
       mIsReadyToHandleInputEvents(false),
       mIsMouseEnterIntoWidgetEventSuppressed(false),
-      mIsDestroyingForProcessSwitch(false) {
+      mSuspendedProgressEvents(false) {
   MOZ_ASSERT(aManager);
   // When the input event queue is disabled, we don't need to handle the case
   // that some input events are dispatched before PBrowserConstructor.
@@ -2599,14 +2599,22 @@ mozilla::ipc::IPCResult BrowserParent::RecvOnStateChange(
     const RequestData& aRequestData, const uint32_t aStateFlags,
     const nsresult aStatus,
     const Maybe<WebProgressStateChangeData>& aStateChangeData) {
-  // If we're being destroyed because we're swapping to a new process,
-  // then suppress the generated STATE_STOP state changes, since from
-  // the parent process' perspective we aren't really stop, just changing
-  // where the load is happening.
-  uint32_t stopFlags = nsIWebProgressListener::STATE_STOP |
-                       nsIWebProgressListener::STATE_IS_WINDOW |
-                       nsIWebProgressListener::STATE_IS_NETWORK;
-  if (mIsDestroyingForProcessSwitch && (aStateFlags & stopFlags) == stopFlags) {
+  if (mSuspendedProgressEvents) {
+    nsCOMPtr<nsIURI> uri = aRequestData.requestURI();
+    const uint32_t startDocumentFlags =
+        nsIWebProgressListener::STATE_START |
+        nsIWebProgressListener::STATE_IS_DOCUMENT |
+        nsIWebProgressListener::STATE_IS_REQUEST |
+        nsIWebProgressListener::STATE_IS_WINDOW |
+        nsIWebProgressListener::STATE_IS_NETWORK;
+    // Once we get a load start from something that isn't the initial
+    // about:blank, we should stop blocking future state changes.
+    if ((aStateFlags & startDocumentFlags) == startDocumentFlags &&
+        (aWebProgressData && aWebProgressData->isTopLevel()) &&
+        (!uri || !NS_IsAboutBlank(uri))) {
+      mSuspendedProgressEvents = false;
+    }
+
     return IPC_OK();
   }
 
@@ -2651,6 +2659,10 @@ mozilla::ipc::IPCResult BrowserParent::RecvOnProgressChange(
     const RequestData& aRequestData, const int32_t aCurSelfProgress,
     const int32_t aMaxSelfProgress, const int32_t aCurTotalProgress,
     const int32_t aMaxTotalProgress) {
+  if (mSuspendedProgressEvents) {
+    return IPC_OK();
+  }
+
   nsCOMPtr<nsIBrowser> browser;
   nsCOMPtr<nsIWebProgress> manager;
   nsCOMPtr<nsIWebProgressListener> managerAsListener;
@@ -2677,6 +2689,10 @@ mozilla::ipc::IPCResult BrowserParent::RecvOnLocationChange(
     const RequestData& aRequestData, nsIURI* aLocation, const uint32_t aFlags,
     const bool aCanGoBack, const bool aCanGoForward,
     const Maybe<WebProgressLocationChangeData>& aLocationChangeData) {
+  if (mSuspendedProgressEvents) {
+    return IPC_OK();
+  }
+
   nsCOMPtr<nsIBrowser> browser;
   nsCOMPtr<nsIWebProgress> manager;
   nsCOMPtr<nsIWebProgressListener> managerAsListener;
@@ -2724,6 +2740,10 @@ mozilla::ipc::IPCResult BrowserParent::RecvOnStatusChange(
     const Maybe<WebProgressData>& aWebProgressData,
     const RequestData& aRequestData, const nsresult aStatus,
     const nsString& aMessage) {
+  if (mSuspendedProgressEvents) {
+    return IPC_OK();
+  }
+
   nsCOMPtr<nsIBrowser> browser;
   nsCOMPtr<nsIWebProgress> manager;
   nsCOMPtr<nsIWebProgressListener> managerAsListener;

dom/ipc/BrowserParent.h

@@ -738,7 +738,13 @@ class BrowserParent final : public PBrowserParent,
                           uint32_t aPresShellId);
   void StopApzAutoscroll(nsViewID aScrollId, uint32_t aPresShellId);
 
-  void SetDestroyingForProcessSwitch() { mIsDestroyingForProcessSwitch = true; }
+  // Suspend nsIWebProgressListener events until after the next STATE_START
+  // onStateChange. This is used to block STATE_STOP events from the old process
+  // when process switching away, as well as the initial about:blank and
+  // STATE_START from the new process.
+  void SuspendProgressEventsUntilAfterNextLoadStarts() {
+    mSuspendedProgressEvents = true;
+  }
 
  protected:
   friend BrowserBridgeParent;
@@ -1001,10 +1007,11 @@ class BrowserParent final : public PBrowserParent,
   // mouse event and the BrowserChild is ready.
   bool mIsMouseEnterIntoWidgetEventSuppressed : 1;
 
-  // Set to true if we're currently in the middle of replacing this
-  // BrowserParent with a new one connected to a different process, and we
-  // should ignore nsIWebProgressListener stop requests.
-  bool mIsDestroyingForProcessSwitch : 1;
+  // Set to true if we're currently suspending nsIWebProgress events.
+  // We keep suspending until we get a STATE_START onStateChange event
+  // (for something that isn't the initial about:blank) and then start
+  // allowing future events.
+  bool mSuspendedProgressEvents : 1;
 };
 
 struct MOZ_STACK_CLASS BrowserParent::AutoUseNewTab final {

uriloader/base/nsDocLoader.cpp

@@ -1356,8 +1356,8 @@ void nsDocLoader::FireOnLocationChange(nsIWebProgress* aWebProgress,
   NOTIFY_LISTENERS(
       nsIWebProgress::NOTIFY_LOCATION,
       MOZ_LOG(gDocLoaderLog, LogLevel::Debug,
-              ("DocLoader [%p] calling %p->OnLocationChange", this,
-               listener.get()));
+              ("DocLoader [%p] calling %p->OnLocationChange to %s %x", this,
+               listener.get(), aUri->GetSpecOrDefault().get(), aFlags));
       listener->OnLocationChange(aWebProgress, aRequest, aUri, aFlags););
 
   // Pass the notification up to the parent...