Skip to content

Commit

Permalink
[PEPC] Close PEPC prompt on tab switch
Browse files Browse the repository at this point in the history
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 <andypaicu@chromium.org>
Reviewed-by: Thomas Nguyen <tungnh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1300313}
  • Loading branch information
andypaicu authored and Chromium LUCI CQ committed May 13, 2024
1 parent e81c63b commit e6143ac
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 69 deletions.
119 changes: 83 additions & 36 deletions chrome/browser/permissions/permission_element_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <memory>
#include <optional>

#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"
Expand Down Expand Up @@ -47,75 +51,97 @@ class PermissionElementBrowserTest : public InProcessBrowserTest {

void SetUpOnMainThread() override {
ASSERT_TRUE(embedded_test_server()->Start());
console_observer_ =
std::make_unique<content::WebContentsConsoleObserver>(web_contents());
ASSERT_TRUE(ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
browser(),
embedded_test_server()->GetURL("/permissions/permission_element.html"),
1));
}

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<blink::mojom::ConsoleMessageLevel>
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<content::WebContentsConsoleObserver>(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<content::WebContentsConsoleObserver> 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);
Expand All @@ -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{},
Expand All @@ -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<content::WebContents> 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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ void EmbeddedPermissionPrompt::CloseCurrentViewAndMaybeShowNext(

EmbeddedPermissionPrompt::TabSwitchingBehavior
EmbeddedPermissionPrompt::GetTabSwitchingBehavior() {
return TabSwitchingBehavior::kDestroyPromptButKeepRequestPending;
return TabSwitchingBehavior::kDestroyPromptAndIgnoreRequest;
}

void EmbeddedPermissionPrompt::RecordOsMetrics(
Expand Down
51 changes: 20 additions & 31 deletions chrome/test/data/permissions/permission_element.html
Original file line number Diff line number Diff line change
@@ -1,41 +1,30 @@
<!DOCTYPE html>
<html>
<script>
function waitForDismissEvent(id) {
(async () => {
await new Promise(resolve => {
document.getElementById(id).ondismiss = function (event) {
resolve();
};
});
})();
return true;
}

function waitForResolveEvent(id) {
(async () => {
await new Promise(resolve => {
document.getElementById(id).onresolve = function (event) {
resolve();
};
});
})();
return true;
}

function clickById(element_id) {
click(document.getElementById(element_id));
}

function click(element) {
element.click();
}
</script>
<body>
<permission id="geolocation" type="geolocation">
<permission id="microphone" type="microphone">
<permission id="camera" type="camera">
<permission id="invalid" type="invalid microphone">
<permission id="camera-microphone" type="camera microphone">
<script>
document.querySelectorAll('permission').forEach(permissionElement => {
// Console logs are used to inform the browser test which events
// have been seen.
permissionElement.addEventListener('resolve', (event) => {
console.log(event.target.id + '-resolve');
});
permissionElement.addEventListener('dismiss', (event) => {
console.log(event.target.id + '-dismiss');
});
});

function clickById(element_id) {
click(document.getElementById(element_id));
}

function click(element) {
element.click();
}
</script>
</body>
</html>
11 changes: 10 additions & 1 deletion components/permissions/permission_request_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit e6143ac

Please sign in to comment.