-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick d74d2b9f00c7 from chromium (#35550)
* chore: [18-x-y] cherry-pick d74d2b9f00c7 from chromium * chore: update patches Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Electron Bot <electron@github.com>
- Loading branch information
1 parent
9e80793
commit 4357c7f
Showing
2 changed files
with
249 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,248 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: Zakhar Voit <voit@google.com> | ||
Date: Wed, 24 Aug 2022 10:59:16 +0000 | ||
Subject: Ensure mouse lock widget pointers are cleared in WebContents | ||
destructor | ||
|
||
Requesting mouse/pointer lock (e.g., via requestPointerLock() from JS) | ||
results in setting mouse_lock_widget_ to point to the | ||
RenderWidgetHost that has the mouse lock, in both the widget's | ||
WebContents and all its outer WebContents. When a WebContents is | ||
destroyed, it normally checks if it has an active mouse lock widget | ||
and calls RejectMouseLockOrUnlockIfNecessary() if so. This usually | ||
results in calling LostMouseLock(), which will clear | ||
mouse_lock_widget_ in both the WebContents being destroyed and all its | ||
ancestor WebContents. However, there's a time window where this | ||
doesn't work with <webview>, where a mouse lock request in the guest | ||
has to go up to the embedder to asynchronously ask it for the | ||
corresponding permission before it can be granted. If the embedder | ||
ends up destroying the <webview> guest while the guest's mouse lock | ||
request is pending (prior to responding to that request), it could end | ||
up with a stale mouse_lock_widget_ pointer, since | ||
RejectMouseLockOrUnlockIfNecessary() follows a different path for | ||
pending requests and doesn't clear those pointers. Sadly, the | ||
RenderWidgetHost destruction is also not going to trigger clearing | ||
these pointers as it normally does, since ~WebContentsImpl clears | ||
delegate_ pointers for all of its widgets before destroying them, | ||
causing ~RenderWidgetHostImpl::Destroy() to not call | ||
WebContentsImpl::RenderWidgetDeleted(), which normally does this. | ||
|
||
This CL ensures that all mouse_lock_widget_ pointers are cleared on | ||
the entire WebContents chain in the WebContentsImpl destructor. In the | ||
future, we could also investigate not setting mouse_lock_widget_ | ||
before we actually decide that a mouse lock request should proceed, | ||
and removing the current implementation's dependency on that behavior. | ||
|
||
(cherry picked from commit 8380553a222cbc2c537ab67fc96e50f611ba4560) | ||
|
||
(cherry picked from commit 859cf771d8364577cce49da5520b0e4b44ebb5a9) | ||
|
||
Bug: 1346245 | ||
Change-Id: Iaf1fec400ca47d7cb20c21ce145dc041317a7db6 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3823606 | ||
Commit-Queue: Alex Moshchuk <alexmos@chromium.org> | ||
Cr-Original-Original-Commit-Position: refs/heads/main@{#1034481} | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3838431 | ||
Cr-Original-Commit-Position: refs/branch-heads/5112@{#1498} | ||
Cr-Original-Branched-From: b13d3fe7b3c47a56354ef54b221008afa754412e-refs/heads/main@{#1012729} | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3848803 | ||
Owners-Override: Artem Sumaneev <asumaneev@google.com> | ||
Commit-Queue: Zakhar Voit <voit@google.com> | ||
Reviewed-by: Artem Sumaneev <asumaneev@google.com> | ||
Cr-Commit-Position: refs/branch-heads/5005@{#1320} | ||
Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738} | ||
|
||
diff --git a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc | ||
index eabc5f0ff26468ce557e7eff5e7d1108eb84887a..e078b7f461b21c498e5d60b150978f34cb3c7ad2 100644 | ||
--- a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc | ||
+++ b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc | ||
@@ -1490,6 +1490,40 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, MAYBE_KeyboardFocusWindowCycle) { | ||
ASSERT_TRUE(next_step_listener.WaitUntilSatisfied()); | ||
} | ||
|
||
+// Ensure that destroying a <webview> with a pending mouse lock request doesn't | ||
+// leave a stale mouse lock widget pointer in the embedder WebContents. See | ||
+// https://crbug.com/1346245. | ||
+IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, | ||
+ DestroyGuestWithPendingPointerLock) { | ||
+ LoadAndLaunchPlatformApp("web_view/pointer_lock_pending", | ||
+ "WebViewTest.LAUNCHED"); | ||
+ | ||
+ content::WebContents* embedder_web_contents = GetFirstAppWindowWebContents(); | ||
+ content::WebContents* guest_web_contents = | ||
+ GetGuestViewManager()->WaitForSingleGuestCreated(); | ||
+ | ||
+ // The embedder is configured to remove the <webview> as soon as it receives | ||
+ // the pointer lock permission request from the guest, without responding to | ||
+ // it. Hence, have the guest request pointer lock and wait for its | ||
+ // destruction. | ||
+ content::RenderFrameDeletedObserver observer( | ||
+ guest_web_contents->GetMainFrame()); | ||
+ EXPECT_TRUE(content::ExecuteScript( | ||
+ guest_web_contents, | ||
+ "document.querySelector('div').requestPointerLock()")); | ||
+ observer.WaitUntilDeleted(); | ||
+ | ||
+ // The embedder WebContents shouldn't have a mouse lock widget. | ||
+ EXPECT_FALSE(GetMouseLockWidget(embedder_web_contents)); | ||
+ | ||
+ // Close the embedder app and ensure that this doesn't crash, which used to | ||
+ // be the case if the mouse lock widget (now destroyed) hadn't been cleared | ||
+ // in the embedder. | ||
+ content::WebContentsDestroyedWatcher destroyed_watcher(embedder_web_contents); | ||
+ CloseAppWindow(GetFirstAppWindow()); | ||
+ destroyed_watcher.Wait(); | ||
+} | ||
+ | ||
#if BUILDFLAG(IS_MAC) | ||
// This test verifies that replacement range for IME works with <webview>s. To | ||
// verify this, a <webview> with an <input> inside is loaded. Then the <input> | ||
diff --git a/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.html b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.html | ||
new file mode 100644 | ||
index 0000000000000000000000000000000000000000..936af1b4ef367a72dbc9c689d119019a10856f42 | ||
--- /dev/null | ||
+++ b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.html | ||
@@ -0,0 +1,10 @@ | ||
+<!-- | ||
+ * Copyright 2022 The Chromium Authors. All rights reserved. Use of this | ||
+ * source code is governed by a BSD-style license that can be found in the | ||
+ * LICENSE file. | ||
+--> | ||
+<html> | ||
+<body> | ||
+ <script src="main.js"></script> | ||
+</body> | ||
+</html> | ||
diff --git a/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.js b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.js | ||
new file mode 100644 | ||
index 0000000000000000000000000000000000000000..c851df9d0ffce8ec432902fb2cd0a3b6ef5047c8 | ||
--- /dev/null | ||
+++ b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/main.js | ||
@@ -0,0 +1,22 @@ | ||
+// Copyright 2022 The Chromium Authors. All rights reserved. | ||
+// Use of this source code is governed by a BSD-style license that can be | ||
+// found in the LICENSE file. | ||
+ | ||
+onload = function() { | ||
+ var webview = document.createElement('webview'); | ||
+ | ||
+ webview.addEventListener('permissionrequest', (e) => { | ||
+ if (e.permission != 'pointerLock') { | ||
+ console.log('Received unexpected permission request: ' + e.permission); | ||
+ e.chrome.test.sendMessage('WebViewTest.FAILURE'); | ||
+ } | ||
+ webview.parentNode.removeChild(webview); | ||
+ }); | ||
+ | ||
+ webview.addEventListener('loadstop', (e) => { | ||
+ chrome.test.sendMessage('WebViewTest.LAUNCHED'); | ||
+ }); | ||
+ | ||
+ webview.src = 'data:text/html,<html><body><div></div></body></html>'; | ||
+ document.body.appendChild(webview); | ||
+}; | ||
diff --git a/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/manifest.json b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/manifest.json | ||
new file mode 100644 | ||
index 0000000000000000000000000000000000000000..ec20c5a50fa78de2e1891a595096a183d6ef7223 | ||
--- /dev/null | ||
+++ b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/manifest.json | ||
@@ -0,0 +1,13 @@ | ||
+{ | ||
+ "name": "<webview> pointer lock test.", | ||
+ "manifest_version": 2, | ||
+ "version": "1", | ||
+ "permissions": [ | ||
+ "webview" | ||
+ ], | ||
+ "app": { | ||
+ "background": { | ||
+ "scripts": ["test.js"] | ||
+ } | ||
+ } | ||
+} | ||
diff --git a/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/test.js b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/test.js | ||
new file mode 100644 | ||
index 0000000000000000000000000000000000000000..2b2b9c232e594f7d5d21f2ce1150518a86f92f0a | ||
--- /dev/null | ||
+++ b/chrome/test/data/extensions/platform_apps/web_view/pointer_lock_pending/test.js | ||
@@ -0,0 +1,7 @@ | ||
+// Copyright 2022 The Chromium Authors. All rights reserved. | ||
+// Use of this source code is governed by a BSD-style license that can be | ||
+// found in the LICENSE file. | ||
+ | ||
+chrome.app.runtime.onLaunched.addListener(function() { | ||
+ chrome.app.window.create('main.html', {}, function () {}); | ||
+}); | ||
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc | ||
index cbcad21d8e320c6019cc7fbf31bd1c2aac5d03d8..8120b63d4d2ea000a9b9ad2e784ef0fe7effbf94 100644 | ||
--- a/content/browser/web_contents/web_contents_impl.cc | ||
+++ b/content/browser/web_contents/web_contents_impl.cc | ||
@@ -1023,10 +1023,22 @@ WebContentsImpl::~WebContentsImpl() { | ||
outermost->SetAsFocusedWebContentsIfNecessary(); | ||
} | ||
|
||
- if (mouse_lock_widget_) | ||
+ if (mouse_lock_widget_) { | ||
mouse_lock_widget_->RejectMouseLockOrUnlockIfNecessary( | ||
blink::mojom::PointerLockResult::kElementDestroyed); | ||
|
||
+ // Normally, the call above clears mouse_lock_widget_ pointers on the | ||
+ // entire WebContents chain, since it results in calling LostMouseLock() | ||
+ // when the mouse lock is already active. However, this doesn't work for | ||
+ // <webview> guests if the mouse lock request is still pending while the | ||
+ // <webview> is destroyed. Hence, ensure that all mouse lock widget | ||
+ // pointers are cleared. See https://crbug.com/1346245. | ||
+ for (WebContentsImpl* current = this; current; | ||
+ current = current->GetOuterWebContents()) { | ||
+ current->mouse_lock_widget_ = nullptr; | ||
+ } | ||
+ } | ||
+ | ||
for (RenderWidgetHostImpl* widget : created_widgets_) | ||
widget->DetachDelegate(); | ||
created_widgets_.clear(); | ||
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h | ||
index fad1663512cf30b270289ecfa9c336b08fe67836..84fd84944cb587729a472f7f630d3ec1c4b1dab6 100644 | ||
--- a/content/browser/web_contents/web_contents_impl.h | ||
+++ b/content/browser/web_contents/web_contents_impl.h | ||
@@ -1318,6 +1318,10 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents, | ||
bool CancelPrerendering(FrameTreeNode* frame_tree_node, | ||
PrerenderHost::FinalStatus final_status); | ||
|
||
+ RenderWidgetHost* mouse_lock_widget_for_testing() { | ||
+ return mouse_lock_widget_; | ||
+ } | ||
+ | ||
private: | ||
using FrameTreeIterationCallback = base::RepeatingCallback<void(FrameTree*)>; | ||
using RenderViewHostIterationCallback = | ||
diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc | ||
index 7d158699fc826ba81fe0a271605c3076b01a7590..0614592e1ac848d5d79657ba0524def297055051 100644 | ||
--- a/content/public/test/browser_test_utils.cc | ||
+++ b/content/public/test/browser_test_utils.cc | ||
@@ -2353,6 +2353,11 @@ RenderWidgetHost* GetKeyboardLockWidget(WebContents* web_contents) { | ||
return static_cast<WebContentsImpl*>(web_contents)->GetKeyboardLockWidget(); | ||
} | ||
|
||
+RenderWidgetHost* GetMouseLockWidget(WebContents* web_contents) { | ||
+ return static_cast<WebContentsImpl*>(web_contents) | ||
+ ->mouse_lock_widget_for_testing(); | ||
+} | ||
+ | ||
bool RequestKeyboardLock(WebContents* web_contents, | ||
absl::optional<base::flat_set<ui::DomCode>> codes) { | ||
DCHECK(!codes.has_value() || !codes.value().empty()); | ||
diff --git a/content/public/test/browser_test_utils.h b/content/public/test/browser_test_utils.h | ||
index f6b2bc3b00cb457e070d92e102692fe2423fe3ed..c74c3336a64d58f2b1ca5c971ff4102776ee57b2 100644 | ||
--- a/content/public/test/browser_test_utils.h | ||
+++ b/content/public/test/browser_test_utils.h | ||
@@ -1062,6 +1062,9 @@ void UiaGetPropertyValueVtArrayVtUnknownValidate( | ||
// Returns the RenderWidgetHost that holds the keyboard lock. | ||
RenderWidgetHost* GetKeyboardLockWidget(WebContents* web_contents); | ||
|
||
+// Returns the RenderWidgetHost that holds the mouse lock. | ||
+RenderWidgetHost* GetMouseLockWidget(WebContents* web_contents); | ||
+ | ||
// Allows tests to drive keyboard lock functionality without requiring access | ||
// to the RenderWidgetHostImpl header or setting up an HTTP test server. | ||
// |codes| represents the set of keys to lock. If |codes| has no value, then |