Skip to content

Commit

Permalink
Simplify FedCmAccountSelectionView (part 12/N)
Browse files Browse the repository at this point in the history 
This CL switches FedCm to use CLIENT_OWNS_WIDGET along with synchronous
destruction. This fixes production bugs related to the widget being
asynchronously destroyed after the FedCmAccountSelectionView is
destroyed.

Bug: 377803489
Change-Id: Ia2780da849086bc5b9d675c25bca7c61eae87431
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043775
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1388489}
  • Loading branch information
@erikchen @pull
erikchen authored and pull[bot] committed Dec 3, 2024
1 parent 3510e03 commit aea6ebd
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 92 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ui/views/webid/account_selection_modal_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ AccountSelectionModalView::AccountSelectionModalView(
rp_for_display) {
SetModalType(ui::mojom::ModalType::kChild);
SetOwnedByWidget(true);
SetOwnershipOfNewWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
set_fixed_width(kDialogWidth);
SetShowTitle(false);
SetShowCloseButton(false);
Expand Down
111 changes: 46 additions & 65 deletions chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h"

#include "base/auto_reset.h"
#include "base/debug/dump_without_crashing.h"
#include "base/functional/bind.h"
#include "base/metrics/histogram_functions.h"
Expand Down Expand Up @@ -207,7 +208,7 @@ bool FedCmAccountSelectionView::Show(
// and other parts of the header.
if ((rp_mode == blink::mojom::RpMode::kPassive && idp_list_.size() > 1) ||
(rp_mode == blink::mojom::RpMode::kActive && !has_modal_support)) {
MaybeResetAccountSelectionView();
CloseWidgetNoNotify();
}

bool create_view = !account_selection_view_;
Expand Down Expand Up @@ -400,7 +401,7 @@ bool FedCmAccountSelectionView::ShowFailureDialog(
// title and other parts of the header.
if ((rp_mode == blink::mojom::RpMode::kPassive && idp_list_.size() > 1) ||
(rp_mode == blink::mojom::RpMode::kActive && !has_modal_support)) {
MaybeResetAccountSelectionView();
CloseWidgetNoNotify();
}

bool create_view = !account_selection_view_;
Expand Down Expand Up @@ -457,7 +458,7 @@ bool FedCmAccountSelectionView::ShowErrorDialog(
// and other parts of the header.
if ((rp_mode == blink::mojom::RpMode::kPassive && idp_list_.size() > 1) ||
(rp_mode == blink::mojom::RpMode::kActive && !has_modal_support)) {
MaybeResetAccountSelectionView();
CloseWidgetNoNotify();
}

bool create_view = !account_selection_view_;
Expand Down Expand Up @@ -592,15 +593,6 @@ void FedCmAccountSelectionView::CreateAccountSelectionView(
CreateDialogWidget();
}

void FedCmAccountSelectionView::OnWidgetDestroying(views::Widget* widget) {
DismissReason dismiss_reason =
(GetDialogWidget()->closed_reason() ==
views::Widget::ClosedReason::kCloseButtonClicked)
? DismissReason::kCloseButton
: DismissReason::kOther;
OnDismiss(dismiss_reason);
}

void FedCmAccountSelectionView::OnAccountsDisplayed() {
delegate_->OnAccountsDisplayed();
}
Expand Down Expand Up @@ -726,8 +718,7 @@ void FedCmAccountSelectionView::OnCloseButtonClicked(const ui::Event& event) {
// pop-up. However if the user clicks cancel, we should dismiss so we should
// set this back to true.
notify_delegate_of_dismiss_ = true;
GetDialogWidget()->CloseWithReason(
views::Widget::ClosedReason::kCloseButtonClicked);
CloseWidget(views::Widget::ClosedReason::kCloseButtonClicked);
}

void FedCmAccountSelectionView::OnLoginToIdP(const GURL& idp_config_url,
Expand Down Expand Up @@ -1024,39 +1015,8 @@ void FedCmAccountSelectionView::Close() {
return;
}

pip_occlusion_observation_.reset();

// This prevents re-entrancy via OnWidgetDestroying().
GetDialogWidget()->RemoveObserver(this);

LogDialogDismissal(DismissReason::kOther);

// Implicitly owned by the dialog widget. Must clear to avoid UaF.
account_selection_view_ = nullptr;
scoped_ignore_input_events_.reset();
input_protector_.reset();

// This synchronously destroys the widget.
dialog_widget_->CloseNow();

if (notify_delegate_of_dismiss_) {
delegate_->OnDismiss(DismissReason::kOther);
}
}

// TODO(https://crbug.com/377803489): Delete this method.
void FedCmAccountSelectionView::OnDismiss(DismissReason dismiss_reason) {
if (!GetDialogWidget()) {
return;
}

LogDialogDismissal(dismiss_reason);
MaybeResetAccountSelectionView();
input_protector_.reset();

if (notify_delegate_of_dismiss_) {
delegate_->OnDismiss(dismiss_reason);
}
// The widget is synchronously destroyed.
CloseWidget(views::Widget::ClosedReason::kUnspecified);
}

views::Widget* FedCmAccountSelectionView::GetDialogWidget() {
Expand All @@ -1066,11 +1026,12 @@ views::Widget* FedCmAccountSelectionView::GetDialogWidget() {
void FedCmAccountSelectionView::CreateDialogWidget() {
CHECK(!dialog_widget_);
CHECK(tab_);
views::Widget* widget = nullptr;
if (dialog_type_ == DialogType::BUBBLE) {
auto* bubble =
static_cast<AccountSelectionBubbleView*>(account_selection_view_);
widget = views::BubbleDialogDelegateView::CreateBubble(bubble);
dialog_widget_ =
base::WrapUnique(views::BubbleDialogDelegateView::CreateBubble(
bubble, views::Widget::InitParams::CLIENT_OWNS_WIDGET));
} else {
// Create and show the dialog widget. This is functionally a tab-modal
// dialog.
Expand All @@ -1080,22 +1041,23 @@ void FedCmAccountSelectionView::CreateDialogWidget() {
tab_->GetContents()->GetTopLevelNativeWindow();
views::Widget* top_level_widget =
views::Widget::GetWidgetForNativeWindow(top_level_native_window);
widget = views::DialogDelegate::CreateDialogWidget(
dialog_widget_ = base::WrapUnique(views::DialogDelegate::CreateDialogWidget(
modal, /*context=*/nullptr,
/*parent=*/top_level_widget->GetNativeView());
/*parent=*/top_level_widget->GetNativeView()));

widget->Show();
dialog_widget_->Show();
scoped_ignore_input_events_ =
tab_->GetContents()->IgnoreInputEvents(std::nullopt);
}

extensions::SecurityDialogTracker::GetInstance()->AddSecurityDialog(widget);
dialog_widget_ = widget->GetWeakPtr();
dialog_widget_->MakeCloseSynchronous(base::BindOnce(
&FedCmAccountSelectionView::CloseWidget, base::Unretained(this)));
extensions::SecurityDialogTracker::GetInstance()->AddSecurityDialog(
dialog_widget_.get());
UpdateDialogPosition();
widget->AddObserver(this);
pip_occlusion_observation_ =
std::make_unique<ScopedPictureInPictureOcclusionObservation>(this);
pip_occlusion_observation_->Observe(widget);
pip_occlusion_observation_->Observe(dialog_widget_.get());
}

FedCmAccountSelectionView::DialogType
Expand All @@ -1113,18 +1075,14 @@ views::View* FedCmAccountSelectionView::GetAnchorView() {
return tab_->GetBrowserWindowInterface()->GetWebView();
}

void FedCmAccountSelectionView::MaybeResetAccountSelectionView() {
void FedCmAccountSelectionView::CloseWidgetNoNotify() {
if (!account_selection_view_) {
return;
}
if (GetDialogWidget()) {
GetDialogWidget()->RemoveObserver(this);
GetDialogWidget()->CloseWithReason(
views::Widget::ClosedReason::kCancelButtonClicked);
scoped_ignore_input_events_.reset();
dialog_widget_.reset();
}
account_selection_view_ = nullptr;

// Never notify the delegate when this method is called.
base::AutoReset<bool> resetter(&notify_delegate_of_dismiss_, false);
CloseWidget(views::Widget::ClosedReason::kCancelButtonClicked);
}

bool FedCmAccountSelectionView::IsIdpSigninPopupOpen() {
Expand Down Expand Up @@ -1279,3 +1237,26 @@ void FedCmAccountSelectionView::LogDialogDismissal(
}
}
}

void FedCmAccountSelectionView::CloseWidget(
views::Widget::ClosedReason reason) {
DismissReason dismiss_reason =
reason == views::Widget::ClosedReason::kCloseButtonClicked
? DismissReason::kCloseButton
: DismissReason::kOther;
LogDialogDismissal(dismiss_reason);
input_protector_.reset();

pip_occlusion_observation_.reset();

// Implicitly owned by the dialog widget. Must clear to avoid UaF.
account_selection_view_ = nullptr;
scoped_ignore_input_events_.reset();
dialog_widget_.reset();

// This delegate call can result in synchronous destruction of `this`. Avoid
// referencing any members after this call.
if (notify_delegate_of_dismiss_) {
delegate_->OnDismiss(dismiss_reason);
}
}
30 changes: 14 additions & 16 deletions chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents_observer.h"
#include "ui/views/input_event_activation_protector.h"
#include "ui/views/widget/widget_observer.h"

using IdentityProviderDataPtr = scoped_refptr<content::IdentityProviderData>;
using IdentityRequestAccountPtr =
Expand Down Expand Up @@ -46,8 +45,7 @@ class TabInterface;
class FedCmAccountSelectionView : public AccountSelectionView,
public FedCmModalDialogView::Observer,
content::WebContentsObserver,
public PictureInPictureOcclusionObserver,
public views::WidgetObserver {
public PictureInPictureOcclusionObserver {
public:
// safe_zone_diameter/icon_size as defined in
// https://www.w3.org/TR/appmanifest/#icon-masks
Expand Down Expand Up @@ -199,6 +197,12 @@ class FedCmAccountSelectionView : public AccountSelectionView,
void WillDetach(tabs::TabInterface* tab,
tabs::TabInterface::DetachReason reason);

// Synchronously closes dialog_widget_.
// This method can result in synchronous destruction of `this`.
// Public for testing.
// TODO(https://crbug.com/377803489): Make private again.
void CloseWidget(views::Widget::ClosedReason reason);

protected:
friend class FedCmAccountSelectionViewBrowserTest;

Expand All @@ -223,10 +227,12 @@ class FedCmAccountSelectionView : public AccountSelectionView,
// Widget to control the dialog i.e. hide, show, add observer etc.
// TODO(https://crbug.com/377803489): Make private again.
// Protected for testing.
base::WeakPtr<views::Widget> dialog_widget_;
std::unique_ptr<views::Widget> dialog_widget_;

// This view controls the contents of the dialog_widget_. Conceptually there
// should be a view if and only if there is a widget.
// is a view if and only if there is a widget. The two are constructed
// together and destroyed together. `dialog_widget_` owns
// `account_selection_view_`.
// Protected for testing.
raw_ptr<AccountSelectionViewBase> account_selection_view_;

Expand Down Expand Up @@ -352,9 +358,6 @@ class FedCmAccountSelectionView : public AccountSelectionView,
kMaxValue = kTapScrim
};

// views::WidgetObserver:
void OnWidgetDestroying(views::Widget* widget) override;

// Called when the tab's WebContents is discarded.
void WillDiscardContents(tabs::TabInterface* tab,
content::WebContents* old_contents,
Expand All @@ -375,14 +378,9 @@ class FedCmAccountSelectionView : public AccountSelectionView,
// Returns the SheetType to be used for metrics reporting.
AccountSelectionView::SheetType GetSheetType();

// Notify the delegate that the widget was closed with reason
// `dismiss_reason`.
void OnDismiss(
content::IdentityRequestDialogController::DismissReason dismiss_reason);

// Resets `account_selection_view_`. Typically, to recreate it later to show a
// different kind of dialog. Virtual for testing purposes.
virtual void MaybeResetAccountSelectionView();
// Closes widget if it exists and never notifies.
// Virtual for testing purposes.
virtual void CloseWidgetNoNotify();

// Returns whether an IDP sign-in pop-up window is currently open.
bool IsIdpSigninPopupOpen();
Expand Down
20 changes: 9 additions & 11 deletions chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ class TestFedCmAccountSelectionView : public FedCmAccountSelectionView {
blink::mojom::RpContext GetRpContext() { return rp_context_; }
size_t num_dialogs_{0u};

MOCK_METHOD(void, MaybeResetAccountSelectionView, (), (override));
MOCK_METHOD(void, CloseWidgetNoNotify, (), (override));
bool can_fit_in_web_contents_{true};
bool dialog_position_updated_{false};
TestAccountSelectionView* GetTestView() {
Expand Down Expand Up @@ -535,12 +535,12 @@ namespace {
void TestFedCmAccountSelectionView::CreateDialogWidget() {
CHECK(!dialog_widget_);
views::Widget::InitParams params =
test_->CreateParams(views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET,
test_->CreateParams(views::Widget::InitParams::CLIENT_OWNS_WIDGET,
views::Widget::InitParams::TYPE_WINDOW_FRAMELESS);
params.delegate = GetTestView();
dialog_widget_ =
test_->CreateTestWidget(std::move(params)).release()->GetWeakPtr();
dialog_widget_->AddObserver(this);
dialog_widget_ = test_->CreateTestWidget(std::move(params));
dialog_widget_->MakeCloseSynchronous(base::BindOnce(
&FedCmAccountSelectionView::CloseWidget, base::Unretained(this)));
}

} // namespace
Expand Down Expand Up @@ -799,7 +799,6 @@ TEST_F(FedCmAccountSelectionViewDesktopTest,
// Emulate user clicking the close icon.
controller->GetDialogWidget()->CloseWithReason(
views::Widget::ClosedReason::kCloseButtonClicked);
controller->OnWidgetDestroying(controller->GetDialogWidget());

histogram_tester_->ExpectUniqueSample(
"Blink.FedCm.IdpSigninStatus.MismatchDialogResult",
Expand All @@ -820,7 +819,6 @@ TEST_F(FedCmAccountSelectionViewDesktopTest,
// Emulate user closing the mismatch dialog for an unspecified reason.
controller->GetDialogWidget()->CloseWithReason(
views::Widget::ClosedReason::kUnspecified);
controller->OnWidgetDestroying(controller->GetDialogWidget());

histogram_tester_->ExpectUniqueSample(
"Blink.FedCm.IdpSigninStatus.MismatchDialogResult",
Expand Down Expand Up @@ -1503,7 +1501,7 @@ TEST_F(FedCmAccountSelectionViewDesktopTest, UseAnotherAccountThenCancel) {

// Emulate the user clicking "cancel" button. This should close the widget.
controller->OnCloseButtonClicked(CreateMouseEvent());
EXPECT_TRUE(controller->GetDialogWidget()->IsClosed());
EXPECT_FALSE(controller->GetDialogWidget());
}

// Tests that the error dialog can be shown.
Expand Down Expand Up @@ -1942,7 +1940,7 @@ TEST_F(FedCmAccountSelectionViewDesktopTest,
std::unique_ptr<TestFedCmAccountSelectionView> controller =
CreateAndShowLoadingDialog();

EXPECT_CALL(*controller, MaybeResetAccountSelectionView).Times(0);
EXPECT_CALL(*controller, CloseWidgetNoNotify).Times(0);
accounts_ = {
CreateAccount(idp_data_, LoginState::kSignIn, LoginState::kSignIn)};
Show(*controller, accounts_, SignInMode::kAuto,
Expand Down Expand Up @@ -2228,7 +2226,7 @@ TEST_F(FedCmAccountSelectionViewDesktopTest,
std::unique_ptr<TestFedCmAccountSelectionView> controller = CreateAndShow(
accounts_, SignInMode::kExplicit, blink::mojom::RpMode::kActive);

EXPECT_CALL(*controller, MaybeResetAccountSelectionView).Times(0);
EXPECT_CALL(*controller, CloseWidgetNoNotify).Times(0);
controller->ShowErrorDialog(
kTopFrameEtldPlusOne, kIdpEtldPlusOne, blink::mojom::RpContext::kSignIn,
blink::mojom::RpMode::kActive, content::IdentityProviderMetadata(),
Expand All @@ -2243,7 +2241,7 @@ TEST_F(FedCmAccountSelectionViewDesktopTest,
std::unique_ptr<TestFedCmAccountSelectionView> controller =
CreateAndShowLoadingDialog();

EXPECT_CALL(*controller, MaybeResetAccountSelectionView).Times(0);
EXPECT_CALL(*controller, CloseWidgetNoNotify).Times(0);
Show(*controller, accounts_, SignInMode::kExplicit,
blink::mojom::RpMode::kActive);
}
Expand Down

0 comments on commit aea6ebd

Please sign in to comment.