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.
2018-08-07 13:42:07 -05:00 · 2018-08-07 13:42:07 -05:00 · 63ca464a8c
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@ -9043,10 +9043,10 @@ nsDocShell::CopyFavicon(nsIURI* aOldURI,
 #endif
 }

-class InternalLoadEvent : public Runnable
+struct InternalLoadData 
 {
 public:
-  InternalLoadEvent(nsDocShell* aDocShell,
+  InternalLoadData(nsDocShell* aDocShell,
                   nsIURI* aURI,
                   nsIURI* aOriginalURI,
                   Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
@ -9056,7 +9056,9 @@ public:
                   nsIPrincipal* aTriggeringPrincipal,
                   nsIPrincipal* aPrincipalToInherit,
                   uint32_t aFlags,
+                   const nsAString& aWindowTarget,
                   const char* aTypeHint,
+                   const nsAString& aFileName,
                   nsIInputStream* aPostData,
                   nsIInputStream* aHeadersData,
                   uint32_t aLoadType,
@ -9064,9 +9066,10 @@ public:
                   bool aFirstParty,
                   const nsAString& aSrcdoc,
                   nsIDocShell* aSourceDocShell,
-                    nsIURI* aBaseURI)
-    : mozilla::Runnable("InternalLoadEvent")
-    , mSrcdoc(aSrcdoc)
+                   nsIURI* aBaseURI,
+                   nsIDocShell** aDocShell2,
+                   nsIRequest** aRequest)
+    : mSrcdoc(aSrcdoc)
    , mDocShell(aDocShell)
    , mURI(aURI)
    , mOriginalURI(aOriginalURI)
@ -9081,10 +9084,14 @@ public:
    , mHeadersData(aHeadersData)
    , mSHEntry(aSHEntry)
    , mFlags(aFlags)
+    , mWindowTarget(aWindowTarget)
+    , mFileName(aFileName)
    , mLoadType(aLoadType)
    , mFirstParty(aFirstParty)
    , mSourceDocShell(aSourceDocShell)
    , mBaseURI(aBaseURI)
+    , mDocShell2(aDocShell2)
+    , mRequest(aRequest)
  {
    // Make sure to keep null things null as needed
    if (aTypeHint) {
@ -9094,8 +9101,7 @@ public:
    }
  }

-  NS_IMETHOD
-  Run() override
+  nsresult Run()
  {
    return mDocShell->InternalLoad(mURI, mOriginalURI, mResultPrincipalURI,
                                   mKeepResultPrincipalURIIfSet,
@ -9103,17 +9109,16 @@ public:
                                   mReferrer,
                                   mReferrerPolicy,
                                   mTriggeringPrincipal, mPrincipalToInherit,
-                                   mFlags, EmptyString(),
+                                   mFlags, mWindowTarget,
                                   mTypeHint.IsVoid() ? nullptr
                                                      : mTypeHint.get(),
-                                   VoidString(), mPostData,
+                                   mFileName, mPostData,
                                   mHeadersData, mLoadType, mSHEntry,
                                   mFirstParty, mSrcdoc, mSourceDocShell,
-                                   mBaseURI, nullptr,
-                                   nullptr);
+                                   mBaseURI, mDocShell2,
+                                   mRequest);
  }

-private:
  nsCString mTypeHint;
  nsString mSrcdoc;

@ -9131,12 +9136,174 @@ private:
  nsCOMPtr<nsIInputStream> mHeadersData;
  nsCOMPtr<nsISHEntry> mSHEntry;
  uint32_t mFlags;
+  nsString mWindowTarget;
+  nsString mFileName;
  uint32_t mLoadType;
  bool mFirstParty;
  nsCOMPtr<nsIDocShell> mSourceDocShell;
  nsCOMPtr<nsIURI> mBaseURI;
+  nsIDocShell** mDocShell2;
+  nsIRequest** mRequest;
 };

+class InternalLoadEvent : public Runnable
+{
+public:
+  InternalLoadEvent(nsDocShell* aDocShell,
+                    nsIURI* aURI,
+                    nsIURI* aOriginalURI,
+                    Maybe<nsCOMPtr<nsIURI>> const& aResultPrincipalURI,
+                    bool aKeepResultPrincipalURIIfSet,
+                    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,
+                aKeepResultPrincipalURIIfSet,
+                aLoadReplace,
+                aReferrer,
+                aReferrerPolicy,
+                aTriggeringPrincipal,
+                aPrincipalToInherit,
+                aFlags,
+                EmptyString(),
+                aTypeHint,
+                VoidString(),
+                aPostData,
+                aHeadersData,
+                aLoadType,
+                aSHEntry,
+                aFirstParty,
+                aSrcdoc,
+                aSourceDocShell,
+                aBaseURI,
+                nullptr,
+                nullptr) 
+  {}
+
+  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 aKeepResultPrincipalURIIfSet,
+                         bool aLoadReplace,
+                         nsIURI* aReferrer,
+                         uint32_t aReferrerPolicy,
+                         nsIPrincipal* aTriggeringPrincipal,
+                         nsIPrincipal* aPrincipalToInherit,
+                         uint32_t aFlags,
+                         const nsAString& aWindowTarget,
+                         const char* aTypeHint,
+                         const nsAString& aFileName,
+                         nsIInputStream* aPostData,
+                         nsIInputStream* aHeadersData,
+                         uint32_t aLoadType,
+                         nsISHEntry* aSHEntry,
+                         bool aFirstParty,
+                         const nsAString& aSrcdoc,
+                         nsIDocShell* aSourceDocShell,
+                         nsIURI* aBaseURI,
+                         nsIDocShell** aDocShell2,
+                         nsIRequest** aRequest)
+    : mLoadData(aDocShell,
+                aURI,
+                aOriginalURI,
+                aResultPrincipalURI,
+                aKeepResultPrincipalURIIfSet,
+                aLoadReplace,
+                aReferrer,
+                aReferrerPolicy,
+                aTriggeringPrincipal,
+                aPrincipalToInherit,
+                aFlags,
+                aWindowTarget,
+                aTypeHint,
+                aFileName,
+                aPostData,
+                aHeadersData,
+                aLoadType,
+                aSHEntry,
+                aFirstParty,
+                aSrcdoc,
+                aSourceDocShell,
+                aBaseURI,
+                aDocShell2,
+                aRequest)
+  {}
+
+  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();
+    } else if (!aValue.isBoolean()) {
+      // If the promise resolves to a non-boolean, let Gecko handle the load
+      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
@ -9384,7 +9551,10 @@ 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);
+  aFlags = 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
@ -9397,11 +9567,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);
-    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,
+                                   aKeepResultPrincipalURIIfSet,
+                                   aLoadReplace, aReferrer, aReferrerPolicy,
+                                   aTriggeringPrincipal, aPrincipalToInherit,
+                                   flags, aWindowTarget, aTypeHint, aFileName, aPostData,
+                                   aHeadersData, aLoadType, aSHEntry, aFirstParty,
+                                   aSrcdoc, aSourceDocShell, aBaseURI, nullptr, nullptr);
+
+      promise->AppendNativeHandler(handler);
+
+      // Checking for load delegates; InternalLoad will be re-called if needed.
      return NS_OK;
    }
  }
--- a/docshell/base/nsIDocShell.idl
+++ b/docshell/base/nsIDocShell.idl
@ -118,6 +118,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;

--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt
@ -79,7 +79,6 @@ class ProgressDelegateTest : BaseSessionTest() {
        })
    }

-    @Ignore
    @Test fun multipleLoads() {
        sessionRule.session.loadUri(INVALID_URI)
        sessionRule.session.loadTestPath(HELLO_HTML_PATH)
--- a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm
+++ b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@ -150,13 +150,24 @@ 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,
-                             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;
@ -179,8 +190,7 @@ class GeckoViewNavigation extends GeckoViewModule {
                                       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;
@ -200,8 +210,7 @@ class GeckoViewNavigation extends GeckoViewModule {
    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;
    }

--- a/mobile/android/modules/geckoview/LoadURIDelegate.jsm
+++ b/mobile/android/modules/geckoview/LoadURIDelegate.jsm
@ -17,7 +17,7 @@ const 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,18 +27,7 @@ const LoadURIDelegate = {
      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).then((response) => response || false).catch(() => false);
  },

  handleLoadError: function(aWindow, aEventDispatcher, aUri, aError,
--- a/xpcom/base/nsILoadURIDelegate.idl
+++ b/xpcom/base/nsILoadURIDelegate.idl
@ -26,10 +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.
-   *
-   * Returns whether the load has been successfully handled.
+   * @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);