Bug 1973269 - Pass the URI directly to browser chrome page r=asuth,Gijs

Currently we load the URI using the resulting BrowsingContext, but that causes race with session restore. Passing the URI as an argument to the browser chrome page solves the issue.

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

Author: saschanaz

dom/clients/manager/ClientOpenWindowUtils.cpp

@@ -215,6 +215,16 @@ static Result<Ok, nsresult> OpenNewWindow(
     nsOpenWindowInfo* aOpenWindowInfo) {
   nsresult rv;
 
+  // XXX(krosylight): In an ideal world we should be able to pass the nsIURI
+  // directly. See bug 1485961.
+  nsAutoCString uriToLoad;
+  MOZ_TRY(aArgsValidated.uri->GetSpec(uriToLoad));
+
+  nsCOMPtr<nsISupportsCString> nsUriToLoad =
+      do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID, &rv);
+  MOZ_TRY(rv);
+  MOZ_TRY(nsUriToLoad->SetData(uriToLoad));
+
   nsCOMPtr<nsISupportsPRBool> nsFalse =
       do_CreateInstance(NS_SUPPORTS_PRBOOL_CONTRACTID, &rv);
   MOZ_TRY(rv);
@@ -227,7 +237,7 @@ static Result<Ok, nsresult> OpenNewWindow(
 
   nsCOMPtr<nsIMutableArray> args = do_CreateInstance(NS_ARRAY_CONTRACTID);
   // https://searchfox.org/mozilla-central/rev/02d33f4bf984f65bd394bfd2d19d66569ae2cfe1/browser/base/content/browser-init.js#725-735
-  args->AppendElement(nullptr);                   // 0: uriToLoad
+  args->AppendElement(nsUriToLoad);               // 0: uriToLoad
   args->AppendElement(nullptr);                   // 1: extraOptions
   args->AppendElement(nullptr);                   // 2: referrerInfo
   args->AppendElement(nullptr);                   // 3: postData
@@ -310,7 +320,7 @@ void OpenWindow(const ClientOpenWindowArgsParsed& aArgsValidated,
 
 void WaitForLoad(const ClientOpenWindowArgsParsed& aArgsValidated,
                  BrowsingContext* aBrowsingContext,
-                 ClientOpPromise::Private* aPromise) {
+                 ClientOpPromise::Private* aPromise, bool aShouldLoadURI) {
   MOZ_DIAGNOSTIC_ASSERT(aBrowsingContext);
 
   RefPtr<ClientOpPromise::Private> promise = aPromise;
@@ -328,7 +338,7 @@ void WaitForLoad(const ClientOpenWindowArgsParsed& aArgsValidated,
     return;
   }
 
-  // Add a progress listener before we start the load of the service worker URI
+  // Add a progress listener before we start the load of the requested URI
   RefPtr<WebProgressListener> listener = new WebProgressListener(
       aBrowsingContext, aArgsValidated.baseURI, do_AddRef(promise));
 
@@ -342,24 +352,27 @@ void WaitForLoad(const ClientOpenWindowArgsParsed& aArgsValidated,
     return;
   }
 
-  // Load the service worker URI
-  RefPtr<nsDocShellLoadState> loadState =
-      new nsDocShellLoadState(aArgsValidated.uri);
-  loadState->SetTriggeringPrincipal(aArgsValidated.principal);
-  loadState->SetFirstParty(true);
-  loadState->SetLoadFlags(
-      nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL);
-  loadState->SetTriggeringRemoteType(
-      aArgsValidated.originContent
-          ? aArgsValidated.originContent->GetRemoteType()
-          : NOT_REMOTE_TYPE);
-
-  rv = aBrowsingContext->LoadURI(loadState, true);
-  if (NS_FAILED(rv)) {
-    CopyableErrorResult result;
-    result.ThrowInvalidStateError("Unable to start the load of the actual URI");
-    promise->Reject(result, __func__);
-    return;
+  if (aShouldLoadURI) {
+    // Load the requested URI
+    RefPtr<nsDocShellLoadState> loadState =
+        new nsDocShellLoadState(aArgsValidated.uri);
+    loadState->SetTriggeringPrincipal(aArgsValidated.principal);
+    loadState->SetFirstParty(true);
+    loadState->SetLoadFlags(
+        nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL);
+    loadState->SetTriggeringRemoteType(
+        aArgsValidated.originContent
+            ? aArgsValidated.originContent->GetRemoteType()
+            : NOT_REMOTE_TYPE);
+
+    rv = aBrowsingContext->LoadURI(loadState, true);
+    if (NS_FAILED(rv)) {
+      CopyableErrorResult result;
+      result.ThrowInvalidStateError(
+          "Unable to start the load of the actual URI");
+      promise->Reject(result, __func__);
+      return;
+    }
   }
 
   // Hold the listener alive until the promise settles.
@@ -371,8 +384,7 @@ void WaitForLoad(const ClientOpenWindowArgsParsed& aArgsValidated,
 
 #ifdef MOZ_GECKOVIEW
 void GeckoViewOpenWindow(const ClientOpenWindowArgsParsed& aArgsValidated,
-                         nsOpenWindowInfo* aOpenInfo, BrowsingContext** aBC,
-                         ErrorResult& aRv) {
+                         nsOpenWindowInfo* aOpenInfo, ErrorResult& aRv) {
   MOZ_ASSERT(aOpenInfo);
 
   // passes the request to open a new window to GeckoView. Allowing the
@@ -487,11 +499,13 @@ RefPtr<ClientOpPromise> ClientOpenWindow(
 
   RefPtr<BrowsingContext> bc;
   IgnoredErrorResult errResult;
+  bool shouldLoadURI = true;
 #ifdef MOZ_GECKOVIEW
   // GeckoView has a delegation for service worker window.
-  GeckoViewOpenWindow(argsValidated, openInfo, getter_AddRefs(bc), errResult);
+  GeckoViewOpenWindow(argsValidated, openInfo, errResult);
 #else
   OpenWindow(argsValidated, openInfo, getter_AddRefs(bc), errResult);
+  shouldLoadURI = !!bc;
 #endif
   if (NS_WARN_IF(errResult.Failed())) {
     promise->Reject(errResult, __func__);
@@ -500,8 +514,9 @@ RefPtr<ClientOpPromise> ClientOpenWindow(
 
   browsingContextReadyPromise->Then(
       GetCurrentSerialEventTarget(), __func__,
-      [argsValidated, promise](const RefPtr<BrowsingContext>& aBC) {
-        WaitForLoad(argsValidated, aBC, promise);
+      [argsValidated, promise,
+       shouldLoadURI](const RefPtr<BrowsingContext>& aBC) {
+        WaitForLoad(argsValidated, aBC, promise, shouldLoadURI);
       },
       [promise]() {
         // in case of failure, reject the original promise

dom/notification/test/browser/browser_openwindow_without_window.js

@@ -46,12 +46,31 @@ add_setup(() => {
   BrowserTestUtils.concealWindow(window, { signal });
 });
 
-for (let permanentPbm of [false, true]) {
+let configs = [
+  {
+    permanentPbm: false,
+    browserStartup: 1,
+  },
+  {
+    permanentPbm: true,
+    browserStartup: 1,
+  },
+  {
+    permanentPbm: false,
+    browserStartup: 3,
+  },
+];
+
+for (let { permanentPbm, browserStartup } of configs) {
   add_task(async function () {
     info(`Test with PBM: ${permanentPbm}`);
+    info(`Test with startup behavior: ${browserStartup}`);
 
     await SpecialPowers.pushPrefEnv({
-      set: [["browser.privatebrowsing.autostart", permanentPbm]],
+      set: [
+        ["browser.privatebrowsing.autostart", permanentPbm],
+        ["browser.startup.page", browserStartup],
+      ],
     });
 
     let win = await BrowserTestUtils.openNewBrowserWindow();
@@ -85,8 +104,15 @@ for (let permanentPbm of [false, true]) {
 
     registerCleanupFunction(() => SpecialPowers.removeAllServiceWorkerData());
 
-    // Now close the current window
-    await BrowserTestUtils.closeWindow(win);
+    // Now close the current window via BrowserCommands
+    // (simple .close() will not trigger session store as it does not notify
+    // browser-lastwindow-close-granted)
+    let closedPromise = BrowserTestUtils.windowClosed(win);
+    win.BrowserCommands.tryToCloseWindow();
+    await closedPromise;
+
+    // Let session startup read the changed pref again
+    SessionStartup.resetForTest();
 
     let newWinPromise = BrowserTestUtils.waitForNewWindow({
       url: OPEN_URI,