Bug 1510569 - Port onStateChange notifications inside WebProgressChild.jsm to C++ r=baku,kmag
☠☠ backed out by 9186bf351f2c ☠ ☠
authorBarret Rennie <barret@brennie.ca>
Thu, 02 May 2019 23:36:24 +0000
changeset 531213 fc0ae629221af74287f6df6f2ffb8bcc0b52c123
parent 531212 97f6ac273b5dcc04a553e4dec24362da2dbc5809
child 531214 3edbe004f8ac490b35bedf15009351b7b9e92384
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, kmag
bugs1510569
milestone68.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
@@ -431,17 +431,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
 {
   /**
@@ -81,9 +82,31 @@ interface nsIBrowser : nsISupports
   void enableDisableCommandsRemoteOnly(in AString action,
                                        in unsigned long enabledLength,
                                        [array, size_is(enabledLength)] in string enabledCommands,
                                        in unsigned long disabledLength,
                                        [array, size_is(disabledLength)] in string 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
@@ -126,16 +126,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);
@@ -3431,17 +3433,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()) {
+    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
@@ -2268,16 +2268,64 @@ 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 =
+      mFrameElement ? mFrameElement->AsBrowser() : nullptr;
+
+  if (!browser) {
+    return IPC_OK();
+  }
+
+  nsCOMPtr<nsIWebProgress> manager;
+  nsresult rv = browser->GetRemoteWebProgressManager(getter_AddRefs(manager));
+  NS_ENSURE_SUCCESS(rv, IPC_OK());
+
+  nsCOMPtr<nsIWebProgressListener> managerAsListener =
+      do_QueryInterface(manager);
+
+  if (!managerAsListener) {
+    // We are no longer remote, so we cannot propagate this message.
+    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 =
       mFrameElement ? mFrameElement->AsBrowser() : nullptr;
 
--- a/dom/ipc/BrowserParent.h
+++ b/dom/ipc/BrowserParent.h
@@ -165,16 +165,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,28 @@ 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;
+};
+
 nested(upto inside_cpow) sync protocol PBrowser
 {
     manager PContent;
 
     manages PColorPicker;
     manages PDocAccessible;
     manages PFilePicker;
     manages PPluginWidget;
@@ -532,16 +544,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;
@@ -1365,16 +1387,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);