Skip to content

Commit

Permalink
Restore a permission prompt on tab change.
Browse files Browse the repository at this point in the history 
This CL fixes a bug where we resolve a permission request if a user
opened a new tab and started editing the location bar view. In that case, when the user switched back to the first tab, the permission request will be automatically ignored.

Bug: 1379249,931657,1003747
Change-Id: I8cd9d573be9c9baba467f4be4bae3c41da103427
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3999327
Commit-Queue: Illia Klimov <elklm@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Ravjit Uppal <ravjit@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075251}
  • Loading branch information
Illia Klimov authored and Chromium LUCI CQ committed Nov 23, 2022
1 parent fc739ea commit 6eca156
Show file tree
Hide file tree
Showing 12 changed files with 232 additions and 54 deletions.
116 changes: 75 additions & 41 deletions chrome/browser/permissions/permission_request_manager_browsertest.cc
View file
Expand Up @@ -38,6 +38,7 @@
#include "components/permissions/request_type.h"
#include "components/permissions/test/mock_permission_prompt_factory.h"
#include "components/permissions/test/mock_permission_request.h"
#include "components/permissions/test/permission_request_observer.h"
#include "components/variations/variations_associated_data.h"
#include "content/public/browser/permission_controller.h"
#include "content/public/browser/render_frame_host.h"
Expand All @@ -60,7 +61,6 @@
#include "url/origin.h"

namespace {

const char* kPermissionsKillSwitchFieldStudy =
permissions::PermissionContextBase::kPermissionsKillSwitchFieldStudy;
const char* kPermissionsKillSwitchBlockedValue =
Expand Down Expand Up @@ -291,7 +291,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
EXPECT_EQ(2, bubble_factory()->TotalRequestCount());
}

// Requests before the load should not be bundled with a request after the load.
// Requests before the load should not be bundled with a request after the
// load.
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
RequestsBeforeAfterLoad) {
ASSERT_TRUE(embedded_test_server()->Start());
Expand All @@ -307,9 +308,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
EXPECT_EQ(1, bubble_factory()->TotalRequestCount());
}

// Navigating twice to the same URL should be equivalent to refresh. This means
// showing the bubbles twice.
// http://crbug.com/512849 flaky
// Navigating twice to the same URL should be equivalent to refresh. This
// means showing the bubbles twice. http://crbug.com/512849 flaky
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, DISABLED_NavTwice) {
ASSERT_TRUE(embedded_test_server()->Start());

Expand All @@ -329,9 +329,9 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, DISABLED_NavTwice) {
EXPECT_EQ(2, bubble_factory()->TotalRequestCount());
}

// Navigating twice to the same URL with a hash should be navigation within the
// page. This means the bubble is only shown once.
// http://crbug.com/512849 flaky
// Navigating twice to the same URL with a hash should be navigation within
// the page. This means the bubble is only shown once. http://crbug.com/512849
// flaky
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
DISABLED_NavTwiceWithHash) {
ASSERT_TRUE(embedded_test_server()->Start());
Expand Down Expand Up @@ -376,9 +376,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,

// Prompts are only shown for active tabs and (on Desktop) hidden on tab
// switching
// Flaky on bots crbug.com/1003747.
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
DISABLED_MultipleTabs) {
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, MultipleTabs) {
ASSERT_TRUE(embedded_test_server()->Start());

ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
Expand All @@ -399,10 +397,27 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
ASSERT_EQ(2, tab_strip_model->count());
ASSERT_EQ(1, tab_strip_model->active_index());

// Request geolocation in foreground tab, prompt should be shown.
ExecuteScriptAndGetValue(
tab_strip_model->GetWebContentsAt(1)->GetPrimaryMainFrame(),
"navigator.geolocation.getCurrentPosition(function(){});");
constexpr char kRequestNotifications[] = R"(
new Promise(resolve => {
Notification.requestPermission().then(function (permission) {
resolve(permission)
});
})
)";

{
permissions::PermissionRequestObserver observer(
tab_strip_model->GetWebContentsAt(1));

// Request permission in foreground tab, prompt should be shown.
EXPECT_TRUE(content::ExecJs(
tab_strip_model->GetWebContentsAt(1)->GetPrimaryMainFrame(),
kRequestNotifications,
content::EvalJsOptions::EXECUTE_SCRIPT_NO_RESOLVE_PROMISES));

observer.Wait();
}

EXPECT_EQ(1, bubble_factory_1->show_count());
EXPECT_FALSE(bubble_factory_0->is_visible());
EXPECT_TRUE(bubble_factory_1->is_visible());
Expand All @@ -416,11 +431,21 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
EXPECT_FALSE(bubble_factory_0->is_visible());
EXPECT_TRUE(bubble_factory_1->is_visible());

// Request notification in background tab. No prompt is shown until the tab
// itself is activated.
ExecuteScriptAndGetValue(
tab_strip_model->GetWebContentsAt(0)->GetPrimaryMainFrame(),
"Notification.requestPermission()");
{
permissions::PermissionRequestObserver observer(
tab_strip_model->GetWebContentsAt(0));

// Request notification in background tab. No prompt is shown until the
// tab itself is activated.
EXPECT_TRUE(content::ExecJs(
tab_strip_model->GetWebContentsAt(0)->GetPrimaryMainFrame(),
kRequestNotifications,
content::EvalJsOptions::EXECUTE_SCRIPT_NO_RESOLVE_PROMISES));

observer.Wait();
EXPECT_TRUE(observer.is_prompt_show_failed_hidden_tab());
}

EXPECT_FALSE(bubble_factory_0->is_visible());
EXPECT_EQ(2, bubble_factory_1->show_count());

Expand All @@ -430,36 +455,37 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
EXPECT_EQ(2, bubble_factory_1->show_count());
}

// Regularly timing out in Windows, Linux and macOS Debug Builds.
// https://crbug.com/931657
IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest,
DISABLED_BackgroundTabNavigation) {
BackgroundTabNavigation) {
ASSERT_TRUE(embedded_test_server()->Start());

ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
browser(), embedded_test_server()->GetURL("/empty.html"), 1);

// Request camera, prompt should be shown.
// Request Notifications, prompt should be shown.
ExecuteScriptAndGetValue(
browser()->tab_strip_model()->GetWebContentsAt(0)->GetPrimaryMainFrame(),
"navigator.getUserMedia({video: true}, ()=>{}, ()=>{})");
"Notification.requestPermission()");
bubble_factory()->WaitForPermissionBubble();
EXPECT_TRUE(bubble_factory()->is_visible());
EXPECT_EQ(1, bubble_factory()->show_count());

// SetUp() only creates a mock prompt factory for the first tab but this test
// doesn't request any permissions in the second tab so it doesn't need one.
// SetUp() only creates a mock prompt factory for the first tab but this
// test doesn't request any permissions in the second tab so it doesn't need
// one.
ui_test_utils::NavigateToURLWithDisposition(
browser(), embedded_test_server()->GetURL("/empty.html"),
WindowOpenDisposition::NEW_FOREGROUND_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);

// Navigate background tab, prompt should be removed.
content::TestNavigationObserver observer(
browser()->tab_strip_model()->GetWebContentsAt(0));

ExecuteScriptAndGetValue(
browser()->tab_strip_model()->GetWebContentsAt(0)->GetPrimaryMainFrame(),
"window.location = 'simple.html'");
content::TestNavigationObserver observer(
browser()->tab_strip_model()->GetWebContentsAt(0));

observer.Wait();
EXPECT_FALSE(bubble_factory()->is_visible());

Expand Down Expand Up @@ -742,6 +768,11 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest,
SetCannedUiDecision(QuietUiReason::kTriggeredDueToAbusiveContent,
WarningReason::kAbusiveContent);

ASSERT_TRUE(embedded_test_server()->Start());

ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
browser(), embedded_test_server()->GetURL("/empty.html"), 1);

auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
permissions::MockPermissionRequest request_quiet(
permissions::RequestType::kNotifications);
Expand All @@ -753,9 +784,12 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest,
auto disposition_from_prompt_bubble =
manager->view_for_testing()->GetPromptDisposition();

// There will be no instance of PermissionPromptImpl after a tab marked as
// HIDDEN.
manager->OnVisibilityChanged(content::Visibility::HIDDEN);
// There will be no instance of PermissionPromptImpl after a new tab is opened
// and existing tab marked as HIDDEN.
ui_test_utils::NavigateToURLWithDisposition(
browser(), embedded_test_server()->GetURL("/empty.html"),
WindowOpenDisposition::NEW_FOREGROUND_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);

absl::optional<permissions::PermissionPromptDisposition> disposition =
manager->current_request_prompt_disposition_for_testing();
Expand All @@ -764,7 +798,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest,
EXPECT_EQ(disposition.value(), disposition_from_prompt_bubble);

// DCHECK failure if Closing executed on HIDDEN PermissionRequestManager.
manager->OnVisibilityChanged(content::Visibility::VISIBLE);
browser()->tab_strip_model()->ActivateTabAt(0);
manager->Dismiss();
base::RunLoop().RunUntilIdle();
}
Expand Down Expand Up @@ -912,10 +946,10 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithBackForwardCacheBrowserTest,
content::RenderFrameHostWrapper rfh_b(GetActiveMainFrame());

// Mic, camera, and pan/tilt/zoom requests are grouped if they come from the
// same origin. Make sure this will not include requests from a cached frame.
// Note pages will not be cached when navigating within the same origin, so we
// have different urls in the navigations above but use the same (default) url
// for the MockPermissionRequest here.
// same origin. Make sure this will not include requests from a cached
// frame. Note pages will not be cached when navigating within the same
// origin, so we have different urls in the navigations above but use the
// same (default) url for the MockPermissionRequest here.
permissions::MockPermissionRequest req_a_1(
permissions::RequestType::kCameraPanTiltZoom,
permissions::PermissionRequestGestureType::GESTURE);
Expand Down Expand Up @@ -1098,8 +1132,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithPrerenderingTest,

base::RunLoop().RunUntilIdle();

// Permission request from main frame should be granted, similar request from
// prerender should be denied.
// Permission request from main frame should be granted, similar request
// from prerender should be denied.
EXPECT_TRUE(request_1.granted());
EXPECT_TRUE(request_2->cancelled());
EXPECT_TRUE(deleted_observer.deleted());
Expand Down Expand Up @@ -1183,8 +1217,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithFencedFrameTest,
})();
)";

// The result of query 'geolocation' permission in the fenced frame should be
// 'denied'.
// The result of query 'geolocation' permission in the fenced frame should
// be 'denied'.
EXPECT_EQ("denied", content::EvalJs(fenced_frame_host, kQueryPermission));

const char kQueryCurrentPosition[] = R"(
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/ui/views/location_bar/location_bar_view.cc
View file
Expand Up @@ -89,6 +89,7 @@
#include "components/omnibox/common/omnibox_features.h"
#include "components/performance_manager/public/features.h"
#include "components/permissions/features.h"
#include "components/permissions/permission_request_manager.h"
#include "components/prefs/pref_service.h"
#include "components/safe_browsing/core/common/features.h"
#include "components/search_engines/template_url.h"
Expand Down Expand Up @@ -831,6 +832,15 @@ void LocationBarView::Update(WebContents* contents) {
qr_generator_icon->SetVisible(false);

OnChanged(); // NOTE: Calls Layout().

// A permission prompt may be suspended due to an invalid state (empty or
// editing location bar). Restore the suspended prompt if possible.
if (contents && !IsEditingOrEmpty()) {
auto* permission_request_manager =
permissions::PermissionRequestManager::FromWebContents(contents);
if (permission_request_manager->CanRestorePrompt())
permission_request_manager->RestorePrompt();
}
}

void LocationBarView::ResetTabState(WebContents* contents) {
Expand Down
16 changes: 12 additions & 4 deletions chrome/browser/ui/views/permissions/permission_prompt_factory.cc
View file
Expand Up @@ -16,6 +16,7 @@
#include "chrome/common/webui_url_constants.h"
#include "components/permissions/features.h"
#include "components/permissions/permission_request.h"
#include "components/permissions/permission_uma_util.h"
#include "components/permissions/request_type.h"
#include "content/public/browser/web_contents.h"

Expand Down Expand Up @@ -46,8 +47,10 @@ LocationBarView* GetLocationBarView(Browser* browser) {
// A permission request should be auto-ignored if a user interacts with the
// LocationBar. The only exception is the NTP page where the user needs to press
// on a microphone icon to get a permission request.
bool ShouldIgnorePermissionRequest(content::WebContents* web_contents,
Browser* browser) {
bool ShouldIgnorePermissionRequest(
content::WebContents* web_contents,
Browser* browser,
permissions::PermissionPrompt::Delegate* delegate) {
DCHECK(web_contents);
DCHECK(browser);

Expand All @@ -58,7 +61,12 @@ bool ShouldIgnorePermissionRequest(content::WebContents* web_contents,
}

LocationBarView* location_bar = GetLocationBarView(browser);
return location_bar && location_bar->IsEditingOrEmpty();
bool can_display_prompt = location_bar && location_bar->IsEditingOrEmpty();

permissions::PermissionUmaUtil::RecordPermissionPromptAttempt(
delegate->Requests(), can_display_prompt);

return can_display_prompt;
}

bool ShouldUseChip(permissions::PermissionPrompt::Delegate* delegate) {
Expand Down Expand Up @@ -168,7 +176,7 @@ std::unique_ptr<permissions::PermissionPrompt> CreatePermissionPrompt(
}

// Auto-ignore the permission request if a user is typing into location bar.
if (ShouldIgnorePermissionRequest(web_contents, browser)) {
if (ShouldIgnorePermissionRequest(web_contents, browser, delegate)) {
return nullptr;
}

Expand Down
12 changes: 10 additions & 2 deletions chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc
View file
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/test/base/ui_test_utils.h"
#include "chrome/test/permissions/permission_request_manager_test_api.h"
#include "components/metrics/content/subprocess_metrics_provider.h"
#include "components/omnibox/browser/omnibox_edit_model.h"
#include "components/omnibox/browser/omnibox_view.h"
#include "components/omnibox/browser/open_tab_provider.h"
Expand Down Expand Up @@ -133,6 +134,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest,
IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest,
PermissionRequestIsAutoIgnored) {
ASSERT_TRUE(embedded_test_server()->Start());
base::HistogramTester histograms;

content::WebContents* embedder_contents =
browser()->tab_strip_model()->GetActiveWebContents();
Expand Down Expand Up @@ -193,13 +195,19 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest,
// Wait until a permission request is shown or finalized.
observer.Wait();

// Permission request was finalized without showing a prompt bubble.
EXPECT_FALSE(manager->IsRequestInProgress());
// Permission request was is in progress without showing a prompt bubble.
EXPECT_TRUE(manager->IsRequestInProgress());
EXPECT_FALSE(observer.request_shown());
EXPECT_TRUE(observer.is_view_recreate_failed());
EXPECT_FALSE(manager->view_for_testing());

EXPECT_FALSE(content::EvalJs(main_rfh, kCheckMicrophone,
content::EXECUTE_SCRIPT_DEFAULT_OPTIONS, 1)
.value.GetBool());

metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
histograms.ExpectBucketCount(
"Permissions.Prompt.AudioCapture.Gesture.Attempt", true, 1);
}

IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest,
Expand Down

0 comments on commit 6eca156

Please sign in to comment.