Bug 1515789 - Ensure nsILoadURIDelegate::loadURI() is only called for toplevel redirects. r=smaug,geckoview-reviewers,esawin
authorJames Willcox <snorp@snorp.net>
Mon, 28 Jan 2019 15:35:17 +0000
changeset 455629 6cf4cace23870e27bfa57caaa0a888c9889d25be
parent 455628 a9a40a96a2fe7846871b86167da4d2bbe217aabc
child 455630 7feeb976784f6ae4132ca9fa7f6494ec36cf5365
push id35453
push userrmaries@mozilla.com
push dateMon, 28 Jan 2019 21:44:32 +0000
treeherdermozilla-central@5f877600f76f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, geckoview-reviewers, esawin
bugs1515789
milestone66.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 1515789 - Ensure nsILoadURIDelegate::loadURI() is only called for toplevel redirects. r=smaug,geckoview-reviewers,esawin Differential Revision: https://phabricator.services.mozilla.com/D16990
docshell/base/nsILoadURIDelegate.idl
mobile/android/geckoview/src/androidTest/assets/www/iframe_redirect_automation.html
mobile/android/geckoview/src/androidTest/assets/www/iframe_redirect_local.html
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
uriloader/base/nsDocLoader.cpp
--- a/docshell/base/nsILoadURIDelegate.idl
+++ b/docshell/base/nsILoadURIDelegate.idl
@@ -15,31 +15,32 @@ interface nsIPrincipal;
  * The nsILoadURIDelegate interface.
  * Used for delegating URI loads to GeckoView's application, e.g., Custom Tabs
  * or Progressive Web Apps.
  */
 [scriptable, uuid(78e42d37-a34c-4d96-b901-25385669aba4)]
 interface nsILoadURIDelegate : nsISupports
 {
   /**
-   * Delegates the URI load.
+   * Delegates the URI load. This should only be called for top-level loads.
    *
    * @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.
    *
    * Returns whether the load has been successfully handled.
   */
   boolean
   loadURI(in nsIURI aURI, in short aWhere, in long aFlags,
           in nsIPrincipal aTriggeringPrincipal);
 
   /**
-   * Delegates page load error handling.
+   * Delegates page load error handling. This may be called for either top-level
+   * loads or subframes.
    *
    * @param aURI The URI that failed to load.
    * @param aError The error code.
    * @param aErrorModule The error module code.
 
    * Returns an error page URL to load, or null to show the default error page.
    * No error page is shown at all if an error is thrown.
    */
new file mode 100644
--- /dev/null
+++ b/mobile/android/geckoview/src/androidTest/assets/www/iframe_redirect_automation.html
@@ -0,0 +1,10 @@
+<html>
+<head>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width, initial-scale=1.0">
+</head>
+<body>
+Some stuff
+<iframe src="http://example.org/tests/robocop/simple_redirect.sjs?http://example.org"></iframe>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/mobile/android/geckoview/src/androidTest/assets/www/iframe_redirect_local.html
@@ -0,0 +1,10 @@
+<html>
+<head>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width, initial-scale=1.0">
+</head>
+<body>
+Some stuff
+<iframe src="http://jigsaw.w3.org/HTTP/300/301.html"></iframe>
+</body>
+</html>
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
@@ -42,17 +42,19 @@ open class BaseSessionTest(noErrorCollec
         const val TITLE_CHANGE_HTML_PATH = "/assets/www/titleChange.html"
         const val TRACKERS_PATH = "/assets/www/trackers.html"
         const val VIDEO_OGG_PATH = "/assets/www/ogg.html"
         const val VIDEO_MP4_PATH = "/assets/www/mp4.html"
         const val VIDEO_WEBM_PATH = "/assets/www/webm.html"
         const val VIDEO_BAD_PATH = "/assets/www/badVideoPath.html"
         const val UNKNOWN_HOST_URI = "http://www.test.invalid/"
         const val FULLSCREEN_PATH = "/assets/www/fullscreen.html"
-        const val VEIWPORT_PATH = "/assets/www/viewport.html"
+        const val VIEWPORT_PATH = "/assets/www/viewport.html"
+        const val IFRAME_REDIRECT_LOCAL = "/assets/www/iframe_redirect_local.html"
+        const val IFRAME_REDIRECT_AUTOMATION = "/assets/www/iframe_redirect_automation.html"
     }
 
     @get:Rule val sessionRule = GeckoSessionTestRule()
 
     @get:Rule val errors = ErrorCollector()
 
     val mainSession get() = sessionRule.session
 
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ -235,16 +235,40 @@ class NavigationDelegateTest : BaseSessi
                         equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_CURRENT))
                 assertThat("Redirect flag is set", request.isRedirect,
                         equalTo(forEachCall(false, true)))
                 return null
             }
         })
     }
 
+    @Test fun redirectLoadIframe() {
+        val path = if (sessionRule.env.isAutomation) {
+            IFRAME_REDIRECT_AUTOMATION
+        } else {
+            IFRAME_REDIRECT_LOCAL
+        }
+
+        sessionRule.session.loadTestPath(path)
+        sessionRule.waitForPageStop()
+
+        // We shouldn't be firing onLoadRequest for iframes, including redirects.
+        sessionRule.forCallbacksDuringWait(object : Callbacks.NavigationDelegate {
+            @AssertCalled(count = 1)
+            override fun onLoadRequest(session: GeckoSession,
+                                       request: LoadRequest):
+                    GeckoResult<AllowOrDeny>? {
+                assertThat("Session should not be null", session, notNullValue())
+                assertThat("URI should not be null", request.uri, notNullValue())
+                assertThat("URI should match", request.uri, startsWith("resource://android"))
+                return null
+            }
+        })
+    }
+
     @Test fun bypassClassifier() {
         val phishingUri = "https://www.itisatrap.org/firefox/its-a-trap.html"
 
         sessionRule.runtime.settings.blockPhishing = true
 
         sessionRule.session.loadUri(phishingUri + "?bypass=true",
                                     GeckoSession.LOAD_FLAGS_BYPASS_CLASSIFIER)
         sessionRule.session.waitForPageStop()
@@ -484,17 +508,17 @@ class NavigationDelegateTest : BaseSessi
         userAgent = sessionRule.session.evaluateJS(userAgentJs) as String
         assertThat("User agent should again be reported as VR after disabling override in onLoadRequest",
                 userAgent, containsString(vrSubStr))
     }
 
     @WithDevToolsAPI
     @WithDisplay(width = 600, height = 200)
     @Test fun viewportMode() {
-        sessionRule.session.loadTestPath(VEIWPORT_PATH)
+        sessionRule.session.loadTestPath(VIEWPORT_PATH)
         sessionRule.waitForPageStop()
 
         val desktopInnerWidth = 980.0
         val physicalWidth = 600.0
         val pixelRatio = sessionRule.session.evaluateJS("window.devicePixelRatio") as Double
         val mobileInnerWidth = physicalWidth / pixelRatio
         val innerWidthJs = "window.innerWidth"
 
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ -3035,18 +3035,18 @@ public class GeckoSession implements Par
 
             /**
              * True if and only if the request was triggered by user interaction.
              */
             public final boolean isRedirect;
         }
 
         /**
-         * A request to open an URI. This is called before each page load to
-         * allow custom behavior implementation.
+         * A request to open an URI. This is called before each top-level page load to
+         * allow custom behavior.
          * For example, this can be used to override the behavior of
          * TAGET_WINDOW_NEW requests, which defaults to requesting a new
          * GeckoSession via onNewSession.
          *
          * @param session The GeckoSession that initiated the callback.
          * @param request The {@link LoadRequest} containing the request details.
          *
          * @return A {@link GeckoResult} with a AllowOrDeny value which indicates whether
--- a/uriloader/base/nsDocLoader.cpp
+++ b/uriloader/base/nsDocLoader.cpp
@@ -1,14 +1,15 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* 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 "nspr.h"
+#include "mozilla/dom/Document.h"
 #include "mozilla/Logging.h"
 #include "mozilla/IntegerPrintfMacros.h"
 
 #include "nsDocLoader.h"
 #include "nsCURILoader.h"
 #include "nsNetUtil.h"
 #include "nsIHttpChannel.h"
 #include "nsIWebNavigation.h"
@@ -35,16 +36,17 @@
 #include "mozilla/dom/Document.h"
 #include "nsPresContext.h"
 #include "nsIAsyncVerifyRedirectCallback.h"
 #include "nsILoadURIDelegate.h"
 #include "nsIBrowserDOMWindow.h"
 
 using mozilla::DebugOnly;
 using mozilla::LogLevel;
+using mozilla::dom::Document;
 
 //
 // Log module for nsIDocumentLoader logging...
 //
 // To enable logging (see mozilla/Logging.h for full details):
 //
 //    set MOZ_LOG=DocLoader:5
 //    set MOZ_LOG_FILE=debug.log
@@ -1337,22 +1339,29 @@ NS_IMETHODIMP nsDocLoader::AsyncOnChanne
         do_QueryInterface(static_cast<nsIRequestObserver*>(this));
 
     nsCOMPtr<nsILoadURIDelegate> delegate;
     if (docShell) {
       docShell->GetLoadURIDelegate(getter_AddRefs(delegate));
     }
 
     nsCOMPtr<nsIURI> newURI;
+    nsCOMPtr<nsILoadInfo> info;
     if (delegate) {
       // No point in getting the URI if we don't have a LoadURIDelegate.
       aNewChannel->GetURI(getter_AddRefs(newURI));
+      aNewChannel->GetLoadInfo(getter_AddRefs(info));
     }
 
-    if (newURI) {
+    RefPtr<Document> loadingDoc;
+    if (info) {
+      info->GetLoadingDocument(getter_AddRefs(loadingDoc));
+    }
+
+    if (newURI && info && !loadingDoc) {
       const int where = nsIBrowserDOMWindow::OPEN_CURRENTWINDOW;
       bool loadURIHandled = false;
       nsresult rv = delegate->LoadURI(
           newURI, where, nsIWebNavigation::LOAD_FLAGS_IS_REDIRECT,
           /* triggering principal */ nullptr, &loadURIHandled);
       if (NS_SUCCEEDED(rv) && loadURIHandled) {
         cb->OnRedirectVerifyCallback(NS_OK);
         return NS_OK;