Bug 1531289 - target=_blank with a download should close the download tab, r=nika,Gijs

Differential Revision: https://phabricator.services.mozilla.com/D66454

--HG--
extra : moz-landing-system : lando

browser/base/content/browser.js

@@ -6206,13 +6206,18 @@ nsBrowserAccess.prototype = {
         browsingContext =
           window.content && BrowsingContext.getFromWindow(window.content);
         if (aURI) {
-          let loadflags = isExternal
-            ? Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL
-            : Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
+          let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
+          if (isExternal) {
+            loadFlags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
+          } else if (!aTriggeringPrincipal.isSystemPrincipal) {
+            // XXX this code must be reviewed and changed when bug 1616353
+            // lands.
+            loadFlags |= Ci.nsIWebNavigation.LOAD_FLAGS_FIRST_LOAD;
+          }
           gBrowser.loadURI(aURI.spec, {
             triggeringPrincipal: aTriggeringPrincipal,
             csp: aCsp,
-            flags: loadflags,
+            loadFlags,
             referrerInfo,
           });
         }

browser/base/content/tabbrowser.js

@@ -2845,6 +2845,10 @@
           }
           if (fromExternal) {
             flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
+          } else if (!triggeringPrincipal.isSystemPrincipal) {
+            // XXX this code must be reviewed and changed when bug 1616353
+            // lands.
+            flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FIRST_LOAD;
           }
           if (allowMixedContent) {
             flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;

docshell/base/nsDSURIContentListener.cpp

@@ -10,7 +10,9 @@
 #include "nsServiceManagerUtils.h"
 #include "nsDocShellCID.h"
 #include "nsIWebNavigationInfo.h"
+#include "mozilla/dom/CanonicalBrowsingContext.h"
 #include "mozilla/dom/Document.h"
+#include "mozilla/Unused.h"
 #include "nsError.h"
 #include "nsContentSecurityManager.h"
 #include "nsDocShellLoadTypes.h"
@@ -41,25 +43,54 @@ void MaybeCloseWindowHelper::SetShouldCloseWindow(bool aShouldCloseWindow) {
 }
 
 BrowsingContext* MaybeCloseWindowHelper::MaybeCloseWindow() {
-  if (mShouldCloseWindow) {
-    // Reset the window context to the opener window so that the dependent
-    // dialogs have a parent
-    RefPtr<BrowsingContext> opener = mBrowsingContext->GetOpener();
-
-    if (opener && !opener->IsDiscarded()) {
-      mBCToClose = mBrowsingContext;
-      mBrowsingContext = opener;
-
-      // Now close the old window.  Do it on a timer so that we don't run
-      // into issues trying to close the window before it has fully opened.
-      NS_ASSERTION(!mTimer, "mTimer was already initialized once!");
-      NS_NewTimerWithCallback(getter_AddRefs(mTimer), this, 0,
-                              nsITimer::TYPE_ONE_SHOT);
-    }
+  if (!mShouldCloseWindow) {
+    return mBrowsingContext;
   }
+
+  // This method should not be called more than once, but it's better to avoid
+  // closing the current window again.
+  mShouldCloseWindow = false;
+
+  // Reset the window context to the opener window so that the dependent
+  // dialogs have a parent
+  RefPtr<BrowsingContext> newBC = ChooseNewBrowsingContext(mBrowsingContext);
+
+  if (newBC != mBrowsingContext && newBC && !newBC->IsDiscarded()) {
+    mBCToClose = mBrowsingContext;
+    mBrowsingContext = newBC;
+
+    // Now close the old window.  Do it on a timer so that we don't run
+    // into issues trying to close the window before it has fully opened.
+    NS_ASSERTION(!mTimer, "mTimer was already initialized once!");
+    NS_NewTimerWithCallback(getter_AddRefs(mTimer), this, 0,
+                            nsITimer::TYPE_ONE_SHOT);
+  }
+
   return mBrowsingContext;
 }
 
+already_AddRefed<BrowsingContext>
+MaybeCloseWindowHelper::ChooseNewBrowsingContext(BrowsingContext* aBC) {
+  RefPtr<BrowsingContext> bc = aBC;
+
+  RefPtr<BrowsingContext> opener = bc->GetOpener();
+  if (opener && !opener->IsDiscarded()) {
+    return opener.forget();
+  }
+
+  if (!XRE_IsParentProcess()) {
+    return bc.forget();
+  }
+
+  CanonicalBrowsingContext* cbc = CanonicalBrowsingContext::Cast(aBC);
+  RefPtr<WindowGlobalParent> wgp = cbc->GetEmbedderWindowGlobal();
+  if (!wgp) {
+    return bc.forget();
+  }
+
+  return do_AddRef(wgp->BrowsingContext());
+}
+
 NS_IMETHODIMP
 MaybeCloseWindowHelper::Notify(nsITimer* timer) {
   NS_ASSERTION(mBCToClose, "No window to close after timer fired");
@@ -135,7 +166,7 @@ nsDSURIContentListener::DoContent(const nsACString& aContentType,
         RefPtr<MaybeCloseWindowHelper> maybeCloseWindowHelper =
             new MaybeCloseWindowHelper(mDocShell->GetBrowsingContext());
         maybeCloseWindowHelper->SetShouldCloseWindow(true);
-        maybeCloseWindowHelper->MaybeCloseWindow();
+        Unused << maybeCloseWindowHelper->MaybeCloseWindow();
       }
       return NS_OK;
     }

docshell/base/nsDSURIContentListener.h

@@ -17,7 +17,7 @@ class nsIInterfaceRequestor;
 class nsIWebNavigationInfo;
 class nsPIDOMWindowOuter;
 
-// Helper Class to eventually close an already openend window
+// Helper Class to eventually close an already opened window
 class MaybeCloseWindowHelper final : public nsITimerCallback {
  public:
   NS_DECL_ISUPPORTS
@@ -27,9 +27,12 @@ class MaybeCloseWindowHelper final : public nsITimerCallback {
       mozilla::dom::BrowsingContext* aContentContext);
 
   /**
-   * Closes the provided window async (if mShouldCloseWindow is true)
-   * and returns its opener if the window was just opened. Otherwise
-   * returns the BrowsingContext provided in the constructor.
+   * Closes the provided window async (if mShouldCloseWindow is true) and
+   * returns a valid browsingContext to be used instead as parent for dialogs or
+   * similar things.
+   * In case mShouldCloseWindow is true, the final browsing context will be the
+   * a valid new chrome window to use. It can be the opener, or the opener's
+   * top, or the top chrome window.
    */
   mozilla::dom::BrowsingContext* MaybeCloseWindow();
 
@@ -39,6 +42,9 @@ class MaybeCloseWindowHelper final : public nsITimerCallback {
   ~MaybeCloseWindowHelper();
 
  private:
+  already_AddRefed<mozilla::dom::BrowsingContext> ChooseNewBrowsingContext(
+      mozilla::dom::BrowsingContext* aBC);
+
   /**
    * The dom window associated to handle content.
    */

uriloader/exthandler/tests/mochitest/browser_auto_close_window.js

@@ -100,6 +100,62 @@ add_task(async function target_blank() {
   });
 });
 
+add_task(async function target_blank_no_opener() {
+  // Tests that a link with target=_blank and no opener opens a new tab
+  // and closes it, returning the window that we're using for navigation.
+  await BrowserTestUtils.withNewTab({ gBrowser, url: PAGE_URL }, async function(
+    browser
+  ) {
+    let dialogAppeared = promiseHelperAppDialog();
+    let tabOpened = BrowserTestUtils.waitForEvent(
+      gBrowser.tabContainer,
+      "TabOpen"
+    ).then(event => {
+      return [event.target, BrowserTestUtils.waitForTabClosing(event.target)];
+    });
+
+    await BrowserTestUtils.synthesizeMouseAtCenter(
+      "#target_blank_no_opener",
+      {},
+      browser
+    );
+
+    let windowContext = await dialogAppeared;
+    is(windowContext, browser.ownerGlobal, "got the right windowContext");
+    let [tab, closingPromise] = await tabOpened;
+    await closingPromise;
+    is(tab.linkedBrowser, null, "tab was opened and closed");
+  });
+});
+
+add_task(async function open_in_new_tab_no_opener() {
+  // Tests that a link with target=_blank and no opener opens a new tab
+  // and closes it, returning the window that we're using for navigation.
+  await BrowserTestUtils.withNewTab({ gBrowser, url: PAGE_URL }, async function(
+    browser
+  ) {
+    let dialogAppeared = promiseHelperAppDialog();
+    let tabOpened = BrowserTestUtils.waitForEvent(
+      gBrowser.tabContainer,
+      "TabOpen"
+    ).then(event => {
+      return [event.target, BrowserTestUtils.waitForTabClosing(event.target)];
+    });
+
+    await BrowserTestUtils.synthesizeMouseAtCenter(
+      "#open_in_new_tab_no_opener",
+      {},
+      browser
+    );
+
+    let windowContext = await dialogAppeared;
+    is(windowContext, browser.ownerGlobal, "got the right windowContext");
+    let [tab, closingPromise] = await tabOpened;
+    await closingPromise;
+    is(tab.linkedBrowser, null, "tab was opened and closed");
+  });
+});
+
 add_task(async function new_window() {
   // Tests that a link that forces us to open a new window (by specifying a
   // width and a height in window.open) opens a new window for the load,
@@ -122,6 +178,46 @@ add_task(async function new_window() {
     // The window should close on its own. If not, this test will time out.
     await BrowserTestUtils.domWindowClosed(win);
     ok(win.closed, "window was opened and closed");
+
+    is(
+      await fetch(SJS_URL + "?reset").then(r => r.text()),
+      "OK",
+      "Test reseted"
+    );
+  });
+});
+
+add_task(async function new_window_no_opener() {
+  // Tests that a link that forces us to open a new window (by specifying a
+  // width and a height in window.open) opens a new window for the load,
+  // realizes that we need to close that window and returns the *original*
+  // window as the window context.
+  await BrowserTestUtils.withNewTab({ gBrowser, url: PAGE_URL }, async function(
+    browser
+  ) {
+    let dialogAppeared = promiseHelperAppDialog();
+    let windowOpened = BrowserTestUtils.waitForNewWindow();
+
+    await BrowserTestUtils.synthesizeMouseAtCenter(
+      "#new_window_no_opener",
+      {},
+      browser
+    );
+    let win = await windowOpened;
+    // Now allow request to complete:
+    fetch(SJS_URL + "?finish");
+
+    await dialogAppeared;
+
+    // The window should close on its own. If not, this test will time out.
+    await BrowserTestUtils.domWindowClosed(win);
+    ok(win.closed, "window was opened and closed");
+
+    is(
+      await fetch(SJS_URL + "?reset").then(r => r.text()),
+      "OK",
+      "Test reseted"
+    );
   });
 });
 

uriloader/exthandler/tests/mochitest/download.sjs

@@ -20,9 +20,13 @@ function handleRequest(req, res) {
       // Set a variable to allow the request to complete immediately:
       setState("finishReq", "true");
     }
+  } else if (req.queryString.includes('reset')) {
+    res.write("OK");
+    setObjectState("downloadReq", null);
+    setState("finishReq", "false");
   } else {
     res.processAsync();
-    if (getState("finishReq")) {
+    if (getState("finishReq") === "true") {
       actuallyHandleRequest(req, res);
     } else {
       let o = {callback() { actuallyHandleRequest(req, res) }};

uriloader/exthandler/tests/mochitest/download_page.html

@@ -6,14 +6,17 @@
   <meta charset=UTF-8>
   <title>Test page for link clicking</title>
   <script type="text/javascript">
-    function launch_download() {
-      window.open("download.sjs", "_blank", "height=100,width=100");
+    function launch_download(extra) {
+      window.open("download.sjs", "_blank", "height=100,width=100" + extra);
     }
   </script>
 </head>
 <body>
   <a href="download.bin" id="regular_load">regular load</a>
   <a href="download.bin" id="target_blank" target="_blank" rel="opener">target blank</a>
-  <a href="#" onclick="launch_download(); return false" id="new_window">new window</a>
+  <a href="#" onclick="launch_download(''); return false" id="new_window">new window</a>
   <a href="#" onclick="window.open('download_page.html?newwin'); return false" id="open_in_new_tab">click to reopen</a>
+  <a href="download.bin" id="target_blank_no_opener" rel="noopener" target="_blank">target blank (noopener)</a>
+  <a href="#" onclick="window.open('download.bin', '_blank', 'noopener'); return false" id="open_in_new_tab_no_opener">click to reopen (noopener)</a>
+  <a href="#" onclick="launch_download(',noopener'); return false" id="new_window_no_opener">new window (noopener)</a>
 </body>