Bug 1510569 - Port onStateChange notifications inside WebProgressChild.jsm to C++ r=baku,kmag
☠☠ backed out by 36a9207c79dd ☠ ☠
authorBarret Rennie <barret@brennie.ca>
Tue, 21 May 2019 19:28:52 +0000
changeset 474852 c5488e2770a6273fae9d9ae62ab9bca888354b95
parent 474851 df98eef1f640b109944ec52da0932e62763bbac6
child 474853 723406ecb5f6063834f416606aea688d3cd56a95
push id113174
push usernerli@mozilla.com
push dateWed, 22 May 2019 03:46:05 +0000
treeherdermozilla-inbound@a1672f31906b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, kmag
bugs1510569
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1510569 - Port onStateChange notifications inside WebProgressChild.jsm to C++ r=baku,kmag We now also only access the document when the state is nsIWebProgress::STATE_STOP. The comments in the previous code indicated that touching the document inside the event handler when the state is not STATE_STOP would result in the content creating a new about:blank document to retrieve the values from. However, it then went on to do this in another location, causing a document to be created whenever we received an onStateChange event. This should no longer occur. Differential Revision: https://phabricator.services.mozilla.com/D28125
devtools/client/responsive.html/browser/tunnel.js
dom/interfaces/base/nsIBrowser.idl
dom/ipc/BrowserChild.cpp
dom/ipc/BrowserParent.cpp
dom/ipc/BrowserParent.h
dom/ipc/PBrowser.ipdl
toolkit/content/widgets/browser-custom-element.js
toolkit/modules/RemoteWebProgress.jsm
toolkit/modules/WebProgressChild.jsm
--- a/devtools/client/responsive.html/browser/tunnel.js
+++ b/devtools/client/responsive.html/browser/tunnel.js
@@ -429,17 +429,16 @@ MessageManagerTunnel.prototype = {
     "Link:SetFailedIcon",
     "Link:AddFeed",
     "Link:AddSearch",
     "PageStyle:StyleSheets",
     // Messages sent to RemoteWebProgress.jsm
     "Content:LoadURIResult",
     "Content:LocationChange",
     "Content:SecurityChange",
-    "Content:StateChange",
     // Messages sent to browser.js
     "DOMTitleChanged",
     "ImageDocumentLoaded",
     "Forms:ShowDropDown",
     "Forms:HideDropDown",
     "InPermitUnload",
     "PermitUnload",
     // Messages sent to tabbrowser.xml
--- a/dom/interfaces/base/nsIBrowser.idl
+++ b/dom/interfaces/base/nsIBrowser.idl
@@ -1,15 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 #include "nsISupports.idl"
 
 interface nsIContentSecurityPolicy;
 interface nsIPrincipal;
+interface nsIURI;
 interface nsIWebProgress;
 
 webidl FrameLoader;
 
 [scriptable, uuid(14e5a0cb-e223-4202-95e8-fe53275193ea)]
 interface nsIBrowser : nsISupports
 {
   /**
@@ -75,9 +76,31 @@ interface nsIBrowser : nsISupports
    * @param disabledCommand commands to disable
    */
   void enableDisableCommandsRemoteOnly(in AString action,
                                        in Array<ACString> enabledCommands,
                                        in Array<ACString> disabledCommands);
 
   readonly attribute nsIPrincipal contentPrincipal;
   readonly attribute nsIContentSecurityPolicy csp;
+
+  /**
+   * Whether or not the browser is in the process of an nsIWebNavigation
+   * navigation method.
+   */
+  attribute boolean isNavigating;
+
+  /**
+   * Whether or not the character encoding menu may be enabled.
+   */
+  attribute boolean mayEnableCharacterEncodingMenu;
+
+  /**
+   * Called by Gecko to update the browser when its state changes.
+   *
+   * @param aCharset the new character set of the document
+   * @param aDocumentURI the URI of the current document
+   * @param aContentType the content type of the document
+   */
+  void updateForStateChange(in AString aCharset,
+                            in nsIURI aDocumentURI,
+                            in AString aContentType);
 };
--- a/dom/ipc/BrowserChild.cpp
+++ b/dom/ipc/BrowserChild.cpp
@@ -127,16 +127,17 @@
 #include "mozilla/dom/DocGroup.h"
 #include "nsString.h"
 #include "nsISupportsPrimitives.h"
 #include "mozilla/Telemetry.h"
 #include "nsDocShellLoadState.h"
 #include "nsWebBrowser.h"
 #include "mozilla/dom/WindowGlobalChild.h"
 #include "MMPrinter.h"
+#include "mozilla/ResultExtensions.h"
 
 #ifdef XP_WIN
 #  include "mozilla/plugins/PluginWidgetChild.h"
 #endif
 
 #ifdef NS_PRINTING
 #  include "nsIPrintSession.h"
 #  include "nsIPrintSettings.h"
@@ -538,18 +539,19 @@ nsresult BrowserChild::Init(mozIDOMWindo
   // IPC uses a WebBrowser object for which DNS prefetching is turned off
   // by default. But here we really want it, so enable it explicitly
   mWebBrowser->SetAllowDNSPrefetch(true);
 
   nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation());
   MOZ_ASSERT(docShell);
 
   const uint32_t notifyMask =
-      nsIWebProgress::NOTIFY_PROGRESS | nsIWebProgress::NOTIFY_STATUS |
-      nsIWebProgress::NOTIFY_REFRESH | nsIWebProgress::NOTIFY_CONTENT_BLOCKING;
+      nsIWebProgress::NOTIFY_STATE_ALL | nsIWebProgress::NOTIFY_PROGRESS |
+      nsIWebProgress::NOTIFY_STATUS | nsIWebProgress::NOTIFY_REFRESH |
+      nsIWebProgress::NOTIFY_CONTENT_BLOCKING;
 
   mStatusFilter = new nsBrowserStatusFilter();
 
   RefPtr<nsIEventTarget> eventTarget =
       TabGroup()->EventTargetFor(TaskCategory::Network);
 
   mStatusFilter->SetTarget(eventTarget);
   nsresult rv = mStatusFilter->AddProgressListener(this, notifyMask);
@@ -3469,17 +3471,62 @@ nsresult BrowserChild::SetHasSiblings(bo
   mHasSiblings = aHasSiblings;
   return NS_OK;
 }
 
 NS_IMETHODIMP BrowserChild::OnStateChange(nsIWebProgress* aWebProgress,
                                           nsIRequest* aRequest,
                                           uint32_t aStateFlags,
                                           nsresult aStatus) {
-  return NS_ERROR_NOT_IMPLEMENTED;
+  if (!IPCOpen() || !mShouldSendWebProgressEventsToParent) {
+    return NS_OK;
+  }
+
+  nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation());
+  if (!docShell) {
+    return NS_OK;
+  }
+
+  RefPtr<Document> document;
+  if (nsCOMPtr<nsPIDOMWindowOuter> outerWindow = do_GetInterface(docShell)) {
+    document = outerWindow->GetExtantDoc();
+  } else {
+    return NS_OK;
+  }
+
+  Maybe<WebProgressData> webProgressData;
+  Maybe<WebProgressStateChangeData> stateChangeData;
+  RequestData requestData;
+
+  MOZ_TRY(PrepareProgressListenerData(aWebProgress, aRequest, webProgressData,
+                                      requestData));
+
+  if (webProgressData->isTopLevel()) {
+    stateChangeData.emplace();
+
+    stateChangeData->isNavigating() = docShell->GetIsNavigating();
+    stateChangeData->mayEnableCharacterEncodingMenu() =
+        docShell->GetMayEnableCharacterEncodingMenu();
+
+    if (aStateFlags & nsIWebProgressListener::STATE_STOP) {
+      MOZ_ASSERT(document);
+
+      document->GetContentType(stateChangeData->contentType());
+      document->GetCharacterSet(stateChangeData->charset());
+      stateChangeData->documentURI() = document->GetDocumentURIObject();
+    } else {
+      stateChangeData->contentType().SetIsVoid(true);
+      stateChangeData->charset().SetIsVoid(true);
+    }
+  }
+
+  Unused << SendOnStateChange(webProgressData, requestData, aStateFlags,
+                              aStatus, stateChangeData);
+
+  return NS_OK;
 }
 
 NS_IMETHODIMP BrowserChild::OnProgressChange(nsIWebProgress* aWebProgress,
                                              nsIRequest* aRequest,
                                              int32_t aCurSelfProgress,
                                              int32_t aMaxSelfProgress,
                                              int32_t aCurTotalProgress,
                                              int32_t aMaxTotalProgress) {
--- a/dom/ipc/BrowserParent.cpp
+++ b/dom/ipc/BrowserParent.cpp
@@ -2396,16 +2396,53 @@ mozilla::ipc::IPCResult BrowserParent::R
   if (registrar) {
     registrar->RegisterProtocolHandler(aScheme, aHandlerURI, aTitle, aDocURI,
                                        mFrameElement);
   }
 
   return IPC_OK();
 }
 
+mozilla::ipc::IPCResult BrowserParent::RecvOnStateChange(
+    const Maybe<WebProgressData>& aWebProgressData,
+    const RequestData& aRequestData, const uint32_t aStateFlags,
+    const nsresult aStatus,
+    const Maybe<WebProgressStateChangeData>& aStateChangeData) {
+  nsCOMPtr<nsIBrowser> browser;
+  nsCOMPtr<nsIWebProgress> manager;
+  nsCOMPtr<nsIWebProgressListener> managerAsListener;
+  if (!GetWebProgressListener(getter_AddRefs(browser), getter_AddRefs(manager),
+                              getter_AddRefs(managerAsListener))) {
+    return IPC_OK();
+  }
+
+  nsCOMPtr<nsIWebProgress> webProgress;
+  nsCOMPtr<nsIRequest> request;
+  ReconstructWebProgressAndRequest(manager, aWebProgressData, aRequestData,
+                                   webProgress, request);
+
+  if (aWebProgressData && aWebProgressData->isTopLevel()) {
+    Unused << browser->SetIsNavigating(aStateChangeData->isNavigating());
+    Unused << browser->SetMayEnableCharacterEncodingMenu(
+        aStateChangeData->mayEnableCharacterEncodingMenu());
+    Unused << browser->UpdateForStateChange(aStateChangeData->charset(),
+                                            aStateChangeData->documentURI(),
+                                            aStateChangeData->contentType());
+  } else if (aStateChangeData.isSome()) {
+    return IPC_FAIL(
+        this,
+        "Unexpected WebProgressStateChangeData for non-top-level WebProgress");
+  }
+
+  Unused << managerAsListener->OnStateChange(webProgress, request, aStateFlags,
+                                             aStatus);
+
+  return IPC_OK();
+}
+
 mozilla::ipc::IPCResult BrowserParent::RecvOnProgressChange(
     const Maybe<WebProgressData>& aWebProgressData,
     const RequestData& aRequestData, const int32_t aCurSelfProgress,
     const int32_t aMaxSelfProgress, const int32_t aCurTotalProgress,
     const int32_t aMaxTotalProgress) {
   nsCOMPtr<nsIBrowser> browser;
   nsCOMPtr<nsIWebProgress> manager;
   nsCOMPtr<nsIWebProgressListener> managerAsListener;
--- a/dom/ipc/BrowserParent.h
+++ b/dom/ipc/BrowserParent.h
@@ -267,16 +267,22 @@ class BrowserParent final : public PBrow
 
   mozilla::ipc::IPCResult RecvSetHasBeforeUnload(const bool& aHasBeforeUnload);
 
   mozilla::ipc::IPCResult RecvRegisterProtocolHandler(const nsString& aScheme,
                                                       nsIURI* aHandlerURI,
                                                       const nsString& aTitle,
                                                       nsIURI* aDocURI);
 
+  mozilla::ipc::IPCResult RecvOnStateChange(
+      const Maybe<WebProgressData>& awebProgressData,
+      const RequestData& aRequestData, const uint32_t aStateFlags,
+      const nsresult aStatus,
+      const Maybe<WebProgressStateChangeData>& aStateChangeData);
+
   mozilla::ipc::IPCResult RecvOnProgressChange(
       const Maybe<WebProgressData>& aWebProgressData,
       const RequestData& aRequestData, const int32_t aCurSelfProgress,
       const int32_t aMaxSelfProgress, const int32_t aCurTotalProgres,
       const int32_t aMaxTotalProgress);
 
   mozilla::ipc::IPCResult RecvOnStatusChange(
       const Maybe<WebProgressData>& aWebProgressData,
--- a/dom/ipc/PBrowser.ipdl
+++ b/dom/ipc/PBrowser.ipdl
@@ -113,16 +113,29 @@ struct WebProgressData
 
 struct RequestData
 {
   nsIURI requestURI;
   nsIURI originalRequestURI;
   nsCString matchedList;
 };
 
+struct WebProgressStateChangeData
+{
+  bool isNavigating;
+  bool mayEnableCharacterEncodingMenu;
+
+  // The following fields are only set when the aStateFlags param passed with
+  // this struct is |nsIWebProgress.STATE_STOP|.
+  nsString contentType;
+  nsString charset;
+  nsIURI documentURI;
+};
+
+
 /**
  * A PBrowser manages a maximal locally connected subtree of BrowsingContexts
  * in a content process.
  *
  * See `dom/docs/Fission-IPC-Diagram.svg` for an overview of the DOM IPC
  * actors.
  */
 nested(upto inside_cpow) sync protocol PBrowser
@@ -539,16 +552,21 @@ parent:
 
     async AccessKeyNotHandled(WidgetKeyboardEvent event);
 
     async SetHasBeforeUnload(bool aHasBeforeUnload);
 
     async RegisterProtocolHandler(nsString scheme, nsIURI handlerURI, nsString title,
                                   nsIURI documentURI);
 
+    async OnStateChange(WebProgressData? aWebProgressData,
+                        RequestData aRequestData, uint32_t aStateFlags,
+                        nsresult aStatus,
+                        WebProgressStateChangeData? aStateChangeData);
+
     async OnProgressChange(WebProgressData? aWebProgressData,
                            RequestData aRequestData, int32_t aCurSelfProgress,
                            int32_t aMaxSelfProgress, int32_t aCurTotalProgress,
                            int32_t aMaxTotalProgress);
 
     async OnStatusChange(WebProgressData? aWebProgressData,
                          RequestData aRequestData, nsresult aStatus,
                          nsString aMessage);
--- a/toolkit/content/widgets/browser-custom-element.js
+++ b/toolkit/content/widgets/browser-custom-element.js
@@ -37,16 +37,22 @@ class MozBrowser extends MozElements.Moz
     }
   }
 
   constructor() {
     super();
 
     this.onPageHide = this.onPageHide.bind(this);
 
+    this.isNavigating = false;
+
+    this._documentURI = null;
+    this._characterSet = null;
+    this._documentContentType = null;
+
     /**
      * These are managed by the tabbrowser:
      */
     this.droppedLinkHandler = null;
     this.mIconURL = null;
     this.lastURI = null;
 
     this.addEventListener("keypress", (event) => {
@@ -347,16 +353,26 @@ class MozBrowser extends MozElements.Moz
 
   get documentContentType() {
     if (this.isRemoteBrowser) {
       return this._documentContentType;
     }
     return this.contentDocument ? this.contentDocument.contentType : null;
   }
 
+  set documentContentType(aContentType) {
+    if (aContentType != null) {
+      if (this.isRemoteBrowser) {
+        this._documentContentType = aContentType;
+      } else {
+        this.contentDocument.documentContentType = aContentType;
+      }
+    }
+  }
+
   set sameProcessAsFrameLoader(val) {
     this._sameProcessAsFrameLoader = Cu.getWeakReference(val);
   }
 
   get sameProcessAsFrameLoader() {
     return this._sameProcessAsFrameLoader && this._sameProcessAsFrameLoader.get();
   }
 
@@ -589,16 +605,22 @@ class MozBrowser extends MozElements.Moz
   get characterSet() {
     return this.isRemoteBrowser ? this._characterSet : this.docShell.charset;
   }
 
   get mayEnableCharacterEncodingMenu() {
     return this.isRemoteBrowser ? this._mayEnableCharacterEncodingMenu : this.docShell.mayEnableCharacterEncodingMenu;
   }
 
+  set mayEnableCharacterEncodingMenu(aMayEnable) {
+    if (this.isRemoteBrowser) {
+      this._mayEnableCharacterEncodingMenu = aMayEnable;
+    }
+  }
+
   get contentPrincipal() {
     return this.isRemoteBrowser ? this._contentPrincipal : this.contentDocument.nodePrincipal;
   }
 
   get csp() {
     // After Bug 965637 we can query the csp directly from the contentDocument
     // instead of contentDocument.nodePrincipal.
     return this.isRemoteBrowser ? this._csp : this.contentDocument.nodePrincipal.csp;
@@ -1364,16 +1386,32 @@ class MozBrowser extends MozElements.Moz
       this._securityUI._updateContentBlockingEvent(aEvent);
     }
   }
 
   get remoteWebProgressManager() {
     return this._remoteWebProgressManager;
   }
 
+  updateForStateChange(aCharset, aDocumentURI, aContentType) {
+    if (this.isRemoteBrowser && this.messageManager) {
+      if (aCharset != null) {
+        this._characterSet = aCharset;
+      }
+
+      if (aDocumentURI != null) {
+        this._documentURI = aDocumentURI;
+      }
+
+      if (aContentType != null) {
+        this._documentContentType = aContentType;
+      }
+    }
+  }
+
   purgeSessionHistory() {
     if (this.isRemoteBrowser) {
       try {
         this.messageManager.sendAsyncMessage("Browser:PurgeSessionHistory");
       } catch (ex) {
         // This can throw if the browser has started to go away.
         if (ex.result != Cr.NS_ERROR_NOT_INITIALIZED) {
           throw ex;
--- a/toolkit/modules/RemoteWebProgress.jsm
+++ b/toolkit/modules/RemoteWebProgress.jsm
@@ -23,25 +23,23 @@ class RemoteWebProgressManager {
       /* aIsTopLevel = */ true);
     this._progressListeners = [];
 
     this.swapBrowser(aBrowser);
   }
 
   swapBrowser(aBrowser) {
     if (this._messageManager) {
-      this._messageManager.removeMessageListener("Content:StateChange", this);
       this._messageManager.removeMessageListener("Content:LocationChange", this);
       this._messageManager.removeMessageListener("Content:SecurityChange", this);
       this._messageManager.removeMessageListener("Content:LoadURIResult", this);
     }
 
     this._browser = aBrowser;
     this._messageManager = aBrowser.messageManager;
-    this._messageManager.addMessageListener("Content:StateChange", this);
     this._messageManager.addMessageListener("Content:LocationChange", this);
     this._messageManager.addMessageListener("Content:SecurityChange", this);
     this._messageManager.addMessageListener("Content:LoadURIResult", this);
   }
 
   swapListeners(aOtherRemoteWebProgressManager) {
     let temp = aOtherRemoteWebProgressManager.progressListeners;
     aOtherRemoteWebProgressManager._progressListeners = this._progressListeners;
@@ -198,23 +196,16 @@ class RemoteWebProgressManager {
       }
       if (json.charset) {
         this._browser._characterSet = json.charset;
         this._browser._mayEnableCharacterEncodingMenu = json.mayEnableCharacterEncodingMenu;
       }
     }
 
     switch (aMessage.name) {
-    case "Content:StateChange":
-      if (isTopLevel) {
-        this._browser._documentURI = Services.io.newURI(json.documentURI);
-      }
-      this.onStateChange(webProgress, request, json.stateFlags, json.status);
-      break;
-
     case "Content:LocationChange":
       let location = Services.io.newURI(json.location);
       let flags = json.flags;
       let remoteWebNav = this._browser._remoteWebNavigationImpl;
 
       // These properties can change even for a sub-frame navigation.
       remoteWebNav.canGoBack = json.canGoBack;
       remoteWebNav.canGoForward = json.canGoForward;
--- a/toolkit/modules/WebProgressChild.jsm
+++ b/toolkit/modules/WebProgressChild.jsm
@@ -20,19 +20,20 @@ XPCOMUtils.defineLazyServiceGetter(this,
 XPCOMUtils.defineLazyServiceGetter(this, "serializationHelper",
                                    "@mozilla.org/network/serialization-helper;1",
                                    "nsISerializationHelper");
 
 class WebProgressChild {
   constructor(mm) {
     this.mm = mm;
 
-    // NOTIFY_PROGRESS, NOTIFY_STATUS, NOTIFY_REFRESH, and
+    // NOTIFY_PROGRESS, NOTIFY_STATE_ALL, NOTIFY_STATUS, NOTIFY_REFRESH, and
     // NOTIFY_CONTENT_BLOCKING are handled by PBrowser.
     let notifyCode = Ci.nsIWebProgress.NOTIFY_ALL &
+                        ~Ci.nsIWebProgress.NOTIFY_STATE_ALL &
                         ~Ci.nsIWebProgress.NOTIFY_PROGRESS &
                         ~Ci.nsIWebProgress.NOTIFY_STATUS &
                         ~Ci.nsIWebProgress.NOTIFY_REFRESH &
                         ~Ci.nsIWebProgress.NOTIFY_CONTENT_BLOCKING;
 
     this._filter = Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
                      .createInstance(Ci.nsIWebProgress);
     this._filter.addProgressListener(this, notifyCode);
@@ -94,36 +95,16 @@ class WebProgressChild {
       innerWindowID,
     };
   }
 
   _send(name, data) {
     this.mm.sendAsyncMessage(name, data);
   }
 
-  onStateChange(aWebProgress, aRequest, aStateFlags, aStatus) {
-    let json = this._setupJSON(aWebProgress, aRequest, aStateFlags);
-
-    json.stateFlags = aStateFlags;
-    json.status = aStatus;
-
-    // It's possible that this state change was triggered by
-    // loading an internal error page, for which the parent
-    // will want to know some details, so we'll update it with
-    // the documentURI.
-    if (aWebProgress && aWebProgress.isTopLevel) {
-      json.documentURI = this.mm.content.document.documentURIObject.spec;
-      json.charset = this.mm.content.document.characterSet;
-      json.mayEnableCharacterEncodingMenu = this.mm.docShell.mayEnableCharacterEncodingMenu;
-      json.isNavigating = this.mm.docShell.isNavigating;
-    }
-
-    this._send("Content:StateChange", json);
-  }
-
   onLocationChange(aWebProgress, aRequest, aLocationURI, aFlags) {
     let json = this._setupJSON(aWebProgress, aRequest);
 
     json.location = aLocationURI ? aLocationURI.spec : "";
     json.flags = aFlags;
 
     // These properties can change even for a sub-frame navigation.
     let webNav = this.mm.docShell.QueryInterface(Ci.nsIWebNavigation);