From 2f99e3829ac315daf4ae98b5e36050d97d807159 Mon Sep 17 00:00:00 2001
From: Timofey Chudakov <[email protected]>
Date: Mon, 18 May 2026 05:50:52 -0700
Subject: [PATCH] Reland "[Autofill] Revise ownership in the AuthenticatorSelectionDialog."

This reverts commit 9296a450e63e8b458c1d31c9cc4a0ae7a01c2d10.

Reason for revert: fix crashes by rearranging calls to controller and
adding documentation.

Original change's description:
> Revert "[Autofill] Revise ownership in the AuthenticatorSelectionDialog."
>
> This reverts commit 2818507d4cd729091d6948ea984823d6cdd04c4c.
>
> Reason for revert: reverting due to crashes.
>
> Original change's description:
> > [Autofill] Revise ownership in the AuthenticatorSelectionDialog.
> >
> > CardUnmaskAuthenticationSelectionDialogControllerImpl used to store the
> > CardUnmaskAuthenticationSelectionDialog as a raw pointer. This makes
> > sense on Desktop because the created view is owned by the dialog
> > manager.
> >
> > The Android's AuthenticatorSelectionDialogView is not owned by the
> > system component and was responsible for deleting itself. This is
> > problematic from the memory management perspective.
> >
> > The root cause of the memory issue is that the Android dialog can be
> > dismissed both from C++ and from Java. The C++ dismissal happens when
> > the server request fails or the WebContents is destroyed. The Java
> > dismissal happens when the use clicks a dialog button.
> >
> > The crashing scenario is following:
> > 1. CardUnmaskAuthenticationSelectionDialogControllerImpl destructor is
> >    called.
> > 2. AuthenticatorSelectionDialogViewAndroid::Dismiss calls the Java
> >    dismiss.
> > 3. The dialog is dismissed asynchronously.
> > 4. AuthenticatorSelectionDialogViewAndroid::OnDismissed is called, which
> >    deletes the native view.
> > 5. AuthenticatorSelectionDialog still holds the native pointer. The user
> >    clicks one of the dialog buttons.
> > 6. AuthenticatorSelectionDialogBridge calls the native view in response
> >    to the button click, but the view has already been destroyed.
> >
> > (1) The AuthenticatorSelectionDialogViewAndroid is now owned by the
> > unique pointer to simplify the memory management. A special wrapper is
> > created to store the native view via a raw pointer on Desktop and iOS.
> >
> > (2) AuthenticatorSelectionDialogViewAndroid is destroyed immediately
> > by the controller after it's dismissed. This means that Java will never
> > call the C++ view if the dismissal call was coming from C++.
> >
> > (3) AuthenticatorSelectionDialogBridge now resets the native pointer
> > immediately when the dismissal happens. This means that Java side will
> > never get called by the C++ side after the view is dismissed by the
> > user.
> >
> > [1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/autofill/payments/card_unmask_authentication_selection_dialog_view.cc;l=282-286;drc=0be01c1048de19d23525ffb5eaae17d9979282fb
> >
> > Bug: 501594107
> > Change-Id: I28fc815e0cba3c135dd20e7fd38e3795ed846dd9
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7789965
> > Reviewed-by: Christoph Schwering <[email protected]>
> > Commit-Queue: Timofey Chudakov <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1622344}
>
> Bug: 501594107, 512800620
> Change-Id: Iebece3ee2cd69a593d6b3081ddda9811bfc25398
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7849045
> Reviewed-by: Christoph Schwering <[email protected]>
> Commit-Queue: Timofey Chudakov <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1632059}

Bug: 501594107, 512800620
Change-Id: If47ab1b8c8121c1e077f2fdedf9485b4f9a6b00d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7852382
Commit-Queue: Timofey Chudakov <[email protected]>
Reviewed-by: Christoph Schwering <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1632109}
---

diff --git a/chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc b/chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc
index 7a5de79..83dfc57 100644
--- a/chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc
+++ b/chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc
@@ -40,15 +40,18 @@
 
 void AuthenticatorSelectionDialogViewAndroid::Dismiss(bool user_closed_dialog,
                                                       bool server_success) {
-  if (controller_) {
-    controller_->OnDialogClosed(user_closed_dialog, server_success);
-    controller_ = nullptr;
-  }
-  JNIEnv* env = base::android::AttachCurrentThread();
   if (java_object_) {
-    Java_AuthenticatorSelectionDialogBridge_dismiss(env, java_object_);
-  } else {
-    delete this;
+    // Multiple calls to `AuthenticatorSelectionDialogViewAndroid::Dismiss`
+    // should result in only 1 call to
+    // `Java_AuthenticatorSelectionDialogBridge_dismiss`.
+    Java_AuthenticatorSelectionDialogBridge_dismiss(
+        base::android::AttachCurrentThread(), java_object_);
+    java_object_.Reset();
+  }
+  if (controller_) {
+    // `OnDialogClosed` destroys this view, no member access or method calls
+    // should happen afterwards.
+    controller_->OnDialogClosed(user_closed_dialog, server_success);
   }
 }
 
@@ -67,18 +70,23 @@
 }
 
 void AuthenticatorSelectionDialogViewAndroid::OnDismissed(JNIEnv* env) {
-  // If |controller_| is not nullptr, it means the dismissal was triggered by
-  // user cancellation.
   if (controller_) {
+    // `OnDialogClosed` destroys this view, no member access or method calls
+    // should happen afterwards.
     controller_->OnDialogClosed(/*user_closed_dialog=*/true,
                                 /*server_success=*/false);
-    controller_ = nullptr;
   }
-  delete this;
 }
 
 bool AuthenticatorSelectionDialogViewAndroid::ShowDialog(
     ui::WindowAndroid* window_android) {
+  // Don't show the dialog twice. This should be impossible as long as
+  // `ShowDialog` is called only from
+  // `CreateAndShowCardUnmaskAuthenticationSelectionDialog`.
+  CHECK(!java_object_, base::NotFatalUntil::M150);
+  if (java_object_) {
+    return false;
+  }
   JNIEnv* env = base::android::AttachCurrentThread();
   DCHECK(window_android);
 
diff --git a/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridge.java b/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridge.java
index 3ad743b..d293158 100644
--- a/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridge.java
+++ b/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridge.java
@@ -25,8 +25,8 @@
 @JNINamespace("autofill")
 @NullMarked
 public class AuthenticatorSelectionDialogBridge implements AuthenticatorSelectionDialog.Listener {
-    private final long mNativeCardUnmaskAuthenticationSelectionDialogView;
     private final AuthenticatorSelectionDialog mAuthenticatorSelectionDialog;
+    private long mNativeCardUnmaskAuthenticationSelectionDialogView;
 
     public AuthenticatorSelectionDialogBridge(
             long nativeAuthenticatorSelectionDialogView,
@@ -120,6 +120,9 @@
     @CalledByNative
     public void dismiss() {
         mAuthenticatorSelectionDialog.dismiss(DialogDismissalCause.DISMISSED_BY_NATIVE);
+        // The native C++ view is destroyed after the dialog is dismissed, reset the native pointer
+        // to make it unreachable.
+        mNativeCardUnmaskAuthenticationSelectionDialogView = 0;
     }
 
     /**
@@ -129,6 +132,9 @@
      */
     @Override
     public void onOptionSelected(String authenticatorOptionIdentifier) {
+        if (mNativeCardUnmaskAuthenticationSelectionDialogView == 0) {
+            return;
+        }
         AuthenticatorSelectionDialogBridgeJni.get()
                 .onOptionSelected(
                         mNativeCardUnmaskAuthenticationSelectionDialogView,
@@ -138,8 +144,14 @@
     /** Notify that the dialog was dismissed. */
     @Override
     public void onDialogDismissed() {
+        if (mNativeCardUnmaskAuthenticationSelectionDialogView == 0) {
+            return;
+        }
         AuthenticatorSelectionDialogBridgeJni.get()
                 .onDismissed(mNativeCardUnmaskAuthenticationSelectionDialogView);
+        // The native C++ view is destroyed after the dialog is dismissed, reset the native pointer
+        // to make it unreachable.
+        mNativeCardUnmaskAuthenticationSelectionDialogView = 0;
     }
 
     @NativeMethods
diff --git a/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridgeTest.java b/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridgeTest.java
index 0194ba0..7129111d 100644
--- a/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridgeTest.java
+++ b/chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridgeTest.java
@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.ui.autofill;