From 830aeea0f06cb49499c00c0d27e74d17817d0264 Mon Sep 17 00:00:00 2001
From: Ryan Qin <ryqin@google.com>
Date: Wed, 09 Sep 2026 11:22:47 -0700
Subject: [PATCH] [FedCM] Guard against reentrancy UAF in modal dialog and desktop view

FedCmModalDialogView::ShowPopupWindow invokes OpenURLFromTab via the
WebContentsDelegate, which can synchronously destroy the modal dialog
view. Calling member functions or writing to member variables after
OpenURLFromTab results in a Use-After-Free. Additionally,
ResizeAndFocusPopupWindow() can also trigger re-entrant deletion of the
view. This CL adds:
- A null check for source_window_->GetDelegate() in ShowPopupWindow().
- base::WeakPtr checks after OpenURLFromTab() and after
  ResizeAndFocusPopupWindow() before continuing in ShowPopupWindow().
- Regression unit tests covering both deletion points.

In FedCmAccountSelectionView, several delegate notification methods can
synchronously destroy the view:
- Introduces a [[nodiscard]] NotifyDelegate() helper that guards
  delegate calls with a base::WeakPtr.
- Uses NotifyDelegate() across all delegate notification call sites
  (OnMoreDetails, OnAccountsDisplayed, OnLoginToIdP, OnGotIt).
- Replaces the existing NotifyDelegateOfAccountSelection() helper with
  direct calls to NotifyDelegate() and early returns upon deletion.

Fixed: 557323166
Change-Id: Ie3763e9fd84a3060b6a998d06c4fc0bb119f9f5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8373422
Commit-Queue: Ryan Qin <ryqin@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1694753}
---
diff --git a/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.cc b/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.cc
index 79893f5..c0d89c9 100644
--- a/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.cc
+++ b/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.cc
@@ -173,8 +173,15 @@
         l10n_util::GetStringUTF16(IDS_FEDERATION_SIGNING_IN_TITLE));
     controller->ShowSuggestionChip(kActionFederation);
     controller->Show(kActionFederation);
-    NotifyDelegateOfAccountSelection(*accounts_[0],
-                                     *accounts_[0]->identity_provider);
+    if (!NotifyDelegate([&]() {
+          delegate_->OnAccountSelected(
+              accounts_[0]->identity_provider->idp_metadata.config_url,
+              accounts_[0]->id,
+              accounts_[0]->idp_claimed_login_state.value_or(
+                  accounts_[0]->browser_trusted_login_state));
+        })) {
+      return;
+    }
   } else {
     // For sign-up users, we show a full modal dialog that gathers the necessary
     // permission from the user (e.g. privacy policies and terms of services).
@@ -344,8 +351,12 @@
 
       if (should_show_verifying_sheet) {
         state_ = State::VERIFYING;
-        if (!NotifyDelegateOfAccountSelection(*new_accounts_[0],
-                                              new_idp_data)) {
+        if (!NotifyDelegate([&]() {
+              delegate_->OnAccountSelected(
+                  new_idp_data.idp_metadata.config_url, new_accounts_[0]->id,
+                  new_accounts_[0]->idp_claimed_login_state.value_or(
+                      new_accounts_[0]->browser_trusted_login_state));
+            })) {
           // `this` has been deleted.
           return false;
         }
@@ -667,7 +678,9 @@
 }
 
 void FedCmAccountSelectionView::OnAccountsDisplayed() {
-  delegate_->OnAccountsDisplayed();
+  if (!NotifyDelegate([&]() { delegate_->OnAccountsDisplayed(); })) {
+    return;
+  }
 }
 
 bool FedCmAccountSelectionView::OnAccountSelected(
@@ -698,7 +711,12 @@
       (state_ == State::SINGLE_ACCOUNT_PICKER &&
        dialog_type_ == DialogType::BUBBLE)) {
     state_ = State::VERIFYING;
-    if (!NotifyDelegateOfAccountSelection(*account, idp_data)) {
+    if (!NotifyDelegate([&]() {
+          delegate_->OnAccountSelected(
+              idp_data.idp_metadata.config_url, account->id,
+              account->idp_claimed_login_state.value_or(
+                  account->browser_trusted_login_state));
+        })) {
       // `this` was deleted.
       return false;
     }
@@ -825,7 +843,10 @@
         webid::AccountChooserResult::kUseOtherAccountButton;
   }
 
-  delegate_->OnLoginToIdP(idp_config_url, idp_login_url);
+  if (!NotifyDelegate(
+          [&]() { delegate_->OnLoginToIdP(idp_config_url, idp_login_url); })) {
+    return;
+  }
 }
 
 void FedCmAccountSelectionView::OnGotIt(const ui::Event& event) {
@@ -835,7 +856,10 @@
     return;
   }
 
-  delegate_->OnDismiss(DismissReason::kGotItButton);
+  if (!NotifyDelegate(
+          [&]() { delegate_->OnDismiss(DismissReason::kGotItButton); })) {
+    return;
+  }
 }
 
 void FedCmAccountSelectionView::OnMoreDetails(const ui::Event& event) {
@@ -845,8 +869,13 @@
     return;
   }
 
-  delegate_->OnMoreDetails();
-  delegate_->OnDismiss(DismissReason::kMoreDetailsButton);
+  if (!NotifyDelegate([&]() { delegate_->OnMoreDetails(); })) {
+    return;
+  }
+  if (!NotifyDelegate(
+          [&]() { delegate_->OnDismiss(DismissReason::kMoreDetailsButton); })) {
+    return;
+  }
 }
 
 content::WebContents* FedCmAccountSelectionView::ShowModalDialog(
@@ -1040,20 +1069,12 @@
   Close(/*notify_delegate=*/true, /*hide_widget=*/false);
 }
 
-bool FedCmAccountSelectionView::NotifyDelegateOfAccountSelection(
-    const Account& account,
-    const content::IdentityProviderData& idp_data) {
-  DCHECK(state_ == State::VERIFYING || state_ == State::AUTO_REAUTHN);
-
-  base::WeakPtr<FedCmAccountSelectionView> weak_ptr(
-      weak_ptr_factory_.GetWeakPtr());
-  delegate_->OnAccountSelected(idp_data.idp_metadata.config_url, account.id,
-                               account.idp_claimed_login_state.value_or(
-                                   account.browser_trusted_login_state));
-
-  // AccountSelectionView::Delegate::OnAccountSelected() might delete this.
-  // See https://crbug.com/40248291 for details.
-  return static_cast<bool>(weak_ptr);
+bool FedCmAccountSelectionView::NotifyDelegate(
+    base::FunctionRef<void()> notify_callback) {
+  base::WeakPtr<FedCmAccountSelectionView> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
+  notify_callback();
+  return weak_this != nullptr;
 }
 
 void FedCmAccountSelectionView::ShowVerifyingSheet(
diff --git a/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h b/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h
index feca0a4e..2d65b55c 100644
--- a/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h
+++ b/chrome/browser/ui/views/webid/fedcm_account_selection_view_desktop.h
@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/functional/function_ref.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -398,11 +399,10 @@
   // the background, becomes foregrounded.
   void BackgroundTaskTabForegrounded(tabs::TabInterface* tab);
 
+  // Calls a delegate method while guarding against `this` being deleted.
   // Returns false if `this` got deleted. In that case, the caller must early
   // return.
-  bool NotifyDelegateOfAccountSelection(
-      const Account& account,
-      const content::IdentityProviderData& idp_data);
+  [[nodiscard]] bool NotifyDelegate(base::FunctionRef<void()> notify_callback);
 
   // Shows the verifying sheet.
   void ShowVerifyingSheet(const IdentityRequestAccountPtr& account);
diff --git a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view.cc b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view.cc
index 33521fa6..f0e0b40a 100644
--- a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view.cc
+++ b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view.cc
@@ -65,10 +65,12 @@
   // the latter only tracks tab fullscreen (e.g., a video playing in
   // fullscreen), not browser fullscreen (e.g., pressing F11). `GetDisplayMode`
   // returns `kFullscreen` in both cases.
-  bool is_fullscreen =
-      source_window_->GetDelegate() &&
-      source_window_->GetDelegate()->GetDisplayMode(source_window_) ==
-          blink::mojom::DisplayMode::kFullscreen;
+  content::WebContentsDelegate* delegate = source_window_->GetDelegate();
+  if (!delegate) {
+    return nullptr;
+  }
+  bool is_fullscreen = delegate->GetDisplayMode(source_window_) ==
+                       blink::mojom::DisplayMode::kFullscreen;
   WindowOpenDisposition disposition =
       is_fullscreen ? WindowOpenDisposition::NEW_FOREGROUND_TAB
                     : WindowOpenDisposition::NEW_POPUP;
@@ -76,9 +78,16 @@
   content::OpenURLParams params(url, content::Referrer(), disposition,
                                 ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                 /*is_renderer_initiated=*/false);
-  popup_window_ = source_window_->GetDelegate()->OpenURLFromTab(
+  base::WeakPtr<FedCmModalDialogView> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
+  content::WebContents* popup_window = delegate->OpenURLFromTab(
       source_window_, params, /*navigation_handle_callback=*/{});
 
+  if (!weak_this) {
+    return nullptr;
+  }
+
+  popup_window_ = popup_window;
   if (!popup_window_) {
     return nullptr;
   }
@@ -87,6 +96,9 @@
   // because we requested a `NEW_FOREGROUND_TAB` disposition.
   if (!is_fullscreen) {
     ResizeAndFocusPopupWindow();
+    if (!weak_this) {
+      return nullptr;
+    }
   }
   Observe(popup_window_);
 
diff --git a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
index 773aa5ec..d3806873 100644
--- a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
+++ b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/ui/webid/identity_ui_utils.h"
 #include "chrome/test/base/testing_profile.h"
@@ -51,6 +52,10 @@
       const content::OpenURLParams& params,
       base::OnceCallback<void(content::NavigationHandle&)>
           navigation_handle_callback) override {
+    if (on_open_url_from_tab_) {
+      std::move(on_open_url_from_tab_).Run();
+    }
+
     if (should_return_null_popup_window_) {
       return nullptr;
     }
@@ -65,6 +70,12 @@
     bounds_ = bounds;
   }
 
+  void ActivateContents(content::WebContents* contents) override {
+    if (on_activate_contents_) {
+      std::move(on_activate_contents_).Run();
+    }
+  }
+
   blink::mojom::DisplayMode GetDisplayMode(
       const content::WebContents* web_contents) override {
     return is_fullscreen_ ? blink::mojom::DisplayMode::kFullscreen
@@ -77,6 +88,14 @@
 
   void SetIsFullscreen(bool is_fullscreen) { is_fullscreen_ = is_fullscreen; }
 
+  void SetOnOpenURLFromTab(base::OnceClosure on_open_url_from_tab) {
+    on_open_url_from_tab_ = std::move(on_open_url_from_tab);
+  }
+
+  void SetOnActivateContents(base::OnceClosure on_activate_contents) {
+    on_activate_contents_ = std::move(on_activate_contents);
+  }
+
   int opened() const { return opened_; }
   gfx::Rect bounds() const { return bounds_; }
   WindowOpenDisposition disposition() const { return disposition_; }
@@ -85,6 +104,8 @@
   int opened_ = 0;
   bool should_return_null_popup_window_{false};
   bool is_fullscreen_{false};
+  base::OnceClosure on_open_url_from_tab_;
+  base::OnceClosure on_activate_contents_;
   gfx::Rect bounds_;
   WindowOpenDisposition disposition_{WindowOpenDisposition::UNKNOWN};
 };
@@ -105,6 +126,36 @@
   ASSERT_TRUE(web_contents);
 }
 
+TEST_F(FedCmModalDialogViewTest, ShowPopupWindowDeletesView) {
+  TestDelegate delegate(web_contents());
+
+  std::unique_ptr<FedCmModalDialogView> popup_window_view =
+      std::make_unique<FedCmModalDialogView>(web_contents(),
+                                             /*observer=*/nullptr);
+  delegate.SetOnOpenURLFromTab(
+      base::BindLambdaForTesting([&]() { popup_window_view.reset(); }));
+  content::WebContents* web_contents = popup_window_view->ShowPopupWindow(
+      GURL(u"https://example.com"), /*user_close_cancels_flow=*/true);
+
+  EXPECT_EQ(1, delegate.opened());
+  EXPECT_FALSE(web_contents);
+}
+
+TEST_F(FedCmModalDialogViewTest, ShowPopupWindowDeletesViewDuringResize) {
+  TestDelegate delegate(web_contents());
+
+  std::unique_ptr<FedCmModalDialogView> popup_window_view =
+      std::make_unique<FedCmModalDialogView>(web_contents(),
+                                             /*observer=*/nullptr);
+  delegate.SetOnActivateContents(
+      base::BindLambdaForTesting([&]() { popup_window_view.reset(); }));
+  content::WebContents* web_contents = popup_window_view->ShowPopupWindow(
+      GURL(u"https://example.com"), /*user_close_cancels_flow=*/true);
+
+  EXPECT_EQ(1, delegate.opened());
+  EXPECT_FALSE(web_contents);
+}
+
 TEST_F(FedCmModalDialogViewTest, ShowPopupWindowInFullscreen) {
   // Override the delegate to test that OpenURLFromTab gets called.
   TestDelegate delegate(web_contents());

diff --git a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
index 773aa5ec..d3806873 100644
--- a/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
+++ b/chrome/browser/ui/views/webid/fedcm_modal_dialog_view_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/ui/webid/identity_ui_utils.h"
 #include "chrome/test/base/testing_profile.h"
@@ -51,6 +52,10 @@
       const content::OpenURLParams& params,
       base::OnceCallback<void(content::NavigationHandle&)>
           navigation_handle_callback) override {
+    if (on_open_url_from_tab_) {
+      std::move(on_open_url_from_tab_).Run();
+    }
+
     if (should_return_null_popup_window_) {
       return nullptr;
     }
@@ -65,6 +70,12 @@
     bounds_ = bounds;
   }
 
+  void ActivateContents(content::WebContents* contents) override {
+    if (on_activate_contents_) {
+      std::move(on_activate_contents_).Run();
+    }
+  }
+
   blink::mojom::DisplayMode GetDisplayMode(
       const content::WebContents* web_contents) override {
     return is_fullscreen_ ? blink::mojom::DisplayMode::kFullscreen
@@ -77,6 +88,14 @@
 
   void SetIsFullscreen(bool is_fullscreen) { is_fullscreen_ = is_fullscreen; }
 
+  void SetOnOpenURLFromTab(base::OnceClosure on_open_url_from_tab) {
+    on_open_url_from_tab_ = std::move(on_open_url_from_tab);
+  }
+
+  void SetOnActivateContents(base::OnceClosure on_activate_contents) {
+    on_activate_contents_ = std::move(on_activate_contents);
+  }
+
   int opened() const { return opened_; }
   gfx::Rect bounds() const { return bounds_; }
   WindowOpenDisposition disposition() const { return disposition_; }
@@ -85,6 +104,8 @@
   int opened_ = 0;
   bool should_return_null_popup_window_{false};
   bool is_fullscreen_{false};
+  base::OnceClosure on_open_url_from_tab_;
+  base::OnceClosure on_activate_contents_;
   gfx::Rect bounds_;
   WindowOpenDisposition disposition_{WindowOpenDisposition::UNKNOWN};
 };
@@ -105,6 +126,36 @@
   ASSERT_TRUE(web_contents);
 }
 
+TEST_F(FedCmModalDialogViewTest, ShowPopupWindowDeletesView) {
+  TestDelegate delegate(web_contents());
+
+  std::unique_ptr<FedCmModalDialogView> popup_window_view =
+      std::make_unique<FedCmModalDialogView>(web_contents(),
+                                             /*observer=*/nullptr);
+  delegate.SetOnOpenURLFromTab(
+      base::BindLambdaForTesting([&]() { popup_window_view.reset(); }));
+  content::WebContents* web_contents = popup_window_view->ShowPopupWindow(
+      GURL(u"https://example.com"), /*user_close_cancels_flow=*/true);
+
+  EXPECT_EQ(1, delegate.opened());
+  EXPECT_FALSE(web_contents);
+}
+
+TEST_F(FedCmModalDialogViewTest, ShowPopupWindowDeletesViewDuringResize) {
+  TestDelegate delegate(web_contents());
+
+  std::unique_ptr<FedCmModalDialogView> popup_window_view =
+      std::make_unique<FedCmModalDialogView>(web_contents(),
+                                             /*observer=*/nullptr);
+  delegate.SetOnActivateContents(
+      base::BindLambdaForTesting([&]() { popup_window_view.reset(); }));
+  content::WebContents* web_contents = popup_window_view->ShowPopupWindow(
+      GURL(u"https://example.com"), /*user_close_cancels_flow=*/true);
+
+  EXPECT_EQ(1, delegate.opened());
+  EXPECT_FALSE(web_contents);
+}
+
 TEST_F(FedCmModalDialogViewTest, ShowPopupWindowInFullscreen) {
   // Override the delegate to test that OpenURLFromTab gets called.
   TestDelegate delegate(web_contents());