From e6143acc03189c5e52959545b110d6d17ecd5286 Mon Sep 17 00:00:00 2001 From: Andy Paicu Date: Mon, 13 May 2024 21:40:59 +0000 Subject: [PATCH] [PEPC] Close PEPC prompt on tab switch As per the DD the PEPC prompt should be closed when tab switching. This CL introduces a new TabSwitchingBehavior that closes the request as "Ignored" but also calls the "Cancelled" callback. This is needed to allow PEPC requests to notify the renderer of the prompt being closed in order to raise the "dismiss" event. Additionally during this CL a flaw was identified in PermissionElementBrowserTest which allowed all tests to falsely pass. |waitForResolveEvent| would immediately return true which made it appear that all conditions are met. This CL fixes that by using console logs to determine which events have been raised so far, and checking that they are the expected ones. Fixed: 339792166 Change-Id: I3671f0b425c54ed889fa24484ed0e09d1ecb6884 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5526465 Commit-Queue: Andy Paicu Reviewed-by: Thomas Nguyen Cr-Commit-Position: refs/heads/main@{#1300313} --- .../permission_element_browsertest.cc | 119 ++++++++++++------ .../permissions/embedded_permission_prompt.cc | 2 +- .../data/permissions/permission_element.html | 51 +++----- .../permissions/permission_request_manager.cc | 11 +- 4 files changed, 114 insertions(+), 69 deletions(-) diff --git a/chrome/browser/permissions/permission_element_browsertest.cc b/chrome/browser/permissions/permission_element_browsertest.cc index 3716d5c3834435..542e47d0c72fb7 100644 --- a/chrome/browser/permissions/permission_element_browsertest.cc +++ b/chrome/browser/permissions/permission_element_browsertest.cc @@ -2,8 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include +#include + #include "base/task/single_thread_task_runner.h" #include "base/test/scoped_feature_list.h" +#include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/views/permissions/embedded_permission_prompt_content_scrim_view.h" @@ -47,6 +51,8 @@ class PermissionElementBrowserTest : public InProcessBrowserTest { void SetUpOnMainThread() override { ASSERT_TRUE(embedded_test_server()->Start()); + console_observer_ = + std::make_unique(web_contents()); ASSERT_TRUE(ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( browser(), embedded_test_server()->GetURL("/permissions/permission_element.html"), @@ -54,68 +60,88 @@ class PermissionElementBrowserTest : public InProcessBrowserTest { } content::WebContents* web_contents() { - return browser()->tab_strip_model()->GetActiveWebContents(); + return browser()->tab_strip_model()->GetWebContentsAt(0); } void WaitForResolveEvent(const std::string& id) { - EXPECT_EQ(true, content::EvalJs( - web_contents(), - content::JsReplace("waitForResolveEvent($1)", id))); + ExpectConsoleMessage(id + "-resolve"); } void WaitForDismissEvent(const std::string& id) { - EXPECT_EQ(true, content::EvalJs( - web_contents(), - content::JsReplace("waitForDismissEvent($1)", id))); + ExpectConsoleMessage(id + "-dismiss"); + } + + void ExpectNoEvents() { EXPECT_EQ(0u, console_observer_->messages().size()); } + + void ExpectConsoleMessage(const std::string& expected_message, + std::optional + log_level = std::nullopt) { + EXPECT_TRUE(console_observer_->Wait()); + + EXPECT_EQ(1u, console_observer_->messages().size()); + EXPECT_EQ(expected_message, console_observer_->GetMessageAt(0)); + if (log_level) { + EXPECT_EQ(log_level.value(), console_observer_->messages()[0].log_level); + } + + // WebContentsConsoleObserver::Wait() will only wait until there is at least + // one message. We need to reset the |console_observer_| in order to be able + // to wait for the next message. + console_observer_ = + std::make_unique(web_contents()); + } + + void SkipInvalidElementMessage() { + ExpectConsoleMessage( + "The permission type 'invalid microphone' is not supported by the " + "permission element."); } private: base::test::ScopedFeatureList feature_list_; + std::unique_ptr console_observer_; }; IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, RequestInvalidPermissionType) { - content::WebContentsConsoleObserver console_observer(web_contents()); - ASSERT_TRUE(ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( - browser(), - embedded_test_server()->GetURL("/permissions/permission_element.html"), - 1)); - ASSERT_TRUE(console_observer.Wait()); - ASSERT_EQ(1u, console_observer.messages().size()); - EXPECT_EQ( + ExpectConsoleMessage( "The permission type 'invalid microphone' is not supported by the " "permission element.", - console_observer.GetMessageAt(0)); - EXPECT_EQ(blink::mojom::ConsoleMessageLevel::kError, - console_observer.messages()[0].log_level); + blink::mojom::ConsoleMessageLevel::kError); } IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, RequestPermissionDispatchResolveEvent) { - permissions::PermissionRequestManager::FromWebContents(web_contents()) - ->set_auto_response_for_test( - permissions::PermissionRequestManager::AutoResponseType::ACCEPT_ALL); - // TODO(crbug.com/40275129): add "camera-microphone" id, after we make sure - // embedded permission request will be routed to PermissionRequestManager - // regardless of the stored permission status. - std::string permission_ids[] = {"geolocation", "microphone", "camera"}; - for (const auto& id : permission_ids) { - permissions::PermissionRequestObserver observer(web_contents()); - ClickElementWithId(web_contents(), id); - observer.Wait(); - WaitForResolveEvent(id); + SkipInvalidElementMessage(); + + permissions::PermissionRequestManager::AutoResponseType responses[] = { + permissions::PermissionRequestManager::AutoResponseType::ACCEPT_ALL, + permissions::PermissionRequestManager::AutoResponseType::ACCEPT_ONCE, + permissions::PermissionRequestManager::AutoResponseType::DENY_ALL}; + + std::string permission_ids[] = {"geolocation", "microphone", "camera", + "camera-microphone"}; + + for (const auto& response : responses) { + permissions::PermissionRequestManager::FromWebContents(web_contents()) + ->set_auto_response_for_test(response); + for (const auto& id : permission_ids) { + permissions::PermissionRequestObserver observer(web_contents()); + ClickElementWithId(web_contents(), id); + observer.Wait(); + WaitForResolveEvent(id); + } } } IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, RequestPermissionDispatchDismissEvent) { + SkipInvalidElementMessage(); permissions::PermissionRequestManager::FromWebContents(web_contents()) ->set_auto_response_for_test( - permissions::PermissionRequestManager::AutoResponseType::DENY_ALL); - // TODO(crbug.com/40275129): add "camera-microphone" id, after we make sure - // embedded permission request will be routed to PermissionRequestManager - // regardless of the stored permission status. - std::string permission_ids[] = {"geolocation", "microphone", "camera"}; + permissions::PermissionRequestManager::AutoResponseType::DISMISS); + std::string permission_ids[] = {"geolocation", "microphone", "camera", + "camera-microphone"}; for (const auto& id : permission_ids) { permissions::PermissionRequestObserver observer(web_contents()); ClickElementWithId(web_contents(), id); @@ -126,10 +152,11 @@ IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, ClickingScrimViewDispatchDismissEvent) { + SkipInvalidElementMessage(); permissions::PermissionRequestManager::FromWebContents(web_contents()) ->set_auto_response_for_test( permissions::PermissionRequestManager::AutoResponseType::NONE); - std::string permission_ids[] = {"microphone", "camera"}; + std::string permission_ids[] = {"microphone", "camera", "camera-microphone"}; for (const auto& id : permission_ids) { views::NamedWidgetShownWaiter waiter( views::test::AnyWidgetTestPasskey{}, @@ -147,6 +174,26 @@ IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, } } +IN_PROC_BROWSER_TEST_F(PermissionElementBrowserTest, TabSwitchingClosesPrompt) { + SkipInvalidElementMessage(); + permissions::PermissionRequestManager::FromWebContents(web_contents()) + ->set_auto_response_for_test( + permissions::PermissionRequestManager::AutoResponseType::NONE); + + permissions::PermissionRequestObserver observer(web_contents()); + ClickElementWithId(web_contents(), "camera"); + observer.Wait(); + + std::unique_ptr new_tab = content::WebContents::Create( + content::WebContents::CreateParams(browser()->profile())); + browser()->tab_strip_model()->AppendWebContents(std::move(new_tab), + /*foreground*/ false); + + ExpectNoEvents(); + browser()->tab_strip_model()->ActivateTabAt(1); + WaitForDismissEvent("camera"); +} + class PermissionElementWithSecurityBrowserTest : public InProcessBrowserTest { public: PermissionElementWithSecurityBrowserTest() { diff --git a/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc b/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc index dd156f0382170f..9bec9377a19578 100644 --- a/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc +++ b/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc @@ -286,7 +286,7 @@ void EmbeddedPermissionPrompt::CloseCurrentViewAndMaybeShowNext( EmbeddedPermissionPrompt::TabSwitchingBehavior EmbeddedPermissionPrompt::GetTabSwitchingBehavior() { - return TabSwitchingBehavior::kDestroyPromptButKeepRequestPending; + return TabSwitchingBehavior::kDestroyPromptAndIgnoreRequest; } void EmbeddedPermissionPrompt::RecordOsMetrics( diff --git a/chrome/test/data/permissions/permission_element.html b/chrome/test/data/permissions/permission_element.html index 6f4d64f4d8e366..c9e38c9ec51557 100644 --- a/chrome/test/data/permissions/permission_element.html +++ b/chrome/test/data/permissions/permission_element.html @@ -1,41 +1,30 @@ - + diff --git a/components/permissions/permission_request_manager.cc b/components/permissions/permission_request_manager.cc index 277f5e9fc9ae77..e681a71af3603e 100644 --- a/components/permissions/permission_request_manager.cc +++ b/components/permissions/permission_request_manager.cc @@ -20,6 +20,7 @@ #include "base/task/sequenced_task_runner.h" #include "base/time/clock.h" #include "base/time/time.h" +#include "build/chromeos_buildflags.h" #include "components/back_forward_cache/back_forward_cache_disable.h" #include "components/permissions/constants.h" #include "components/permissions/features.h" @@ -542,7 +543,15 @@ void PermissionRequestManager::OnVisibilityChanged( break; case PermissionPrompt::TabSwitchingBehavior:: kDestroyPromptAndIgnoreRequest: - CurrentRequestsDecided(PermissionAction::IGNORED); +// Lacros has an issue with focus switching if a view is destroyed while the +// webcontents is losing visibility, therefore the Ignore() call gets delayed. +#if BUILDFLAG(IS_CHROMEOS_LACROS) + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, base::BindOnce(&PermissionRequestManager::Ignore, + weak_factory_.GetWeakPtr())); +#else // BUILDFLAG(IS_CHROMEOS_LACROS) + Ignore(); +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) break; case PermissionPrompt::TabSwitchingBehavior::kKeepPromptAlive: break;