From c7ffea8862ed9014fa12e83c320dfd2eb8e562da Mon Sep 17 00:00:00 2001
From: Rakina Zata Amni <rakina@chromium.org>
Date: Mon, 16 Aug 2021 10:04:28 +0000
Subject: [PATCH] Allow windows created with noopener option to be closable

Per https://html.spec.whatwg.org/multipage/window-object.html#script-closable
all top-level windows that are created by script, regardless of
whether it actually has its opener bit set or not, should be closable.

Previously our implementation does not set the bit that makes a window
closable when noopener is set. This CL fixes that, by setting that bit
regardless. Also renames the parameter used for that to be more clear.

Bug: 1170131
Change-Id: I4a7da6d1e7a974eb3931a1979bf3b3c6f56b58a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3084253
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912111}
---
 .../tab_web_contents_delegate_android.cc      |  2 +-
 chrome/browser/sync/sync_ui_util.cc           |  2 +-
 .../android/tab_model/tab_model_jni_bridge.cc |  4 +--
 .../blocked_content/blocked_window_params.cc  |  2 +-
 chrome/browser/ui/browser_navigator.cc        |  2 +-
 chrome/browser/ui/browser_navigator_params.cc |  2 +-
 chrome/browser/ui/browser_navigator_params.h  |  7 +++---
 .../renderer_host/render_view_host_impl.cc    |  5 ++--
 .../renderer_host/render_view_host_impl.h     |  8 +++---
 .../web_contents/opened_by_dom_browsertest.cc | 25 +++++++++++++++++++
 .../browser/web_contents/web_contents_impl.cc | 12 ++++-----
 .../browser/web_contents/web_contents_impl.h  |  6 ++---
 content/common/frame.mojom                    |  4 +--
 content/public/browser/web_contents.cc        |  2 +-
 content/public/browser/web_contents.h         |  6 ++---
 content/public/test/render_view_test.cc       |  2 +-
 content/renderer/render_view_impl.cc          |  4 +--
 17 files changed, 62 insertions(+), 33 deletions(-)

diff --git a/chrome/browser/android/tab_web_contents_delegate_android.cc b/chrome/browser/android/tab_web_contents_delegate_android.cc
index d29d111f01d13..02b1440f77e59 100644
--- a/chrome/browser/android/tab_web_contents_delegate_android.cc
+++ b/chrome/browser/android/tab_web_contents_delegate_android.cc
@@ -360,7 +360,7 @@ WebContents* TabWebContentsDelegateAndroid::OpenURLFromTab(
     return WebContentsDelegateAndroid::OpenURLFromTab(source, params);
   }
 
-  popup_delegate->nav_params()->created_with_opener = true;
+  popup_delegate->nav_params()->opened_by_another_window = true;
   TabModelList::HandlePopupNavigation(popup_delegate->nav_params());
   return nullptr;
 }
diff --git a/chrome/browser/sync/sync_ui_util.cc b/chrome/browser/sync/sync_ui_util.cc
index 7bbc08ba34f2f..98d72c269eb85 100644
--- a/chrome/browser/sync/sync_ui_util.cc
+++ b/chrome/browser/sync/sync_ui_util.cc
@@ -193,7 +193,7 @@ void OpenTabForSyncTrustedVaultUserAction(Browser* browser, const GURL& url) {
 
   NavigateParams params(GetSingletonTabNavigateParams(browser, url));
   // Allow the window to close itself.
-  params.created_with_opener = true;
+  params.opened_by_another_window = true;
   Navigate(&params);
 }
 
diff --git a/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc b/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
index d0d9ebf4cf56f..0f547f3c314dc 100644
--- a/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
+++ b/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
@@ -122,8 +122,8 @@ void TabModelJniBridge::HandlePopupNavigation(TabAndroid* parent,
       content::ConvertResourceRequestBodyToJavaObject(env, params->post_data);
   Java_TabModelJniBridge_openNewTab(
       env, jobj, parent->GetJavaObject(), jurl, jinitiator_origin, jheaders,
-      jpost_data, static_cast<int>(disposition), params->created_with_opener,
-      params->is_renderer_initiated);
+      jpost_data, static_cast<int>(disposition),
+      params->opened_by_another_window, params->is_renderer_initiated);
 }
 
 WebContents* TabModelJniBridge::GetWebContentsAt(int index) const {
diff --git a/chrome/browser/ui/blocked_content/blocked_window_params.cc b/chrome/browser/ui/blocked_content/blocked_window_params.cc
index 1823141641b38..1ae4fe0d56a35 100644
--- a/chrome/browser/ui/blocked_content/blocked_window_params.cc
+++ b/chrome/browser/ui/blocked_content/blocked_window_params.cc
@@ -52,7 +52,7 @@ NavigateParams BlockedWindowParams::CreateNavigateParams(
   nav_params.is_renderer_initiated = true;
   nav_params.window_action = NavigateParams::SHOW_WINDOW;
   nav_params.user_gesture = user_gesture_;
-  nav_params.created_with_opener = !opener_suppressed_;
+  nav_params.opened_by_another_window = !opener_suppressed_;
   nav_params.window_bounds = web_contents->GetContainerBounds();
   if (features_.has_x)
     nav_params.window_bounds.set_x(features_.x);
diff --git a/chrome/browser/ui/browser_navigator.cc b/chrome/browser/ui/browser_navigator.cc
index 1fb512b9f9c2d..1c874573619ca 100644
--- a/chrome/browser/ui/browser_navigator.cc
+++ b/chrome/browser/ui/browser_navigator.cc
@@ -443,7 +443,7 @@ std::unique_ptr<content::WebContents> CreateTargetContents(
         params.opener->GetProcess()->GetID();
   }
   if (params.source_contents) {
-    create_params.created_with_opener = params.created_with_opener;
+    create_params.opened_by_another_window = params.opened_by_another_window;
   }
   if (params.disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB)
     create_params.initially_hidden = true;
diff --git a/chrome/browser/ui/browser_navigator_params.cc b/chrome/browser/ui/browser_navigator_params.cc
index 85dd2dbb7066b..d8149ec70af10 100644
--- a/chrome/browser/ui/browser_navigator_params.cc
+++ b/chrome/browser/ui/browser_navigator_params.cc
@@ -86,7 +86,7 @@ void NavigateParams::FillNavigateParamsFromOpenURLParams(
   //   The following NavigateParams don't have an equivalent in OpenURLParams:
   //     browser
   //     contents_to_insert
-  //     created_with_opener
+  //     opened_by_another_window
   //     extension_app_id
   //     frame_name
   //     group
diff --git a/chrome/browser/ui/browser_navigator_params.h b/chrome/browser/ui/browser_navigator_params.h
index 8cf44a922efd2..b5588a3c2cfd1 100644
--- a/chrome/browser/ui/browser_navigator_params.h
+++ b/chrome/browser/ui/browser_navigator_params.h
@@ -262,9 +262,10 @@ struct NavigateParams {
   // navigation entry.
   bool should_replace_current_entry = false;
 
-  // Indicates whether |contents_to_insert| is being created with a
-  // window.opener.
-  bool created_with_opener = false;
+  // Indicates whether |contents_to_insert| is being created by another window,
+  // and thus can be closed via window.close(). This may be true even when
+  // "noopener" was used.
+  bool opened_by_another_window = false;
 
   // Whether or not the related navigation was started in the context menu.
   bool started_from_context_menu = false;
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index 9786cb8ef6633..e4f71e2ce9cf7 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -369,7 +369,7 @@ RenderViewHostDelegate* RenderViewHostImpl::GetDelegate() {
 bool RenderViewHostImpl::CreateRenderView(
     const absl::optional<blink::FrameToken>& opener_frame_token,
     int proxy_route_id,
-    bool window_was_created_with_opener) {
+    bool window_was_opened_by_another_window) {
   TRACE_EVENT0("renderer_host,navigation",
                "RenderViewHostImpl::CreateRenderView");
   DCHECK(!IsRenderViewLive()) << "Creating view twice";
@@ -461,7 +461,8 @@ bool RenderViewHostImpl::CreateRenderView(
       frame_tree_->controller().GetSessionStorageNamespace(site_info_)->id();
   params->hidden = frame_tree_->delegate()->IsHidden();
   params->never_composited = delegate_->IsNeverComposited();
-  params->window_was_created_with_opener = window_was_created_with_opener;
+  params->window_was_opened_by_another_window =
+      window_was_opened_by_another_window;
   params->base_background_color = delegate_->GetBaseBackgroundColor();
 
   bool is_portal = delegate_->IsPortal();
diff --git a/content/browser/renderer_host/render_view_host_impl.h b/content/browser/renderer_host/render_view_host_impl.h
index 0e95eaa02ae02..b9a91d4cf2237 100644
--- a/content/browser/renderer_host/render_view_host_impl.h
+++ b/content/browser/renderer_host/render_view_host_impl.h
@@ -152,14 +152,16 @@ class CONTENT_EXPORT RenderViewHostImpl
   // TestRenderViewHost.
   // |opener_route_id| parameter indicates which RenderView created this
   //   (MSG_ROUTING_NONE if none).
-  // |window_was_created_with_opener| is true if this top-level frame was
-  //   created with an opener. (The opener may have been closed since.)
+  // |window_was_opened_by_another_window| is true if this top-level frame was
+  // created by another window, as opposed to independently created (through
+  // the browser UI, etc). This is true even when the window is opened with
+  // "noopener", and even if the opener has been closed since.
   // |proxy_route_id| is only used when creating a RenderView in an inactive
   //   state.
   virtual bool CreateRenderView(
       const absl::optional<blink::FrameToken>& opener_frame_token,
       int proxy_route_id,
-      bool window_was_created_with_opener);
+      bool window_was_opened_by_another_window);
 
   // Tracks whether this RenderViewHost is in an active state (rather than
   // pending unload or unloaded), according to its main frame
diff --git a/content/browser/web_contents/opened_by_dom_browsertest.cc b/content/browser/web_contents/opened_by_dom_browsertest.cc
index 4dab737256977..d92e94fb952d9 100644
--- a/content/browser/web_contents/opened_by_dom_browsertest.cc
+++ b/content/browser/web_contents/opened_by_dom_browsertest.cc
@@ -115,6 +115,31 @@ IN_PROC_BROWSER_TEST_F(OpenedByDOMTest, Popup) {
   EXPECT_TRUE(AttemptCloseFromJavaScript(popup->web_contents()));
 }
 
+// Tests that window.close() works in a popup window that's opened with noopener
+// that has navigated a few times.
+IN_PROC_BROWSER_TEST_F(OpenedByDOMTest, NoOpenerPopup) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url1 = embedded_test_server()->GetURL("/site_isolation/blank.html?1");
+  GURL url2 = embedded_test_server()->GetURL("/site_isolation/blank.html?2");
+  GURL url3 = embedded_test_server()->GetURL("/site_isolation/blank.html?3");
+  EXPECT_TRUE(NavigateToURL(shell(), url1));
+
+  // Create a popup through window.open() and 'noopener'.
+  ShellAddedObserver new_shell_observer;
+  TestNavigationObserver nav_observer(nullptr);
+  nav_observer.StartWatchingNewWebContents();
+  CHECK(
+      ExecJs(shell(), JsReplace("window.open($1, '_blank','noopener')", url2)));
+  nav_observer.Wait();
+  Shell* popup = new_shell_observer.GetShell();
+
+  // Navigate the popup.
+  EXPECT_TRUE(NavigateToURL(popup, url3));
+  // Closing the popup should still work.
+  EXPECT_TRUE(AttemptCloseFromJavaScript(popup->web_contents()));
+}
+
 // Tests that window.close() works in a popup window that has navigated a few
 // times and swapped processes.
 IN_PROC_BROWSER_TEST_F(OpenedByDOMTest, CrossProcessPopup) {
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 357c6a58efccf..6f0d2cee13b39 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -824,7 +824,7 @@ void WebContentsImpl::WebContentsObserverList::RemoveObserver(
 WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
     : delegate_(nullptr),
       render_view_host_delegate_view_(nullptr),
-      created_with_opener_(false),
+      opened_by_another_window_(false),
       node_(this),
       frame_tree_(browser_context,
                   this,
@@ -1038,8 +1038,8 @@ std::unique_ptr<WebContentsImpl> WebContentsImpl::CreateWithOpener(
 
   // This may be true even when opener is null, such as when opening blocked
   // popups.
-  if (params.created_with_opener)
-    new_contents->created_with_opener_ = true;
+  if (params.opened_by_another_window)
+    new_contents->opened_by_another_window_ = true;
 
   WebContentsImpl* outer_web_contents = nullptr;
   if (params.guest_delegate) {
@@ -7845,9 +7845,9 @@ bool WebContentsImpl::CreateRenderViewForRenderManager(
   const auto proxy_routing_id =
       proxy_host ? proxy_host->GetRoutingID() : MSG_ROUTING_NONE;
   // TODO(https://crbug.com/1171646): Given MPArch, should we pass
-  // created_with_opener_ for non primary FrameTrees?
+  // opened_by_another_window_ for non primary FrameTrees?
   if (!rvh_impl->CreateRenderView(opener_frame_token, proxy_routing_id,
-                                  created_with_opener_)) {
+                                  opened_by_another_window_)) {
     return false;
   }
   // Set the TextAutosizer state from the main frame's renderer on the new view,
@@ -8806,10 +8806,10 @@ void WebContentsImpl::SetOpenerForNewContents(FrameTreeNode* opener,
     // https://crbug.com/705316
     new_root->SetOriginalOpener(opener->frame_tree()->root());
     new_root->SetOpenerDevtoolsFrameToken(opener->devtools_frame_token());
+    opened_by_another_window_ = true;
 
     if (!opener_suppressed) {
       new_root->SetOpener(opener);
-      created_with_opener_ = true;
     }
   }
 }
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 59fcd30e80b9f..7183994d0bbcd 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -1833,9 +1833,9 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   // the observer list then.
   WebContentsObserverList observers_;
 
-  // True if this tab was opened by another tab. This is not unset if the opener
-  // is closed.
-  bool created_with_opener_;
+  // True if this tab was opened by another window. This is true even if the tab
+  // is opened with "noopener", and won't be unset if the opener is closed.
+  bool opened_by_another_window_;
 
 #if defined(OS_ANDROID)
   std::unique_ptr<WebContentsAndroid> web_contents_android_;
diff --git a/content/common/frame.mojom b/content/common/frame.mojom
index 192a5084eac3e..72fd11b1733aa 100644
--- a/content/common/frame.mojom
+++ b/content/common/frame.mojom
@@ -118,8 +118,8 @@ struct CreateViewParams {
   // for displaying graphical output.
   bool never_composited;
 
-  // Whether the window associated with this view was created with an opener.
-  bool window_was_created_with_opener;
+  // Whether the window associated with this view was created by another window.
+  bool window_was_opened_by_another_window;
 
   // Whether lookup of frames in the created RenderView (e.g. lookup via
   // window.open or via <a target=...>) should be renderer-wide (i.e. going
diff --git a/content/public/browser/web_contents.cc b/content/public/browser/web_contents.cc
index f1dcf53ea481b..e40e14fd4913f 100644
--- a/content/public/browser/web_contents.cc
+++ b/content/public/browser/web_contents.cc
@@ -22,7 +22,7 @@ WebContents::CreateParams::CreateParams(BrowserContext* context,
       opener_render_process_id(content::ChildProcessHost::kInvalidUniqueID),
       opener_render_frame_id(MSG_ROUTING_NONE),
       opener_suppressed(false),
-      created_with_opener(false),
+      opened_by_another_window(false),
       initially_hidden(false),
       guest_delegate(nullptr),
       context(nullptr),
diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
index 91c8bb57fb045..ddf33e29d8f06 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -141,11 +141,11 @@ class WebContents : public PageNavigator,
     // reference to its opener.
     bool opener_suppressed;
 
-    // Indicates whether this WebContents was created with a window.opener.
+    // Indicates whether this WebContents was created by another window.
     // This is used when determining whether the WebContents is allowed to be
     // closed via window.close(). This may be true even with a null |opener|
-    // (e.g., for blocked popups).
-    bool created_with_opener;
+    // (e.g., for blocked popups), or when the window is opened with "noopener".
+    bool opened_by_another_window;
 
     // The name of the top-level frame of the new window. It is non-empty
     // when creating a named window (e.g. <a target="foo"> or
diff --git a/content/public/test/render_view_test.cc b/content/public/test/render_view_test.cc
index 2d2ff399ff5f4..aa13a70f24c42 100644
--- a/content/public/test/render_view_test.cc
+++ b/content/public/test/render_view_test.cc
@@ -496,7 +496,7 @@ void RenderViewTest::SetUp() {
 
   mojom::CreateViewParamsPtr view_params = mojom::CreateViewParams::New();
   view_params->opener_frame_token = absl::nullopt;
-  view_params->window_was_created_with_opener = false;
+  view_params->window_was_opened_by_another_window = false;
   view_params->renderer_preferences = blink::RendererPreferences();
   view_params->web_preferences = blink::web_pref::WebPreferences();
   view_params->view_id = render_thread_->GetNextRoutingID();
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index afec7767d9054..092352d96858b 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -152,7 +152,7 @@ void RenderViewImpl::Initialize(
   }
 
   // TODO(davidben): Move this state from Blink into content.
-  if (params->window_was_created_with_opener)
+  if (params->window_was_opened_by_another_window)
     GetWebView()->SetOpenedByDOM();
 
   webview_->SetRendererPreferences(params->renderer_preferences);
@@ -339,7 +339,7 @@ WebView* RenderViewImpl::CreateView(
   view_params->opener_frame_token = creator->GetFrameToken();
   DCHECK_EQ(GetRoutingID(), creator_frame->render_view()->GetRoutingID());
 
-  view_params->window_was_created_with_opener = true;
+  view_params->window_was_opened_by_another_window = true;
   view_params->renderer_preferences = webview_->GetRendererPreferences();
   view_params->web_preferences = webview_->GetWebPreferences();
   view_params->view_id = reply->route_id;