Bug 1561015: Part 2 - Return BrowsingContext from openWindow2. r=bzbarsky

There are some unfortunate aspects of openWindow() and openWindow2() that make
this difficult. Namely, they sometimes return top-level chrome windows, and
sometimes a single content window from the top-level chrome window that they
open, depending on how they're called.

There really isn't any reason to return a BrowsingContext rather than a chrome
window in the former case, but there also really isn't a way to make the API
polymorphic in a way that would allow handling the two cases differently. So
at some point, the two cases should ideally be split into separate APIs rather
than separate special cases of a single API.

In the mean time, I've left openWindow() returning local mozIDOMWindowProxy
objects, since it isn't used by the DOM window.open() or openDialog() APIs,
and only updated openWindow2(). As a follow-up, we should remove both
openWindow() and openWindow2(), and replace them with openChromeWindow() and
openContentWindow() (or similar) methods which make it immediately clear what
type of window is being returned.

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

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

dom/base/nsGlobalWindowOuter.cpp

@@ -7275,7 +7275,6 @@ nsresult nsGlobalWindowOuter::OpenInternal(
     // dialog is open.
     AutoPopupStatePusher popupStatePusher(PopupBlocker::openAbused, true);
 
-    nsCOMPtr<mozIDOMWindowProxy> win;
     if (!aCalledNoScript) {
       // We asserted at the top of this function that aNavigate is true for
       // !aCalledNoScript.
@@ -7283,7 +7282,7 @@ nsresult nsGlobalWindowOuter::OpenInternal(
           this, url.IsVoid() ? nullptr : url.get(), name_ptr, options_ptr,
           /* aCalledFromScript = */ true, aDialog, aNavigate, argv,
           isPopupSpamWindow, forceNoOpener, forceNoReferrer, aLoadState,
-          getter_AddRefs(win));
+          getter_AddRefs(domReturn));
     } else {
       // Force a system caller here so that the window watcher won't screw us
       // up.  We do NOT want this case looking at the JS context on the stack
@@ -7303,10 +7302,7 @@ nsresult nsGlobalWindowOuter::OpenInternal(
           this, url.IsVoid() ? nullptr : url.get(), name_ptr, options_ptr,
           /* aCalledFromScript = */ false, aDialog, aNavigate, aExtraArgument,
           isPopupSpamWindow, forceNoOpener, forceNoReferrer, aLoadState,
-          getter_AddRefs(win));
-    }
-    if (win) {
-      domReturn = nsPIDOMWindowOuter::From(win)->GetBrowsingContext();
+          getter_AddRefs(domReturn));
     }
   }
 

dom/clients/manager/ClientOpenWindowUtils.cpp

@@ -9,6 +9,7 @@
 #include "ClientInfo.h"
 #include "ClientState.h"
 #include "mozilla/SystemGroup.h"
+#include "mozilla/ResultExtensions.h"
 #include "nsContentUtils.h"
 #include "nsFocusManager.h"
 #include "nsIBrowserDOMWindow.h"
@@ -211,8 +212,7 @@ nsresult OpenWindow(const ClientOpenWindowArgs& aArgs, BrowsingContext** aBC) {
       return rv;
     }
 
-    nsCOMPtr<mozIDOMWindowProxy> newWindow;
-    rv = pwwatch->OpenWindow2(
+    MOZ_TRY(pwwatch->OpenWindow2(
         nullptr, spec.get(), nullptr, nullptr, false, false, true, nullptr,
         // Not a spammy popup; we got permission, we swear!
         /* aIsPopupSpam = */ false,
@@ -221,13 +221,7 @@ nsresult OpenWindow(const ClientOpenWindowArgs& aArgs, BrowsingContext** aBC) {
         // window.
         /* aForceNoOpener = */ false,
         /* aForceNoReferrer = */ false,
-        /* aLoadInfp = */ nullptr, getter_AddRefs(newWindow));
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-    RefPtr<BrowsingContext> bc(
-        nsPIDOMWindowOuter::From(newWindow)->GetBrowsingContext());
-    bc.forget(aBC);
+        /* aLoadInfp = */ nullptr, aBC));
     MOZ_DIAGNOSTIC_ASSERT(*aBC);
     return NS_OK;
   }

dom/ipc/ContentChild.cpp

@@ -1104,10 +1104,7 @@ nsresult ContentChild::ProvideWindowCommon(
       newChild->RecvLoadURL(urlToLoad, showInfo);
     }
 
-    nsCOMPtr<nsPIDOMWindowOuter> win =
-        do_GetInterface(newChild->WebNavigation());
-    RefPtr<BrowsingContext> bc(win->GetBrowsingContext());
-    bc.forget(aReturn);
+    browsingContext.forget(aReturn);
   };
 
   // NOTE: Capturing by reference here is safe, as this function won't return

toolkit/components/windowwatcher/nsPIWindowWatcher.idl

@@ -14,6 +14,7 @@
 class nsDocShellLoadState;
 %}
 
+webidl BrowsingContext;
 interface mozIDOMWindowProxy;
 interface nsIDOMWindow;
 interface nsISimpleEnumerator;
@@ -81,16 +82,16 @@ interface nsPIWindowWatcher : nsISupports
             (which is determined based on the JS stack and the value of
             aParent).  This is not guaranteed, however.
   */
-  mozIDOMWindowProxy openWindow2(in mozIDOMWindowProxy aParent, in string aUrl,
-                                 in string aName, in string aFeatures,
-                                 in boolean aCalledFromScript,
-                                 in boolean aDialog,
-                                 in boolean aNavigate,
-                                 in nsISupports aArgs,
-                                 in boolean aIsPopupSpam,
-                                 in boolean aForceNoOpener,
-                                 in boolean aForceNoReferrer,
-                                 in nsDocShellLoadStatePtr aLoadState);
+  BrowsingContext openWindow2(in mozIDOMWindowProxy aParent, in string aUrl,
+                              in string aName, in string aFeatures,
+                              in boolean aCalledFromScript,
+                              in boolean aDialog,
+                              in boolean aNavigate,
+                              in nsISupports aArgs,
+                              in boolean aIsPopupSpam,
+                              in boolean aForceNoOpener,
+                              in boolean aForceNoReferrer,
+                              in nsDocShellLoadStatePtr aLoadState);
 
   /**
    * Opens a new window so that the window that aOpeningTab belongs to

toolkit/components/windowwatcher/nsWindowWatcher.cpp

@@ -61,6 +61,7 @@
 #include "mozilla/CheckedInt.h"
 #include "mozilla/NullPrincipal.h"
 #include "mozilla/Preferences.h"
+#include "mozilla/ResultExtensions.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/Storage.h"
 #include "mozilla/dom/ScriptSettings.h"
@@ -287,13 +288,17 @@ nsWindowWatcher::OpenWindow(mozIDOMWindowProxy* aParent, const char* aUrl,
   }
   bool dialog = (argc != 0);
 
-  return OpenWindowInternal(aParent, aUrl, aName, aFeatures,
-                            /* calledFromJS = */ false, dialog,
-                            /* navigate = */ true, argv,
-                            /* aIsPopupSpam = */ false,
-                            /* aForceNoOpener = */ false,
-                            /* aForceNoReferrer = */ false,
-                            /* aLoadState = */ nullptr, aResult);
+  RefPtr<BrowsingContext> bc;
+  MOZ_TRY(OpenWindowInternal(aParent, aUrl, aName, aFeatures,
+                             /* calledFromJS = */ false, dialog,
+                             /* navigate = */ true, argv,
+                             /* aIsPopupSpam = */ false,
+                             /* aForceNoOpener = */ false,
+                             /* aForceNoReferrer = */ false,
+                             /* aLoadState = */ nullptr, getter_AddRefs(bc)));
+  nsCOMPtr<mozIDOMWindowProxy> win(bc->GetDOMWindow());
+  win.forget(aResult);
+  return NS_OK;
 }
 
 struct SizeSpec {
@@ -350,7 +355,7 @@ nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy* aParent, const char* aUrl,
                              bool aIsPopupSpam, bool aForceNoOpener,
                              bool aForceNoReferrer,
                              nsDocShellLoadState* aLoadState,
-                             mozIDOMWindowProxy** aResult) {
+                             BrowsingContext** aResult) {
   nsCOMPtr<nsIArray> argv = ConvertArgsToArray(aArguments);
 
   uint32_t argc = 0;
@@ -577,7 +582,7 @@ nsresult nsWindowWatcher::OpenWindowInternal(
     const char* aFeatures, bool aCalledFromJS, bool aDialog, bool aNavigate,
     nsIArray* aArgv, bool aIsPopupSpam, bool aForceNoOpener,
     bool aForceNoReferrer, nsDocShellLoadState* aLoadState,
-    mozIDOMWindowProxy** aResult) {
+    BrowsingContext** aResult) {
   MOZ_ASSERT_IF(aForceNoReferrer, aForceNoOpener);
 
   nsresult rv = NS_OK;
@@ -926,12 +931,17 @@ nsresult nsWindowWatcher::OpenWindowInternal(
     newDocShell->SetSandboxFlags(activeDocsSandboxFlags);
   }
 
+  nsCOMPtr<mozIDOMWindowProxy> win;
   rv = ReadyOpenedDocShellItem(newDocShellItem, parentWindow, windowIsNew,
-                               aForceNoOpener, aResult);
+                               aForceNoOpener, getter_AddRefs(win));
   if (NS_FAILED(rv)) {
     return rv;
   }
 
+  RefPtr<BrowsingContext> bc(
+      nsPIDOMWindowOuter::From(win)->GetBrowsingContext());
+  bc.forget(aResult);
+
   if (isNewToplevelWindow) {
     nsCOMPtr<nsIDocShellTreeOwner> newTreeOwner;
     newDocShellItem->GetTreeOwner(getter_AddRefs(newTreeOwner));
@@ -940,7 +950,7 @@ nsresult nsWindowWatcher::OpenWindowInternal(
 
   if ((aDialog || windowIsModalContentDialog) && aArgv) {
     // Set the args on the new window.
-    nsCOMPtr<nsPIDOMWindowOuter> piwin(do_QueryInterface(*aResult));
+    nsCOMPtr<nsPIDOMWindowOuter> piwin(do_QueryInterface(win));
     NS_ENSURE_TRUE(piwin, NS_ERROR_UNEXPECTED);
 
     rv = piwin->SetArguments(aArgv);
@@ -1009,7 +1019,7 @@ nsresult nsWindowWatcher::OpenWindowInternal(
     // the JS stack, just use the principal of our parent window.  In those
     // cases we do _not_ set the parent window principal as the owner of the
     // load--since we really don't know who the owner is, just leave it null.
-    nsCOMPtr<nsPIDOMWindowOuter> newWindow = do_QueryInterface(*aResult);
+    nsCOMPtr<nsPIDOMWindowOuter> newWindow = do_QueryInterface(win);
     NS_ASSERTION(newWindow == newDocShell->GetWindow(), "Different windows??");
 
     // The principal of the initial about:blank document gets set up in
@@ -1114,7 +1124,7 @@ nsresult nsWindowWatcher::OpenWindowInternal(
     nsCOMPtr<nsIObserverService> obsSvc =
         mozilla::services::GetObserverService();
     if (obsSvc) {
-      obsSvc->NotifyObservers(*aResult, "toplevel-window-ready", nullptr);
+      obsSvc->NotifyObservers(win, "toplevel-window-ready", nullptr);
     }
   }
 

toolkit/components/windowwatcher/nsWindowWatcher.h

@@ -86,7 +86,7 @@ class nsWindowWatcher : public nsIWindowWatcher,
                               nsIArray* aArgv, bool aIsPopupSpam,
                               bool aForceNoOpener, bool aForceNoReferrer,
                               nsDocShellLoadState* aLoadState,
-                              mozIDOMWindowProxy** aResult);
+                              mozilla::dom::BrowsingContext** aResult);
 
   static nsresult URIfromURL(const char* aURL, mozIDOMWindowProxy* aParent,
                              nsIURI** aURI);