Bug 1441059 - Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz
☠☠ backed out by ba9884ab1267 ☠ ☠
authorDylan Roeh <droeh@mozilla.com>
Mon, 23 Jul 2018 10:12:18 -0500
changeset 427813 9a8f58cb7315a92f9ebede217e1b8d968c5e5a1b
parent 427812 d3ecceff8d68c16b27fabe65150dca3bd36e4dd4
child 427814 ccef9e2643a19668dc06426aa0e55ed5d61ae535
push id34318
push userccoroiu@mozilla.com
push dateMon, 23 Jul 2018 21:44:05 +0000
treeherdermozilla-central@fe48e26ca88c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssnorp, bz
bugs1441059
milestone63.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 1441059 - Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz This alters nsILoadURIDelegate.loadURI() to return a Promise rather than spinning the event loop to synchronously return a boolean, and alters nsDocShell::InternalLoad to allow for those changes by re-calling itself if necessary based on the resolution of the promise.
docshell/base/nsDocShell.cpp
docshell/base/nsIDocShell.idl
mobile/android/modules/geckoview/GeckoViewNavigation.jsm
mobile/android/modules/geckoview/LoadURIDelegate.jsm
xpcom/base/nsILoadURIDelegate.idl
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -8991,39 +8991,38 @@ nsDocShell::CopyFavicon(nsIURI* aOldURI,
   if (favSvc) {
     favSvc->CopyFavicons(aOldURI, aNewURI,
       aInPrivateBrowsing ? nsIFaviconService::FAVICON_LOAD_PRIVATE
                          : nsIFaviconService::FAVICON_LOAD_NON_PRIVATE, nullptr);
   }
 #endif
 }
 
-class InternalLoadEvent : public Runnable
-{
+struct InternalLoadData {
 public:
-  InternalLoadEvent(nsDocShell* aDocShell,
-                    nsIURI* aURI,
-                    nsIURI* aOriginalURI,
-                    Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
-                    bool aLoadReplace,
-                    nsIURI* aReferrer, uint32_t aReferrerPolicy,
-                    nsIPrincipal* aTriggeringPrincipal,
-                    nsIPrincipal* aPrincipalToInherit,
-                    uint32_t aFlags,
-                    const char* aTypeHint,
-                    nsIInputStream* aPostData,
-                    nsIInputStream* aHeadersData,
-                    uint32_t aLoadType,
-                    nsISHEntry* aSHEntry,
-                    bool aFirstParty,
-                    const nsAString& aSrcdoc,
-                    nsIDocShell* aSourceDocShell,
-                    nsIURI* aBaseURI)
-    : mozilla::Runnable("InternalLoadEvent")
-    , mSrcdoc(aSrcdoc)
+  InternalLoadData(nsDocShell* aDocShell,
+                   nsIURI* aURI,
+                   nsIURI* aOriginalURI,
+                   Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
+                   bool aLoadReplace,
+                   nsIURI* aReferrer,
+                   uint32_t aReferrerPolicy,
+                   nsIPrincipal* aTriggeringPrincipal,
+                   nsIPrincipal* aPrincipalToInherit,
+                   uint32_t aFlags,
+                   const char* aTypeHint,
+                   nsIInputStream* aPostData,
+                   nsIInputStream* aHeadersData,
+                   uint32_t aLoadType,
+                   nsISHEntry* aSHEntry,
+                   bool aFirstParty,
+                   const nsAString& aSrcdoc,
+                   nsIDocShell* aSourceDocShell,
+                   nsIURI* aBaseURI)
+    : mSrcdoc(aSrcdoc)
     , mDocShell(aDocShell)
     , mURI(aURI)
     , mOriginalURI(aOriginalURI)
     , mResultPrincipalURI(aResultPrincipalURI)
     , mLoadReplace(aLoadReplace)
     , mReferrer(aReferrer)
     , mReferrerPolicy(aReferrerPolicy)
     , mTriggeringPrincipal(aTriggeringPrincipal)
@@ -9040,35 +9039,29 @@ public:
     // Make sure to keep null things null as needed
     if (aTypeHint) {
       mTypeHint = aTypeHint;
     } else {
       mTypeHint.SetIsVoid(true);
     }
   }
 
-  NS_IMETHOD
-  Run() override
+  nsresult Run()
   {
     return mDocShell->InternalLoad(mURI, mOriginalURI, mResultPrincipalURI,
-                                   mLoadReplace,
-                                   mReferrer,
-                                   mReferrerPolicy,
+                                   mLoadReplace, mReferrer, mReferrerPolicy,
                                    mTriggeringPrincipal, mPrincipalToInherit,
                                    mFlags, EmptyString(),
                                    mTypeHint.IsVoid() ? nullptr
                                                       : mTypeHint.get(),
-                                   VoidString(), mPostData,
-                                   mHeadersData, mLoadType, mSHEntry,
-                                   mFirstParty, mSrcdoc, mSourceDocShell,
-                                   mBaseURI, nullptr,
-                                   nullptr);
-  }
-
-private:
+                                   VoidString(), mPostData, mHeadersData, 
+                                   mLoadType, mSHEntry, mFirstParty, mSrcdoc, 
+                                   mSourceDocShell, mBaseURI, nullptr, nullptr);
+  }
+
   nsCString mTypeHint;
   nsString mSrcdoc;
 
   RefPtr<nsDocShell> mDocShell;
   nsCOMPtr<nsIURI> mURI;
   nsCOMPtr<nsIURI> mOriginalURI;
   Maybe<nsCOMPtr<nsIURI>> mResultPrincipalURI;
   bool mLoadReplace;
@@ -9081,16 +9074,155 @@ private:
   nsCOMPtr<nsISHEntry> mSHEntry;
   uint32_t mFlags;
   uint32_t mLoadType;
   bool mFirstParty;
   nsCOMPtr<nsIDocShell> mSourceDocShell;
   nsCOMPtr<nsIURI> mBaseURI;
 };
 
+class InternalLoadEvent : public Runnable
+{
+public:
+  InternalLoadEvent(nsDocShell* aDocShell,
+                    nsIURI* aURI,
+                    nsIURI* aOriginalURI,
+                    Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
+                    bool aLoadReplace,
+                    nsIURI* aReferrer,
+                    uint32_t aReferrerPolicy,
+                    nsIPrincipal* aTriggeringPrincipal,
+                    nsIPrincipal* aPrincipalToInherit,
+                    uint32_t aFlags,
+                    const char* aTypeHint,
+                    nsIInputStream* aPostData,
+                    nsIInputStream* aHeadersData,
+                    uint32_t aLoadType,
+                    nsISHEntry* aSHEntry,
+                    bool aFirstParty,
+                    const nsAString& aSrcdoc,
+                    nsIDocShell* aSourceDocShell,
+                    nsIURI* aBaseURI)
+    : mozilla::Runnable("InternalLoadEvent")
+    , mLoadData(aDocShell,
+                aURI,
+                aOriginalURI,
+                aResultPrincipalURI,
+                aLoadReplace,
+                aReferrer,
+                aReferrerPolicy,
+                aTriggeringPrincipal,
+                aPrincipalToInherit,
+                aFlags,
+                aTypeHint,
+                aPostData,
+                aHeadersData,
+                aLoadType,
+                aSHEntry,
+                aFirstParty,
+                aSrcdoc,
+                aSourceDocShell,
+                aBaseURI) 
+  {}
+
+  NS_IMETHOD
+  Run() override
+  {
+    return mLoadData.Run();
+  }
+
+private:
+  InternalLoadData mLoadData;
+};
+
+class LoadURIDelegateHandler final : public PromiseNativeHandler
+{
+public:
+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTION_CLASS(LoadURIDelegateHandler)
+
+  LoadURIDelegateHandler(nsDocShell* aDocShell,
+                         nsIURI* aURI,
+                         nsIURI* aOriginalURI,
+                         Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
+                         bool aLoadReplace,
+                         nsIURI* aReferrer,
+                         uint32_t aReferrerPolicy,
+                         nsIPrincipal* aTriggeringPrincipal,
+                         nsIPrincipal* aPrincipalToInherit,
+                         uint32_t aFlags,
+                         const char* aTypeHint,
+                         nsIInputStream* aPostData,
+                         nsIInputStream* aHeadersData,
+                         uint32_t aLoadType,
+                         nsISHEntry* aSHEntry,
+                         bool aFirstParty,
+                         const nsAString& aSrcdoc,
+                         nsIDocShell* aSourceDocShell,
+                         nsIURI* aBaseURI)
+    : mLoadData(aDocShell,
+                aURI,
+                aOriginalURI,
+                aResultPrincipalURI,
+                aLoadReplace,
+                aReferrer,
+                aReferrerPolicy,
+                aTriggeringPrincipal,
+                aPrincipalToInherit,
+                aFlags,
+                aTypeHint,
+                aPostData,
+                aHeadersData,
+                aLoadType,
+                aSHEntry,
+                aFirstParty,
+                aSrcdoc,
+                aSourceDocShell,
+                aBaseURI)
+  {}
+
+  void
+  ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
+  {
+    if (aValue.isBoolean() && !aValue.toBoolean()) {
+      // Things went fine, not handled by app, let gecko do its thing
+      mLoadData.Run();
+    }
+  }
+
+  void
+  RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
+  {
+    // In the event of a rejected callback, let Gecko handle the load
+    mLoadData.Run();
+  }
+
+private:
+  ~LoadURIDelegateHandler()
+  {}
+
+  InternalLoadData mLoadData;
+};
+
+NS_IMPL_CYCLE_COLLECTION(LoadURIDelegateHandler, mLoadData.mDocShell,
+                         mLoadData.mURI, mLoadData.mOriginalURI,
+                         mLoadData.mResultPrincipalURI, mLoadData.mReferrer,
+                         mLoadData.mTriggeringPrincipal,
+                         mLoadData.mPrincipalToInherit, 
+                         mLoadData.mPostData, mLoadData.mHeadersData,
+                         mLoadData.mSHEntry, mLoadData.mSourceDocShell,
+                         mLoadData.mBaseURI)
+
+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(LoadURIDelegateHandler)
+  NS_INTERFACE_MAP_ENTRY(nsISupports)
+NS_INTERFACE_MAP_END
+
+NS_IMPL_CYCLE_COLLECTING_ADDREF(LoadURIDelegateHandler)
+NS_IMPL_CYCLE_COLLECTING_RELEASE(LoadURIDelegateHandler)
+
 /**
  * Returns true if we started an asynchronous load (i.e., from the network), but
  * the document we're loading there hasn't yet become this docshell's active
  * document.
  *
  * When JustStartedNetworkLoad is true, you should be careful about modifying
  * mLoadType and mLSHE.  These are both set when the asynchronous load first
  * starts, and the load expects that, when it eventually runs InternalLoad,
@@ -9327,34 +9459,49 @@ nsDocShell::InternalLoad(nsIURI* aURI,
   }
 
   nsIDocument* doc = mContentViewer ? mContentViewer->GetDocument()
                                     : nullptr;
 
   const bool isDocumentAuxSandboxed = doc &&
     (doc->GetSandboxFlags() & SANDBOXED_AUXILIARY_NAVIGATION);
 
-  if (aURI && mLoadURIDelegate &&
+  const bool checkLoadDelegates = !(aFlags & INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED);
+
+  if (aURI && mLoadURIDelegate && checkLoadDelegates &&
       (!targetDocShell || targetDocShell == static_cast<nsIDocShell*>(this))) {
     // Dispatch only load requests for the current or a new window to the
     // delegate, e.g., to allow for GeckoView apps to handle the load event
     // outside of Gecko.
     const int where = (aWindowTarget.IsEmpty() || targetDocShell)
                       ? nsIBrowserDOMWindow::OPEN_CURRENTWINDOW
                       : nsIBrowserDOMWindow::OPEN_NEWWINDOW;
 
     if (where == nsIBrowserDOMWindow::OPEN_NEWWINDOW && isDocumentAuxSandboxed) {
       return NS_ERROR_DOM_INVALID_ACCESS_ERR;
     }
 
-    bool loadURIHandled = false;
+    RefPtr<dom::Promise> promise;
     rv = mLoadURIDelegate->LoadURI(aURI, where, aFlags, aTriggeringPrincipal,
-                                   &loadURIHandled);
-    if (NS_SUCCEEDED(rv) && loadURIHandled) {
-      // The request has been handled, nothing to do here.
+                                   getter_AddRefs(promise));
+
+    if (NS_SUCCEEDED(rv) && promise) {
+      const uint32_t flags = aFlags | INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED;
+
+      RefPtr<LoadURIDelegateHandler> handler = 
+        new LoadURIDelegateHandler(this, aURI, aOriginalURI, aResultPrincipalURI,
+                                   aLoadReplace, aReferrer, aReferrerPolicy,
+                                   aTriggeringPrincipal, principalToInherit,
+                                   flags, aTypeHint, aPostData,
+                                   aHeadersData, aLoadType, aSHEntry, aFirstParty,
+                                   aSrcdoc, aSourceDocShell, aBaseURI);
+
+      promise->AppendNativeHandler(handler);
+
+      // Checking for load delegates; InternalLoad will be re-called if needed.
       return NS_OK;
     }
   }
 
   //
   // Resolve the window target before going any further...
   // If the load has been targeted to another DocShell, then transfer the
   // load to it...
--- a/docshell/base/nsIDocShell.idl
+++ b/docshell/base/nsIDocShell.idl
@@ -111,16 +111,19 @@ interface nsIDocShell : nsIDocShellTreeI
   // Whether this is the load of a frame's original src attribute
   const long INTERNAL_LOAD_FLAGS_ORIGINAL_FRAME_SRC      = 0x80;
 
   const long INTERNAL_LOAD_FLAGS_NO_OPENER               = 0x100;
 
   // Whether a top-level data URI navigation is allowed for that load
   const long INTERNAL_LOAD_FLAGS_FORCE_ALLOW_DATA_URI    = 0x200;
 
+  // Whether load delegates have already been checked for this load
+  const long INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED       = 0x400;
+
   // Whether the load was triggered by user interaction.
   const long INTERNAL_LOAD_FLAGS_IS_USER_TRIGGERED       = 0x1000;
 
   /**
    * Loads the given URI.  This method is identical to loadURI(...) except
    * that its parameter list is broken out instead of being packaged inside
    * of an nsIDocShellLoadInfo object...
    *
--- a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm
+++ b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ -145,23 +145,35 @@ class GeckoViewNavigation extends GeckoV
     });
 
     // Wait indefinitely for app to respond with a browser or null
     Services.tm.spinEventLoopUntil(() =>
         this.window.closed || browser !== undefined);
     return browser || null;
   }
 
+  isURIHandled(aUri, aWhere, aFlags) {
+    debug `isURIHandled: uri=${aUri} where=${aWhere} flags=${aFlags}`;
+
+    let handled = undefined;
+    LoadURIDelegate.load(this.window, this.eventDispatcher, aUri, aWhere, aFlags).then((response) => {
+      handled = response;
+    });
+
+    Services.tm.spinEventLoopUntil(() => this.window.closed || handled !== undefined);
+
+    return handled;
+  }
+
   // nsIBrowserDOMWindow.
   createContentWindow(aUri, aOpener, aWhere, aFlags, aTriggeringPrincipal) {
     debug `createContentWindow: uri=${aUri && aUri.spec}
                                 where=${aWhere} flags=${aFlags}`;
 
-    if (LoadURIDelegate.load(this.window, this.eventDispatcher,
-                             aUri, aWhere, aFlags)) {
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
       // The app has handled the load, abort open-window handling.
       Components.returnCode = Cr.NS_ERROR_ABORT;
       return null;
     }
 
     const browser = this.handleNewSession(aUri, aOpener, aWhere, aFlags, null);
     if (!browser) {
       Components.returnCode = Cr.NS_ERROR_ABORT;
@@ -174,18 +186,17 @@ class GeckoViewNavigation extends GeckoV
   // nsIBrowserDOMWindow.
   createContentWindowInFrame(aUri, aParams, aWhere, aFlags, aNextTabParentId,
                              aName) {
     debug `createContentWindowInFrame: uri=${aUri && aUri.spec}
                                        where=${aWhere} flags=${aFlags}
                                        nextTabParentId=${aNextTabParentId}
                                        name=${aName}`;
 
-    if (LoadURIDelegate.load(this.window, this.eventDispatcher,
-                             aUri, aWhere, aFlags)) {
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
       // The app has handled the load, abort open-window handling.
       Components.returnCode = Cr.NS_ERROR_ABORT;
       return null;
     }
 
     const browser = this.handleNewSession(aUri, null, aWhere, aFlags, aNextTabParentId);
     if (!browser) {
       Components.returnCode = Cr.NS_ERROR_ABORT;
@@ -195,18 +206,17 @@ class GeckoViewNavigation extends GeckoV
     return browser;
   }
 
   handleOpenUri(aUri, aOpener, aWhere, aFlags, aTriggeringPrincipal,
                 aNextTabParentId) {
     debug `handleOpenUri: uri=${aUri && aUri.spec}
                           where=${aWhere} flags=${aFlags}`;
 
-    if (LoadURIDelegate.load(this.window, this.eventDispatcher,
-                             aUri, aWhere, aFlags)) {
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
       return null;
     }
 
     let browser = this.browser;
 
     if (aWhere === Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW ||
         aWhere === Ci.nsIBrowserDOMWindow.OPEN_NEWTAB ||
         aWhere === Ci.nsIBrowserDOMWindow.OPEN_SWITCHTAB) {
--- a/mobile/android/modules/geckoview/LoadURIDelegate.jsm
+++ b/mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ -12,32 +12,21 @@ XPCOMUtils.defineLazyModuleGetters(this,
   Services: "resource://gre/modules/Services.jsm",
 });
 
 var LoadURIDelegate = {
   // Delegate URI loading to the app.
   // Return whether the loading has been handled.
   load: function(aWindow, aEventDispatcher, aUri, aWhere, aFlags) {
     if (!aWindow) {
-      return false;
+      return Promise.resolve(false);
     }
 
     const message = {
       type: "GeckoView:OnLoadRequest",
       uri: aUri ? aUri.displaySpec : "",
       where: aWhere,
       flags: aFlags
     };
 
-    let handled = undefined;
-    aEventDispatcher.sendRequestForResult(message).then(response => {
-      handled = response;
-    }, () => {
-      // There was an error or listener was not registered in GeckoSession,
-      // treat as unhandled.
-      handled = false;
-    });
-    Services.tm.spinEventLoopUntil(() =>
-        aWindow.closed || handled !== undefined);
-
-    return handled || false;
+    return aEventDispatcher.sendRequestForResult(message).catch(() => false);
   }
 };
--- a/xpcom/base/nsILoadURIDelegate.idl
+++ b/xpcom/base/nsILoadURIDelegate.idl
@@ -21,13 +21,16 @@ interface nsILoadURIDelegate : nsISuppor
 {
   /**
    * Delegates the URI load.
    *
    * @param aURI The URI to load.
    * @param aWhere See possible values described in nsIBrowserDOMWindow.
    * @param aFlags Flags which control the behavior of the load.
    * @param aTriggeringPrincipal The principal that triggered the load of aURI.
+   * @return A promise which can resolve to a boolean indicating whether or
+   *         not the app handled the load. Rejection should be treated the same
+   *         as a false resolution.
   */
-  boolean
+  Promise
   loadURI(in nsIURI aURI, in short aWhere, in long aFlags,
           in nsIPrincipal aTriggeringPrincipal);
 };