0

Remove OnWidgetClosing from PermissionPromptImpl

OnWidgetClosing is not triggered if Close() is not explicitly called
and misses cases where the OS triggers widget closure.
OnWidgetDestroying should be used to ensure all call paths are covered.
(See bug for more details)

This CL removes OnWidgetClosing from PermissionPromptImpl and replaces
it with an asynchronous cleanup as well as cleanup during
OnWidgetDestroying for when the PermissionPromptBubbleView is destroyed
outside of the PermissionPromptImpl. This CL also adds a browsertest
to ensure that switching between PermissionChip and
PermissionPromptBubbleView does not cause any crashes after this
change which was a code path that was previously untested.

Bug: 1240365
Change-Id: I1381c6536962bef25ca89bccb794be6cf587bfe5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3651724
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Elaine Chien <elainechien@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004871}
This commit is contained in:
Elaine Chien
2022-05-18 18:26:45 +00:00
committed by Chromium LUCI CQ
parent 564fa57f10
commit 6daaaf82fe
3 changed files with 35 additions and 8 deletions

@ -6,6 +6,7 @@
#include "base/feature_list.h"
#include "base/run_loop.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
@ -256,6 +257,24 @@ IN_PROC_BROWSER_TEST_P(PermissionPromptBubbleViewBrowserTest,
}
}
// Test switching between PermissionChip and PermissionPromptBubbleView and make
// sure no crashes.
IN_PROC_BROWSER_TEST_P(PermissionPromptBubbleViewBrowserTest,
SwitchBetweenChipAndBubble) {
BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
browser_view->GetLocationBarView()->SetVisible(false);
ShowUi("geolocation");
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
permissions::PermissionRequestManager* permission_request_manager =
permissions::PermissionRequestManager::FromWebContents(web_contents);
permission_request_manager->UpdateAnchor();
browser_view->GetLocationBarView()->SetVisible(true);
permission_request_manager->UpdateAnchor();
browser_view->GetLocationBarView()->SetVisible(false);
permission_request_manager->UpdateAnchor();
}
// Test bubbles showing when tabs move between windows. Simulates a situation
// that could result in permission bubbles not being dismissed, and a problem
// referencing a temporary drag window. See http://crbug.com/754552.

@ -119,8 +119,7 @@ PermissionPromptImpl::~PermissionPromptImpl() {
switch (prompt_style_) {
case PermissionPromptStyle::kBubbleOnly:
DCHECK(!chip_);
if (prompt_bubble_)
prompt_bubble_->GetWidget()->Close();
CleanUpPromptBubble();
break;
case PermissionPromptStyle::kChip:
case PermissionPromptStyle::kQuietChip:
@ -162,14 +161,13 @@ void PermissionPromptImpl::UpdateAnchor() {
// Change prompt style to chip to avoid dismissing request while
// switching UI style.
prompt_bubble_->SetPromptStyle(PermissionPromptStyle::kChip);
prompt_bubble_->GetWidget()->Close();
CleanUpPromptBubble();
ShowChip();
chip_->OpenBubble();
} else {
// If |browser_| changed, recreate bubble for correct browser.
if (was_browser_changed) {
prompt_bubble_->GetWidget()->CloseWithReason(
views::Widget::ClosedReason::kUnspecified);
CleanUpPromptBubble();
ShowBubble();
} else {
prompt_bubble_->UpdateAnchorPosition();
@ -245,6 +243,15 @@ PermissionPromptImpl::GetPromptDisposition() const {
}
}
void PermissionPromptImpl::CleanUpPromptBubble() {
if (prompt_bubble_) {
views::Widget* widget = prompt_bubble_->GetWidget();
widget->RemoveObserver(this);
widget->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
prompt_bubble_ = nullptr;
}
}
views::Widget* PermissionPromptImpl::GetPromptBubbleWidgetForTesting() {
if (prompt_bubble_) {
return prompt_bubble_->GetWidget();
@ -253,8 +260,7 @@ views::Widget* PermissionPromptImpl::GetPromptBubbleWidgetForTesting() {
: nullptr;
}
void PermissionPromptImpl::OnWidgetClosing(views::Widget* widget) {
DCHECK_EQ(widget, prompt_bubble_->GetWidget());
void PermissionPromptImpl::OnWidgetDestroying(views::Widget* widget) {
widget->RemoveObserver(this);
prompt_bubble_ = nullptr;
}

@ -41,10 +41,12 @@ class PermissionPromptImpl : public permissions::PermissionPrompt,
permissions::PermissionPromptDisposition GetPromptDisposition()
const override;
void CleanUpPromptBubble();
views::Widget* GetPromptBubbleWidgetForTesting();
// views::WidgetObserver:
void OnWidgetClosing(views::Widget* widget) override;
void OnWidgetDestroying(views::Widget* widget) override;
private:
bool IsLocationBarDisplayed();