|
@@ -0,0 +1,242 @@
|
|
|
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
|
|
+From: Garrett Tanzer <[email protected]>
|
|
|
+Date: Wed, 2 Mar 2022 18:33:42 +0000
|
|
|
+Subject: Reland "Fix noopener case for user activation consumption"
|
|
|
+
|
|
|
+This is a reland of e9828a82b5c182dc9a7fb0ae7226c35ba1726e7d
|
|
|
+
|
|
|
+The MSAN error is from checking status before err in
|
|
|
+content/renderer/render_view_impl.cc .
|
|
|
+https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/b8821495655905086193/overview
|
|
|
+
|
|
|
+The fix is to split the check for err and kIgnore into two checks,
|
|
|
+and put the err check before kBlocked.
|
|
|
+
|
|
|
+It is probably possible for the browser to consume user activation
|
|
|
+but then eventually mojo returns an error and the renderer doesn't
|
|
|
+consume activation, but that seems pretty marginal.
|
|
|
+
|
|
|
+Original change's description:
|
|
|
+> Fix noopener case for user activation consumption
|
|
|
+>
|
|
|
+>
|
|
|
+> The flow for user activation consumption in window.open was as follows:
|
|
|
+>
|
|
|
+> Renderer: ask the browser to create a new window
|
|
|
+> Browser: consume transient user activation (in the browser, and via RPC
|
|
|
+> to remote frames only)
|
|
|
+> Browser: return success for opener, return ignore for noopener
|
|
|
+> Renderer: consume transient user activation upon success
|
|
|
+>
|
|
|
+> So in the noopener case, the renderer with the local frame where the
|
|
|
+> window.open originated didn't have its transient user activation
|
|
|
+> consumed.
|
|
|
+>
|
|
|
+>
|
|
|
+> The new behavior is to consume user activation in the calling renderer
|
|
|
+> whenever it is consumed in the browser. We accomplish this by returning
|
|
|
+> a distinct value kBlocked to represent failure before the browser
|
|
|
+> consumes user activation.
|
|
|
+>
|
|
|
+> Bug: 1264543, 1291210
|
|
|
+> Change-Id: Iffb6e3fd772bef625d3d28e600e6fb73d70ab29f
|
|
|
+> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3468171
|
|
|
+> Reviewed-by: Dominic Farolino <[email protected]>
|
|
|
+> Reviewed-by: Ken Buchanan <[email protected]>
|
|
|
+> Reviewed-by: Mustaq Ahmed <[email protected]>
|
|
|
+> Reviewed-by: Charles Reis <[email protected]>
|
|
|
+> Reviewed-by: Jonathan Ross <[email protected]>
|
|
|
+> Reviewed-by: Daniel Cheng <[email protected]>
|
|
|
+> Commit-Queue: Garrett Tanzer <[email protected]>
|
|
|
+> Cr-Commit-Position: refs/heads/main@{#973876}
|
|
|
+
|
|
|
+Bug: 1264543, 1291210
|
|
|
+Change-Id: Ie27c4d68db34dfd98adee7cc5c743953dad59834
|
|
|
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3481666
|
|
|
+Reviewed-by: Jonathan Ross <[email protected]>
|
|
|
+Reviewed-by: Daniel Cheng <[email protected]>
|
|
|
+Reviewed-by: Mustaq Ahmed <[email protected]>
|
|
|
+Reviewed-by: Ken Buchanan <[email protected]>
|
|
|
+Reviewed-by: Charles Reis <[email protected]>
|
|
|
+Commit-Queue: Garrett Tanzer <[email protected]>
|
|
|
+Cr-Commit-Position: refs/heads/main@{#976745}
|
|
|
+
|
|
|
+diff --git a/chrome/browser/site_isolation/chrome_site_per_process_browsertest.cc b/chrome/browser/site_isolation/chrome_site_per_process_browsertest.cc
|
|
|
+index 84867adc5ec36ac76a4d043e80d331ff314858d5..6c21360f68569805989598e525c89ad44ecaba11 100644
|
|
|
+--- a/chrome/browser/site_isolation/chrome_site_per_process_browsertest.cc
|
|
|
++++ b/chrome/browser/site_isolation/chrome_site_per_process_browsertest.cc
|
|
|
+@@ -1187,6 +1187,52 @@ IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
|
|
|
+ EXPECT_FALSE(frame_c_popup_opened);
|
|
|
+ }
|
|
|
+
|
|
|
++// Test that opening a window with `noopener` consumes user activation.
|
|
|
++// crbug.com/1264543, crbug.com/1291210
|
|
|
++IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
|
|
|
++ UserActivationConsumptionNoopener) {
|
|
|
++ // Start on a page a.com.
|
|
|
++ GURL main_url(embedded_test_server()->GetURL(
|
|
|
++ "a.com", "/cross_site_iframe_factory.html?a"));
|
|
|
++ ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), main_url));
|
|
|
++ content::WebContents* web_contents =
|
|
|
++ browser()->tab_strip_model()->GetActiveWebContents();
|
|
|
++
|
|
|
++ // Activate the frame by executing a dummy script.
|
|
|
++ const std::string no_op_script = "// No-op script";
|
|
|
++ EXPECT_TRUE(ExecuteScript(web_contents, no_op_script));
|
|
|
++
|
|
|
++ // Add a popup observer.
|
|
|
++ content::TestNavigationObserver popup_observer(nullptr);
|
|
|
++ popup_observer.StartWatchingNewWebContents();
|
|
|
++
|
|
|
++ // Open a popup from the frame, with `noopener`. This should consume
|
|
|
++ // transient user activation.
|
|
|
++ GURL popup_url(embedded_test_server()->GetURL("popup.com", "/"));
|
|
|
++ EXPECT_TRUE(ExecuteScriptWithoutUserGesture(
|
|
|
++ web_contents,
|
|
|
++ base::StringPrintf(
|
|
|
++ "window.w = window.open('%s'+'title1.html', '_blank', 'noopener');",
|
|
|
++ popup_url.spec().c_str())));
|
|
|
++
|
|
|
++ // Try to open another popup.
|
|
|
++ EXPECT_TRUE(ExecuteScriptWithoutUserGesture(
|
|
|
++ web_contents,
|
|
|
++ base::StringPrintf(
|
|
|
++ "window.w = window.open('%s'+'title2.html', '_blank', 'noopener');",
|
|
|
++ popup_url.spec().c_str())));
|
|
|
++
|
|
|
++ // Wait and check that only one popup was opened.
|
|
|
++ popup_observer.Wait();
|
|
|
++ EXPECT_EQ(2, browser()->tab_strip_model()->count());
|
|
|
++
|
|
|
++ content::WebContents* popup =
|
|
|
++ browser()->tab_strip_model()->GetActiveWebContents();
|
|
|
++ EXPECT_EQ(embedded_test_server()->GetURL("popup.com", "/title1.html"),
|
|
|
++ popup->GetLastCommittedURL());
|
|
|
++ EXPECT_NE(popup, web_contents);
|
|
|
++}
|
|
|
++
|
|
|
+ // TODO(crbug.com/1021895): Flaky.
|
|
|
+ // Tests that a cross-site iframe runs its beforeunload handler when closing a
|
|
|
+ // tab. See https://crbug.com/853021.
|
|
|
+diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
|
|
|
+index 6bcd908794675c046bbfed26a979f7fbf123b190..4fc8b67dc616ed744735d5f6ce9a6d985edcef09 100644
|
|
|
+--- a/content/browser/renderer_host/render_frame_host_impl.cc
|
|
|
++++ b/content/browser/renderer_host/render_frame_host_impl.cc
|
|
|
+@@ -6639,17 +6639,22 @@ void RenderFrameHostImpl::CreateNewWindow(
|
|
|
+ effective_transient_activation_state, params->opener_suppressed,
|
|
|
+ &no_javascript_access);
|
|
|
+
|
|
|
+- bool was_consumed = false;
|
|
|
+- if (can_create_window) {
|
|
|
+- // Consume activation even w/o User Activation v2, to sync other renderers
|
|
|
+- // with calling renderer.
|
|
|
+- was_consumed = frame_tree_node_->UpdateUserActivationState(
|
|
|
+- blink::mojom::UserActivationUpdateType::kConsumeTransientActivation,
|
|
|
+- blink::mojom::UserActivationNotificationType::kNone);
|
|
|
+- } else {
|
|
|
+- std::move(callback).Run(mojom::CreateNewWindowStatus::kIgnore, nullptr);
|
|
|
+- return;
|
|
|
+- }
|
|
|
++ // If this frame isn't allowed to create a window, return early (before we
|
|
|
++ // consume transient user activation).
|
|
|
++ if (!can_create_window) {
|
|
|
++ std::move(callback).Run(mojom::CreateNewWindowStatus::kBlocked, nullptr);
|
|
|
++ return;
|
|
|
++ }
|
|
|
++
|
|
|
++ // Otherwise, consume user activation before we proceed. In particular, it is
|
|
|
++ // important to do this before we return from the |opener_suppressed| case
|
|
|
++ // below.
|
|
|
++ // NB: This call will consume activations in the browser and the remote frame
|
|
|
++ // proxies for this frame. The initiating renderer will consume its view of
|
|
|
++ // the activations after we return.
|
|
|
++ bool was_consumed = frame_tree_node_->UpdateUserActivationState(
|
|
|
++ blink::mojom::UserActivationUpdateType::kConsumeTransientActivation,
|
|
|
++ blink::mojom::UserActivationNotificationType::kNone);
|
|
|
+
|
|
|
+ // For Android WebView, we support a pop-up like behavior for window.open()
|
|
|
+ // even if the embedding app doesn't support multiple windows. In this case,
|
|
|
+diff --git a/content/common/frame.mojom b/content/common/frame.mojom
|
|
|
+index da44b637ee5fdf371974f322aaf1a07ba8c018d7..44444b35b55e6dd3af181b256515d4dad6cba749 100644
|
|
|
+--- a/content/common/frame.mojom
|
|
|
++++ b/content/common/frame.mojom
|
|
|
+@@ -558,8 +558,10 @@ struct CreateNewWindowParams {
|
|
|
+
|
|
|
+ // Operation result when the renderer asks the browser to create a new window.
|
|
|
+ enum CreateNewWindowStatus {
|
|
|
+- // Ignore creation of the new window. This can happen because creation is
|
|
|
+- // blocked or because the new window should have no opener relationship.
|
|
|
++ // Creation of the new window was blocked, e.g. because the source frame
|
|
|
++ // doesn't have user activation.
|
|
|
++ kBlocked,
|
|
|
++ // Ignore creation of the new window, e.g. because noopener is in effect.
|
|
|
+ kIgnore,
|
|
|
+ // Reuse the current window rather than creating a new window.
|
|
|
+ kReuse,
|
|
|
+diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
|
|
|
+index 190b517cea51bd3eae29695ba45efb22c4c82877..e1d189467bbd266de01da186c1fac6e20c9f35ad 100644
|
|
|
+--- a/content/renderer/render_view_impl.cc
|
|
|
++++ b/content/renderer/render_view_impl.cc
|
|
|
+@@ -310,8 +310,27 @@ WebView* RenderViewImpl::CreateView(
|
|
|
+ mojom::CreateNewWindowStatus status;
|
|
|
+ mojom::CreateNewWindowReplyPtr reply;
|
|
|
+ auto* frame_host = creator_frame->GetFrameHost();
|
|
|
+- bool err = !frame_host->CreateNewWindow(std::move(params), &status, &reply);
|
|
|
+- if (err || status == mojom::CreateNewWindowStatus::kIgnore)
|
|
|
++ if (!frame_host->CreateNewWindow(std::move(params), &status, &reply)) {
|
|
|
++ // The sync IPC failed, e.g. maybe the render process is in the middle of
|
|
|
++ // shutting down. Can't create a new window without the browser process,
|
|
|
++ // so just bail out.
|
|
|
++ return nullptr;
|
|
|
++ }
|
|
|
++
|
|
|
++ // If creation of the window was blocked (e.g. because this frame doesn't
|
|
|
++ // have user activation), return before consuming user activation. A frame
|
|
|
++ // that isn't allowed to open a window shouldn't be able to consume the
|
|
|
++ // activation for the rest of the frame tree.
|
|
|
++ if (status == mojom::CreateNewWindowStatus::kBlocked)
|
|
|
++ return nullptr;
|
|
|
++
|
|
|
++ // Consume the transient user activation in the current renderer.
|
|
|
++ consumed_user_gesture = creator->ConsumeTransientUserActivation(
|
|
|
++ blink::UserActivationUpdateSource::kBrowser);
|
|
|
++
|
|
|
++ // If we should ignore the new window (e.g. because of `noopener`), return
|
|
|
++ // now that user activation was consumed.
|
|
|
++ if (status == mojom::CreateNewWindowStatus::kIgnore)
|
|
|
+ return nullptr;
|
|
|
+
|
|
|
+ // For Android WebView, we support a pop-up like behavior for window.open()
|
|
|
+@@ -331,11 +350,6 @@ WebView* RenderViewImpl::CreateView(
|
|
|
+ DCHECK_NE(MSG_ROUTING_NONE, reply->main_frame_route_id);
|
|
|
+ DCHECK_NE(MSG_ROUTING_NONE, reply->widget_params->routing_id);
|
|
|
+
|
|
|
+- // The browser allowed creation of a new window and consumed the user
|
|
|
+- // activation.
|
|
|
+- consumed_user_gesture = creator->ConsumeTransientUserActivation(
|
|
|
+- blink::UserActivationUpdateSource::kBrowser);
|
|
|
+-
|
|
|
+ // While this view may be a background extension page, it can spawn a visible
|
|
|
+ // render view. So we just assume that the new one is not another background
|
|
|
+ // page instead of passing on our own value.
|
|
|
+diff --git a/third_party/blink/web_tests/wpt_internal/fenced_frame/restrict-size.https.html b/third_party/blink/web_tests/wpt_internal/fenced_frame/restrict-size.https.html
|
|
|
+new file mode 100644
|
|
|
+index 0000000000000000000000000000000000000000..5668407d7e1e6ac7840fc47b76869787cb3f3d63
|
|
|
+--- /dev/null
|
|
|
++++ b/third_party/blink/web_tests/wpt_internal/fenced_frame/restrict-size.https.html
|
|
|
+@@ -0,0 +1,15 @@
|
|
|
++<!DOCTYPE html>
|
|
|
++<title>Test fencedframe size restrictions in opaque ads mode.</title>
|
|
|
++<script src="/resources/testharness.js"></script>
|
|
|
++<script src="/resources/testharnessreport.js"></script>
|
|
|
++<script src="/common/utils.js"></script>
|
|
|
++<script src="/common/dispatcher/dispatcher.js"></script>
|
|
|
++<script src="resources/utils.js"></script>
|
|
|
++
|
|
|
++<body>
|
|
|
++<script>
|
|
|
++promise_test(async () => {
|
|
|
++ const frame = attachFencedFrameContext();
|
|
|
++}, 'restrict fencedframe size');
|
|
|
++</script>
|
|
|
++</body>
|