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}

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;
 }

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);
 }
 

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 {

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);

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;

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

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;

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();

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

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) {

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;
     }
   }
 }

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_;

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

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),

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

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();

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;