Bug 1914594 - Move docshell.newWindowTarget to be on LoadInfo a=diannaS

Previously, this was passed around using the nsHashPropertyBag base
class on HTTP channels. In bug 1907269, this base was added to
nsExtProtocolChannel to support reading this property for external
protocols.

This unfortunately ended up not working when a HTTP channel redirects to
an external protocol, as the hash property propagation logic in
nsHttpChannel happens after the check that the target channel is a HTTP
channel, so the property was lost when redirecting to an external
protocol.

Rather than refactoring HTTP redirect logic, this patch simplifies by
moving the property onto the LoadInfo where it will be trivially
preserved across redirects, and available in the places which need it.

Because the nsHashPropertyBag base is no longer required for
nsExtProtocolChannel, it is also removed.

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

docshell/base/nsDocShell.cpp

@@ -9746,6 +9746,8 @@ nsIPrincipal* nsDocShell::GetInheritedPrincipal(
       INTERNAL_LOAD_FLAGS_FORCE_ALLOW_DATA_URI));
   aLoadInfo->SetOriginalFrameSrcLoad(
       aLoadState->HasInternalLoadFlags(INTERNAL_LOAD_FLAGS_ORIGINAL_FRAME_SRC));
+  aLoadInfo->SetIsNewWindowTarget(
+      aLoadState->HasInternalLoadFlags(INTERNAL_LOAD_FLAGS_FIRST_LOAD));
 
   bool inheritAttrs = false;
   if (aLoadState->PrincipalToInherit()) {
@@ -9886,10 +9888,6 @@ nsIPrincipal* nsDocShell::GetInheritedPrincipal(
     // save true referrer for those who need it (e.g. xpinstall whitelisting)
     // Currently only http and ftp channels support this.
     props->SetPropertyAsInterface(u"docshell.internalReferrer"_ns, referrer);
-
-    if (aLoadState->HasInternalLoadFlags(INTERNAL_LOAD_FLAGS_FIRST_LOAD)) {
-      props->SetPropertyAsBool(u"docshell.newWindowTarget"_ns, true);
-    }
   }
 
   nsCOMPtr<nsICacheInfoChannel> cacheChannel(do_QueryInterface(channel));

dom/ipc/ContentParent.cpp

@@ -4809,8 +4809,7 @@ ContentParent::AllocPExternalHelperAppParent(
     const uint32_t& aContentDispositionHint,
     const nsAString& aContentDispositionFilename, const bool& aForceSave,
     const int64_t& aContentLength, const bool& aWasFileChannel,
-    nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext,
-    const bool& aShouldCloseWindow) {
+    nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext) {
   RefPtr<ExternalHelperAppParent> parent = new ExternalHelperAppParent(
       uri, aContentLength, aWasFileChannel, aContentDisposition,
       aContentDispositionHint, aContentDispositionFilename);
@@ -4824,12 +4823,10 @@ mozilla::ipc::IPCResult ContentParent::RecvPExternalHelperAppConstructor(
     const uint32_t& aContentDispositionHint,
     const nsAString& aContentDispositionFilename, const bool& aForceSave,
     const int64_t& aContentLength, const bool& aWasFileChannel,
-    nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext,
-    const bool& aShouldCloseWindow) {
+    nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext) {
   BrowsingContext* context = aContext.IsDiscarded() ? nullptr : aContext.get();
   if (!static_cast<ExternalHelperAppParent*>(actor)->Init(
-          loadInfoArgs, aMimeContentType, aForceSave, aReferrer, context,
-          aShouldCloseWindow)) {
+          loadInfoArgs, aMimeContentType, aForceSave, aReferrer, context)) {
     return IPC_FAIL(this, "Init failed.");
   }
   return IPC_OK();

dom/ipc/ContentParent.h

@@ -929,8 +929,7 @@ class ContentParent final : public PContentParent,
       const uint32_t& aContentDispositionHint,
       const nsAString& aContentDispositionFilename, const bool& aForceSave,
       const int64_t& aContentLength, const bool& aWasFileChannel,
-      nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext,
-      const bool& aShouldCloseWindow);
+      nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext);
 
   mozilla::ipc::IPCResult RecvPExternalHelperAppConstructor(
       PExternalHelperAppParent* actor, nsIURI* uri,
@@ -939,8 +938,8 @@ class ContentParent final : public PContentParent,
       const uint32_t& aContentDispositionHint,
       const nsAString& aContentDispositionFilename, const bool& aForceSave,
       const int64_t& aContentLength, const bool& aWasFileChannel,
-      nsIURI* aReferrer, const MaybeDiscarded<BrowsingContext>& aContext,
-      const bool& aShouldCloseWindow) override;
+      nsIURI* aReferrer,
+      const MaybeDiscarded<BrowsingContext>& aContext) override;
 
   already_AddRefed<PHandlerServiceParent> AllocPHandlerServiceParent();
 

dom/ipc/PContent.ipdl

@@ -1173,8 +1173,6 @@ parent:
     //
     // Pass true for aForceSave to always save this content to disk, regardless of
     // nsIMIMEInfo and other such influences.
-    // Pass true for aShouldCloseWindow to specify that aContext was opened specifically
-    // for this load, and should be closed once we've handled it.
     async PExternalHelperApp(nullable nsIURI uri,
                              LoadInfoArgs loadInfoArgs,
                              nsCString aMimeContentType,
@@ -1185,8 +1183,7 @@ parent:
                              int64_t aContentLength,
                              bool aWasFileChannel,
                              nullable nsIURI aReferrer,
-                             MaybeDiscardedBrowsingContext aContext,
-                             bool aShouldCloseWindow);
+                             MaybeDiscardedBrowsingContext aContext);
 
     async PHandlerService();
 

ipc/glue/BackgroundUtils.cpp

@@ -596,7 +596,7 @@ nsresult LoadInfoToLoadInfoArgs(nsILoadInfo* aLoadInfo,
       aLoadInfo->GetStoragePermission(), overriddenFingerprintingSettingsArg,
       aLoadInfo->GetIsMetaRefresh(), aLoadInfo->GetLoadingEmbedderPolicy(),
       aLoadInfo->GetIsOriginTrialCoepCredentiallessEnabledForTopLevel(),
-      unstrippedURI, interceptionInfoArg);
+      unstrippedURI, interceptionInfoArg, aLoadInfo->GetIsNewWindowTarget());
 
   return NS_OK;
 }
@@ -885,7 +885,7 @@ nsresult LoadInfoArgsToLoadInfo(const LoadInfoArgs& loadInfoArgs,
       loadInfoArgs.originTrialCoepCredentiallessEnabledForTopLevel(),
       loadInfoArgs.unstrippedURI(), interceptionInfo,
       loadInfoArgs.hasInjectedCookieForCookieBannerHandling(),
-      loadInfoArgs.wasSchemelessInput());
+      loadInfoArgs.wasSchemelessInput(), loadInfoArgs.isNewWindowTarget());
 
   if (loadInfoArgs.isFromProcessingFrameAttributes()) {
     loadInfo->SetIsFromProcessingFrameAttributes();

netwerk/base/LoadInfo.cpp

@@ -691,7 +691,8 @@ LoadInfo::LoadInfo(const LoadInfo& rhs)
       mInterceptionInfo(rhs.mInterceptionInfo),
       mHasInjectedCookieForCookieBannerHandling(
           rhs.mHasInjectedCookieForCookieBannerHandling),
-      mWasSchemelessInput(rhs.mWasSchemelessInput) {
+      mWasSchemelessInput(rhs.mWasSchemelessInput),
+      mIsNewWindowTarget(rhs.mIsNewWindowTarget) {
 }
 
 LoadInfo::LoadInfo(
@@ -738,7 +739,8 @@ LoadInfo::LoadInfo(
     nsILoadInfo::CrossOriginEmbedderPolicy aLoadingEmbedderPolicy,
     bool aIsOriginTrialCoepCredentiallessEnabledForTopLevel,
     nsIURI* aUnstrippedURI, nsIInterceptionInfo* aInterceptionInfo,
-    bool aHasInjectedCookieForCookieBannerHandling, bool aWasSchemelessInput)
+    bool aHasInjectedCookieForCookieBannerHandling, bool aWasSchemelessInput,
+    bool aIsNewWindowTarget)
     : mLoadingPrincipal(aLoadingPrincipal),
       mTriggeringPrincipal(aTriggeringPrincipal),
       mPrincipalToInherit(aPrincipalToInherit),
@@ -816,7 +818,8 @@ LoadInfo::LoadInfo(
       mInterceptionInfo(aInterceptionInfo),
       mHasInjectedCookieForCookieBannerHandling(
           aHasInjectedCookieForCookieBannerHandling),
-      mWasSchemelessInput(aWasSchemelessInput) {
+      mWasSchemelessInput(aWasSchemelessInput),
+      mIsNewWindowTarget(aIsNewWindowTarget) {
   // Only top level TYPE_DOCUMENT loads can have a null loadingPrincipal
   MOZ_ASSERT(mLoadingPrincipal ||
              aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT);
@@ -2416,4 +2419,16 @@ LoadInfo::SetWasSchemelessInput(bool aWasSchemelessInput) {
   return NS_OK;
 }
 
+NS_IMETHODIMP
+LoadInfo::GetIsNewWindowTarget(bool* aIsNewWindowTarget) {
+  *aIsNewWindowTarget = mIsNewWindowTarget;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+LoadInfo::SetIsNewWindowTarget(bool aIsNewWindowTarget) {
+  mIsNewWindowTarget = aIsNewWindowTarget;
+  return NS_OK;
+}
+
 }  // namespace mozilla::net

netwerk/base/LoadInfo.h

@@ -258,7 +258,9 @@ class LoadInfo final : public nsILoadInfo {
       nsILoadInfo::CrossOriginEmbedderPolicy aLoadingEmbedderPolicy,
       bool aIsOriginTrialCoepCredentiallessEnabledForTopLevel,
       nsIURI* aUnstrippedURI, nsIInterceptionInfo* aInterceptionInfo,
-      bool aHasInjectedCookieForCookieBannerHandling, bool aWasSchemelessInput);
+      bool aHasInjectedCookieForCookieBannerHandling, bool aWasSchemelessInput,
+      bool aIsNewWindowTarget);
+
   LoadInfo(const LoadInfo& rhs);
 
   NS_IMETHOD GetRedirects(JSContext* aCx,
@@ -408,6 +410,8 @@ class LoadInfo final : public nsILoadInfo {
 
   bool mHasInjectedCookieForCookieBannerHandling = false;
   bool mWasSchemelessInput = false;
+
+  bool mIsNewWindowTarget = false;
 };
 
 // This is exposed solely for testing purposes and should not be used outside of

netwerk/base/TRRLoadInfo.cpp

@@ -870,5 +870,15 @@ TRRLoadInfo::SetWasSchemelessInput(bool aWasSchemelessInput) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
+NS_IMETHODIMP
+TRRLoadInfo::GetIsNewWindowTarget(bool* aIsNewWindowTarget) {
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
+NS_IMETHODIMP
+TRRLoadInfo::SetIsNewWindowTarget(bool aIsNewWindowTarget) {
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
 }  // namespace net
 }  // namespace mozilla

netwerk/base/nsILoadInfo.idl

@@ -1544,4 +1544,10 @@ interface nsILoadInfo : nsISupports
    * Whether the load has gone through the URL bar, where the fixup had to add * the protocol scheme.
    */
    [infallible] attribute boolean wasSchemelessInput;
+
+  /**
+   * Is this the first load in a new pop-up window/tab?
+   */
+  [infallible] attribute boolean isNewWindowTarget;
+
 };

netwerk/ipc/DocumentLoadListener.cpp

@@ -1646,13 +1646,8 @@ void DocumentLoadListener::SerializeRedirectData(
 }
 
 static bool IsFirstLoadInWindow(nsIChannel* aChannel) {
-  if (nsCOMPtr<nsIPropertyBag2> props = do_QueryInterface(aChannel)) {
-    bool tmp = false;
-    nsresult rv =
-        props->GetPropertyAsBool(u"docshell.newWindowTarget"_ns, &tmp);
-    return NS_SUCCEEDED(rv) && tmp;
-  }
-  return false;
+  nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
+  return loadInfo->GetIsNewWindowTarget();
 }
 
 // Get where the document loaded by this nsIChannel should be rendered. This

netwerk/ipc/NeckoChannelParams.ipdlh

@@ -192,6 +192,7 @@ struct LoadInfoArgs
   bool                        originTrialCoepCredentiallessEnabledForTopLevel;
   nullable nsIURI             unstrippedURI;
   InterceptionInfoArg?        interceptionInfo;
+  bool                        isNewWindowTarget;
 };
 
 /**

uriloader/exthandler/ExternalHelperAppParent.cpp

@@ -62,8 +62,7 @@ ExternalHelperAppParent::ExternalHelperAppParent(
 bool ExternalHelperAppParent::Init(
     const mozilla::net::LoadInfoArgs& aLoadInfoArgs,
     const nsACString& aMimeContentType, const bool& aForceSave,
-    nsIURI* aReferrer, BrowsingContext* aContext,
-    const bool& aShouldCloseWindow) {
+    nsIURI* aReferrer, BrowsingContext* aContext) {
   nsresult rv = mozilla::ipc::LoadInfoArgsToLoadInfo(
       aLoadInfoArgs, ContentParent::Cast(Manager())->GetRemoteType(),
       getter_AddRefs(mLoadInfo));
@@ -95,18 +94,7 @@ bool ExternalHelperAppParent::Init(
 
   helperAppService->CreateListener(aMimeContentType, this, aContext, aForceSave,
                                    nullptr, getter_AddRefs(mListener));
-  if (!mListener) {
-    return false;
-  }
-
-  if (aShouldCloseWindow) {
-    RefPtr<nsExternalAppHandler> handler = do_QueryObject(mListener);
-    if (handler) {
-      handler->SetShouldCloseWindow();
-    }
-  }
-
-  return true;
+  return mListener != nullptr;
 }
 
 void ExternalHelperAppParent::ActorDestroy(ActorDestroyReason why) {

uriloader/exthandler/ExternalHelperAppParent.h

@@ -87,8 +87,7 @@ class ExternalHelperAppParent
                           const nsAString& aContentDispositionFilename);
   bool Init(const mozilla::net::LoadInfoArgs& aLoadInfoArgs,
             const nsACString& aMimeContentType, const bool& aForceSave,
-            nsIURI* aReferrer, BrowsingContext* aContext,
-            const bool& aShouldCloseWindow);
+            nsIURI* aReferrer, BrowsingContext* aContext);
 
  protected:
   virtual ~ExternalHelperAppParent();

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -678,14 +678,6 @@ nsresult nsExternalHelperAppService::DoContentContentProcessHelper(
   mozilla::net::LoadInfoArgs loadInfoArgs;
   MOZ_ALWAYS_SUCCEEDS(LoadInfoToLoadInfoArgs(loadInfo, &loadInfoArgs));
 
-  nsCOMPtr<nsIPropertyBag2> props(do_QueryInterface(aChannel));
-  // Determine whether a new window was opened specifically for this request
-  bool shouldCloseWindow = false;
-  if (props) {
-    props->GetPropertyAsBool(u"docshell.newWindowTarget"_ns,
-                             &shouldCloseWindow);
-  }
-
   // Now we build a protocol for forwarding our data to the parent.  The
   // protocol will act as a listener on the child-side and create a "real"
   // helperAppService listener on the parent-side, via another call to
@@ -694,7 +686,7 @@ nsresult nsExternalHelperAppService::DoContentContentProcessHelper(
   MOZ_ALWAYS_TRUE(child->SendPExternalHelperAppConstructor(
       childListener, uri, loadInfoArgs, nsCString(aMimeContentType), disp,
       contentDisposition, fileName, aForceSave, contentLength, wasFileChannel,
-      referrer, aContentContext, shouldCloseWindow));
+      referrer, aContentContext));
 
   NS_ADDREF(*aStreamListener = childListener);
 
@@ -1287,7 +1279,6 @@ nsExternalAppHandler::nsExternalAppHandler(
       mCanceled(false),
       mStopRequestIssued(false),
       mIsFileChannel(false),
-      mShouldCloseWindow(false),
       mHandleInternally(false),
       mDialogShowing(false),
       mReason(aReason),
@@ -1603,15 +1594,12 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
 
   if (mBrowsingContext) {
     mMaybeCloseWindowHelper = new MaybeCloseWindowHelper(mBrowsingContext);
-    mMaybeCloseWindowHelper->SetShouldCloseWindow(mShouldCloseWindow);
-    nsCOMPtr<nsIPropertyBag2> props(do_QueryInterface(request, &rv));
+
     // Determine whether a new window was opened specifically for this request
-    if (props) {
-      bool tmp = false;
-      if (NS_SUCCEEDED(
-              props->GetPropertyAsBool(u"docshell.newWindowTarget"_ns, &tmp))) {
-        mMaybeCloseWindowHelper->SetShouldCloseWindow(tmp);
-      }
+    if (aChannel) {
+      nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
+      mMaybeCloseWindowHelper->SetShouldCloseWindow(
+          loadInfo->GetIsNewWindowTarget());
     }
   }
 

uriloader/exthandler/nsExternalHelperAppService.h

@@ -315,8 +315,6 @@ class nsExternalAppHandler final : public nsIStreamListener,
    */
   void MaybeApplyDecodingForExtension(nsIRequest* request);
 
-  void SetShouldCloseWindow() { mShouldCloseWindow = true; }
-
  protected:
   bool IsDownloadSpam(nsIChannel* aChannel);
 
@@ -382,12 +380,6 @@ class nsExternalAppHandler final : public nsIStreamListener,
 
   bool mIsFileChannel;
 
-  /**
-   * True if the ExternalHelperAppChild told us that we should close the window
-   * if we handle the content as a download.
-   */
-  bool mShouldCloseWindow;
-
   /**
    * True if the file should be handled internally.
    */

uriloader/exthandler/nsExternalProtocolHandler.cpp

@@ -21,7 +21,6 @@
 #include "nsNetUtil.h"
 #include "nsContentSecurityManager.h"
 #include "nsExternalHelperAppService.h"
-#include "nsHashPropertyBag.h"
 
 // used to dispatch urls to default protocol handlers
 #include "nsCExternalHandlerService.h"
@@ -36,12 +35,11 @@ class nsILoadInfo;
 // OpenInputStream to calls in the OS for loading the url.
 ////////////////////////////////////////////////////////////////////////
 
-class nsExtProtocolChannel : public nsHashPropertyBag,
-                             public nsIChannel,
+class nsExtProtocolChannel : public nsIChannel,
                              public nsIChildChannel,
                              public nsIParentChannel {
  public:
-  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSICHANNEL
   NS_DECL_NSIREQUESTOBSERVER
   NS_DECL_NSISTREAMLISTENER
@@ -75,17 +73,18 @@ class nsExtProtocolChannel : public nsHashPropertyBag,
   nsCOMPtr<nsIStreamListener> mListener;
 };
 
-NS_IMPL_ADDREF_INHERITED(nsExtProtocolChannel, nsHashPropertyBag)
-NS_IMPL_RELEASE_INHERITED(nsExtProtocolChannel, nsHashPropertyBag)
+NS_IMPL_ADDREF(nsExtProtocolChannel)
+NS_IMPL_RELEASE(nsExtProtocolChannel)
 
 NS_INTERFACE_MAP_BEGIN(nsExtProtocolChannel)
+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIChannel)
   NS_INTERFACE_MAP_ENTRY(nsIChannel)
   NS_INTERFACE_MAP_ENTRY(nsIRequest)
   NS_INTERFACE_MAP_ENTRY(nsIChildChannel)
   NS_INTERFACE_MAP_ENTRY(nsIParentChannel)
   NS_INTERFACE_MAP_ENTRY(nsIStreamListener)
   NS_INTERFACE_MAP_ENTRY(nsIRequestObserver)
-NS_INTERFACE_MAP_END_INHERITING(nsHashPropertyBag)
+NS_INTERFACE_MAP_END
 
 nsExtProtocolChannel::nsExtProtocolChannel(nsIURI* aURI, nsILoadInfo* aLoadInfo)
     : mUrl(aURI),
@@ -177,13 +176,10 @@ nsresult nsExtProtocolChannel::OpenURL() {
       mLoadInfo->RedirectChain().LastElement()->GetPrincipal(
           getter_AddRefs(redirectPrincipal));
     }
-    bool newWindowTarget = false;
-    GetPropertyAsBool(u"docshell.newWindowTarget"_ns, &newWindowTarget);
-
     rv = extProtService->LoadURI(mUrl, triggeringPrincipal, redirectPrincipal,
                                  ctx, mLoadInfo->GetLoadTriggeredFromExternal(),
                                  mLoadInfo->GetHasValidUserGestureActivation(),
-                                 newWindowTarget);
+                                 mLoadInfo->GetIsNewWindowTarget());
 
     if (NS_SUCCEEDED(rv) && mListener) {
       mStatus = NS_ERROR_NO_CONTENT;

uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog.js

@@ -528,3 +528,68 @@ add_task(async function iframe_popup_tab() {
   gBrowser.removeTab(tab);
   gBrowser.removeTab(newTab);
 });
+
+/**
+ * Check that when navigating to a http channel which redirects to a external
+ * protocol in a noopener pop-up window, we show the dialog in the correct tab.
+ */
+add_task(async function redirect_popup_tab() {
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    "https://example.com/"
+  );
+
+  // Wait for the chooser dialog to open in the background tab. It should not
+  // open in the foreground tab which is unrelated to the external protocol
+  // navigation.
+  let dialogWindowPromise = waitForProtocolAppChooserDialog(gBrowser, true);
+
+  // Wait for the new tab to appear. The URI in this tab will never change from
+  // `about:blank` as we're going to just end up opening a dialog, so we can't
+  // use `waitForNewTab`, as that will wait for the tab to actually load
+  // something.
+  let newTabPromise = new Promise(resolve => {
+    gBrowser.tabContainer.addEventListener(
+      "TabOpen",
+      openEvent => resolve(openEvent.target),
+      { once: true }
+    );
+  });
+
+  info("Navigating to redirect to external proto in pop-up");
+  await SpecialPowers.spawn(
+    tab.linkedBrowser,
+    [TEST_PATH + "redirect_helper.sjs?uri=mailto:example@example.com"],
+    async function (popupUri) {
+      content.eval("window.open('" + popupUri + "', '_blank', 'noopener');");
+    }
+  );
+
+  // Wait for the new tab to be opened.
+  info("Waiting for new tab to appear");
+  let newTab = await newTabPromise;
+
+  // Wait for dialog to open in one of the tabs.
+  info("Waiting for dialog to appear");
+  let dialog = await dialogWindowPromise;
+
+  is(
+    gBrowser.getTabDialogBox(newTab.linkedBrowser)._tabDialogManager._topDialog,
+    dialog,
+    "Dialog opened in the background tab"
+  );
+
+  is(
+    dialog._frame.contentDocument.location.href,
+    CONTENT_HANDLING_URL,
+    "Opened dialog is appChooser dialog."
+  );
+
+  // Close the dialog:
+  let dialogClosedPromise = waitForProtocolAppChooserDialog(gBrowser, false);
+  dialog.close();
+  await dialogClosedPromise;
+
+  gBrowser.removeTab(tab);
+  gBrowser.removeTab(newTab);
+});