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

@@ -8996,29 +8996,28 @@ nsDocShell::CopyFavicon(nsIURI* aOldURI,
 #endif
 }
 
-class InternalLoadEvent : public Runnable
+struct InternalLoadData {
-{
 public:
-  InternalLoadEvent(nsDocShell* aDocShell,
+  InternalLoadData(nsDocShell* aDocShell,
-                    nsIURI* aURI,
+                   nsIURI* aURI,
-                    nsIURI* aOriginalURI,
+                   nsIURI* aOriginalURI,
-                    Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
+                   Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
-                    bool aLoadReplace,
+                   bool aLoadReplace,
-                    nsIURI* aReferrer, uint32_t aReferrerPolicy,
+                   nsIURI* aReferrer,
-                    nsIPrincipal* aTriggeringPrincipal,
+                   uint32_t aReferrerPolicy,
-                    nsIPrincipal* aPrincipalToInherit,
+                   nsIPrincipal* aTriggeringPrincipal,
-                    uint32_t aFlags,
+                   nsIPrincipal* aPrincipalToInherit,
-                    const char* aTypeHint,
+                   uint32_t aFlags,
-                    nsIInputStream* aPostData,
+                   const char* aTypeHint,
-                    nsIInputStream* aHeadersData,
+                   nsIInputStream* aPostData,
-                    uint32_t aLoadType,
+                   nsIInputStream* aHeadersData,
-                    nsISHEntry* aSHEntry,
+                   uint32_t aLoadType,
-                    bool aFirstParty,
+                   nsISHEntry* aSHEntry,
-                    const nsAString& aSrcdoc,
+                   bool aFirstParty,
-                    nsIDocShell* aSourceDocShell,
+                   const nsAString& aSrcdoc,
-                    nsIURI* aBaseURI)
+                   nsIDocShell* aSourceDocShell,
-    : mozilla::Runnable("InternalLoadEvent")
+                   nsIURI* aBaseURI)
-    , mSrcdoc(aSrcdoc)
+    : mSrcdoc(aSrcdoc)
     , mDocShell(aDocShell)
     , mURI(aURI)
     , mOriginalURI(aOriginalURI)
@@ -9045,25 +9044,19 @@ public:
     }
   }
 
-  NS_IMETHOD
+  nsresult Run()
-  Run() override
   {
     return mDocShell->InternalLoad(mURI, mOriginalURI, mResultPrincipalURI,
-                                   mLoadReplace,
+                                   mLoadReplace, mReferrer, mReferrerPolicy,
-                                   mReferrer,
-                                   mReferrerPolicy,
                                    mTriggeringPrincipal, mPrincipalToInherit,
                                    mFlags, EmptyString(),
                                    mTypeHint.IsVoid() ? nullptr
                                                       : mTypeHint.get(),
-                                   VoidString(), mPostData,
+                                   VoidString(), mPostData, mHeadersData, 
-                                   mHeadersData, mLoadType, mSHEntry,
+                                   mLoadType, mSHEntry, mFirstParty, mSrcdoc, 
-                                   mFirstParty, mSrcdoc, mSourceDocShell,
+                                   mSourceDocShell, mBaseURI, nullptr, nullptr);
-                                   mBaseURI, nullptr,
-                                   nullptr);
   }
 
-private:
   nsCString mTypeHint;
   nsString mSrcdoc;
 
@@ -9086,6 +9079,145 @@ private:
   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
@@ -9332,7 +9464,9 @@ nsDocShell::InternalLoad(nsIURI* aURI,
   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
@@ -9345,11 +9479,24 @@ nsDocShell::InternalLoad(nsIURI* aURI,
       return NS_ERROR_DOM_INVALID_ACCESS_ERR;
     }
 
-    bool loadURIHandled = false;
+    RefPtr<dom::Promise> promise;
     rv = mLoadURIDelegate->LoadURI(aURI, where, aFlags, aTriggeringPrincipal,
-                                   &loadURIHandled);
+                                   getter_AddRefs(promise));
-    if (NS_SUCCEEDED(rv) && loadURIHandled) {
+
-      // The request has been handled, nothing to do here.
+    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;
     }
   }

docshell/base/nsIDocShell.idl

@@ -116,6 +116,9 @@ interface nsIDocShell : nsIDocShellTreeItem
   // 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;
 

mobile/android/modules/geckoview/GeckoViewNavigation.jsm

@@ -150,13 +150,25 @@ class GeckoViewNavigation extends GeckoViewModule {
     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,
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
-                             aUri, aWhere, aFlags)) {
       // The app has handled the load, abort open-window handling.
       Components.returnCode = Cr.NS_ERROR_ABORT;
       return null;
@@ -179,8 +191,7 @@ class GeckoViewNavigation extends GeckoViewModule {
                                        nextTabParentId=${aNextTabParentId}
                                        name=${aName}`;
 
-    if (LoadURIDelegate.load(this.window, this.eventDispatcher,
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
-                             aUri, aWhere, aFlags)) {
       // The app has handled the load, abort open-window handling.
       Components.returnCode = Cr.NS_ERROR_ABORT;
       return null;
@@ -200,8 +211,7 @@ class GeckoViewNavigation extends GeckoViewModule {
     debug `handleOpenUri: uri=${aUri && aUri.spec}
                           where=${aWhere} flags=${aFlags}`;
 
-    if (LoadURIDelegate.load(this.window, this.eventDispatcher,
+    if (this.isURIHandled(aUri, aWhere, aFlags)) {
-                             aUri, aWhere, aFlags)) {
       return null;
     }
 

mobile/android/modules/geckoview/LoadURIDelegate.jsm

@@ -17,7 +17,7 @@ var LoadURIDelegate = {
   // Return whether the loading has been handled.
   load: function(aWindow, aEventDispatcher, aUri, aWhere, aFlags) {
     if (!aWindow) {
-      return false;
+      return Promise.resolve(false);
     }
 
     const message = {
@@ -27,17 +27,6 @@ var LoadURIDelegate = {
       flags: aFlags
     };
 
-    let handled = undefined;
+    return aEventDispatcher.sendRequestForResult(message).catch(() => false);
-    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;
   }
 };

xpcom/base/nsILoadURIDelegate.idl

@@ -26,8 +26,11 @@ interface nsILoadURIDelegate : nsISupports
    * @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);
 };