[Mfill][Android] Recreate destroyed filling components

There are rare cases when the the Android native UI is recreated but the
native parts remain untouched (e.g. when the theme is changed).

To prevent native classes using stale providers, reset them when the
component is destroyed and recreate them lazily as needed.
(Since this touches the way the native object is created, clean that
up as well to prevent rare crashes.)

Bug: 1094397, 1051030
Change-Id: I9c5cd1e225db1b4ca61c5136571a0e552fe86940
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2403641Reviewed-by: Vadym Doroshenko  <dvadym@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806934}

chrome/android/features/keyboard_accessory/internal/java/src/org/chromium/chrome/browser/keyboard_accessory/ManualFillingComponentBridge.java

@@ -4,6 +4,7 @@
 
 package org.chromium.chrome.browser.keyboard_accessory;
 
+import android.app.Activity;
 import android.util.SparseArray;
 
 import androidx.annotation.VisibleForTesting;
@@ -25,25 +26,23 @@ import org.chromium.ui.base.WindowAndroid;
 class ManualFillingComponentBridge {
     private final SparseArray<PropertyProvider<AccessorySheetData>> mProviders =
             new SparseArray<>();
-    private final PropertyProvider<Action[]> mActionProvider =
-            new PropertyProvider<>(AccessoryAction.GENERATE_PASSWORD_AUTOMATIC);
-    private final ManualFillingComponent mManualFillingComponent;
-    private final ChromeActivity mActivity;
+    private PropertyProvider<Action[]> mActionProvider;
+    private final WindowAndroid mWindowAndroid;
     private long mNativeView;
+    private final ManualFillingComponent.Observer mDestructionObserver = this::onComponentDestroyed;
 
     private ManualFillingComponentBridge(long nativeView, WindowAndroid windowAndroid) {
         mNativeView = nativeView;
-        mActivity = (ChromeActivity) windowAndroid.getActivity().get();
-        mManualFillingComponent = mActivity.getManualFillingComponent();
-        mManualFillingComponent.registerActionProvider(mActionProvider);
+        mWindowAndroid = windowAndroid;
     }
 
     PropertyProvider<AccessorySheetData> getOrCreateProvider(@AccessoryTabType int tabType) {
         PropertyProvider<AccessorySheetData> provider = mProviders.get(tabType);
         if (provider != null) return provider;
+        if (getManualFillingComponent() == null) return null;
         provider = new PropertyProvider<>();
         mProviders.put(tabType, provider);
-        mManualFillingComponent.registerSheetDataProvider(tabType, provider);
+        getManualFillingComponent().registerSheetDataProvider(tabType, provider);
         return provider;
     }
 
@@ -56,20 +55,23 @@ class ManualFillingComponentBridge {
     @CalledByNative
     private void onItemsAvailable(Object objAccessorySheetData) {
         AccessorySheetData accessorySheetData = (AccessorySheetData) objAccessorySheetData;
-        getOrCreateProvider(accessorySheetData.getSheetType()).notifyObservers(accessorySheetData);
+        PropertyProvider<AccessorySheetData> provider =
+                getOrCreateProvider(accessorySheetData.getSheetType());
+        if (provider != null) provider.notifyObservers(accessorySheetData);
     }
 
     @CalledByNative
     private void onAutomaticGenerationStatusChanged(boolean available) {
         final Action[] generationAction;
-        if (available) {
+        final Activity activity = mWindowAndroid.getActivity().get();
+        if (available && activity != null) {
             // This is meant to suppress the warning that the short string is not used.
             // TODO(crbug.com/855581): Switch between strings based on whether they fit on the
             // screen or not.
             boolean useLongString = true;
             String caption = useLongString
-                    ? mActivity.getString(R.string.password_generation_accessory_button)
-                    : mActivity.getString(R.string.password_generation_accessory_button_short);
+                    ? activity.getString(R.string.password_generation_accessory_button)
+                    : activity.getString(R.string.password_generation_accessory_button_short);
             generationAction = new Action[] {
                     new Action(caption, AccessoryAction.GENERATE_PASSWORD_AUTOMATIC, (action) -> {
                         assert mNativeView
@@ -84,31 +86,46 @@ class ManualFillingComponentBridge {
         } else {
             generationAction = new Action[0];
         }
-        mActionProvider.notifyObservers(generationAction);
+        if (mActionProvider == null && getManualFillingComponent() != null) {
+            mActionProvider = new PropertyProvider<>(AccessoryAction.GENERATE_PASSWORD_AUTOMATIC);
+            getManualFillingComponent().registerActionProvider(mActionProvider);
+        }
+        if (mActionProvider != null) mActionProvider.notifyObservers(generationAction);
     }
 
     @CalledByNative
     void showWhenKeyboardIsVisible() {
-        mManualFillingComponent.showWhenKeyboardIsVisible();
+        if (getManualFillingComponent() != null) {
+            getManualFillingComponent().showWhenKeyboardIsVisible();
+        }
     }
 
     @CalledByNative
     void hide() {
-        mManualFillingComponent.hide();
+        if (getManualFillingComponent() != null) {
+            getManualFillingComponent().hide();
+        }
     }
 
     @CalledByNative
     private void closeAccessorySheet() {
-        mManualFillingComponent.closeAccessorySheet();
+        if (getManualFillingComponent() != null) {
+            getManualFillingComponent().closeAccessorySheet();
+        }
     }
 
     @CalledByNative
     private void swapSheetWithKeyboard() {
-        mManualFillingComponent.swapSheetWithKeyboard();
+        if (getManualFillingComponent() != null) {
+            getManualFillingComponent().swapSheetWithKeyboard();
+        }
     }
 
     @CalledByNative
     private void destroy() {
+        if (getManualFillingComponent() != null) {
+            getManualFillingComponent().removeObserver(mDestructionObserver);
+        }
         for (int i = 0; i < mProviders.size(); ++i) {
             mProviders.valueAt(i).notifyObservers(null);
         }
@@ -195,6 +212,20 @@ class ManualFillingComponentBridge {
         ManualFillingComponentBridgeJni.get().disableServerPredictionsForTesting();
     }
 
+    private ManualFillingComponent getManualFillingComponent() {
+        ChromeActivity activity = (ChromeActivity) mWindowAndroid.getActivity().get();
+        if (activity == null) return null; // Has the activity died since it was last checked?
+        activity.getManualFillingComponent().addObserver(mDestructionObserver);
+        return activity.getManualFillingComponent();
+    }
+
+    private void onComponentDestroyed() {
+        if (mNativeView != 0) {
+            ManualFillingComponentBridgeJni.get().onViewDestroyed(
+                    mNativeView, ManualFillingComponentBridge.this);
+        }
+    }
+
     @NativeMethods
     interface Natives {
         void onFillingTriggered(long nativeManualFillingViewAndroid,
@@ -203,6 +234,8 @@ class ManualFillingComponentBridge {
                 ManualFillingComponentBridge caller, int accessoryAction);
         void onToggleChanged(long nativeManualFillingViewAndroid,
                 ManualFillingComponentBridge caller, int accessoryAction, boolean enabled);
+        void onViewDestroyed(
+                long nativeManualFillingViewAndroid, ManualFillingComponentBridge caller);
         void cachePasswordSheetDataForTesting(WebContents webContents, String[] userNames,
                 String[] passwords, boolean originBlacklisted);
         void notifyFocusedFieldTypeForTesting(WebContents webContents, int focusedFieldType);

chrome/android/features/keyboard_accessory/internal/java/src/org/chromium/chrome/browser/keyboard_accessory/ManualFillingCoordinator.java

@@ -9,6 +9,7 @@ import android.view.ViewStub;
 
 import androidx.annotation.VisibleForTesting;
 
+import org.chromium.base.ObserverList;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryCoordinator;
 import org.chromium.chrome.browser.keyboard_accessory.data.KeyboardAccessoryData;
@@ -30,6 +31,7 @@ import org.chromium.ui.base.WindowAndroid;
  */
 class ManualFillingCoordinator implements ManualFillingComponent {
     private final ManualFillingMediator mMediator = new ManualFillingMediator();
+    private ObserverList<Observer> mObserverList = new ObserverList<>();
 
     public ManualFillingCoordinator() {}
 
@@ -55,6 +57,7 @@ class ManualFillingCoordinator implements ManualFillingComponent {
 
     @Override
     public void destroy() {
+        for (Observer observer : mObserverList) observer.onDestroy();
         mMediator.destroy();
     }
 
@@ -126,6 +129,16 @@ class ManualFillingCoordinator implements ManualFillingComponent {
         return mMediator.isFillingViewShown(view);
     }
 
+    @Override
+    public boolean addObserver(Observer observer) {
+        return mObserverList.addObserver(observer);
+    }
+
+    @Override
+    public boolean removeObserver(Observer observer) {
+        return mObserverList.addObserver(observer);
+    }
+
     @VisibleForTesting
     ManualFillingMediator getMediatorForTesting() {
         return mMediator;

chrome/android/features/keyboard_accessory/public/java/src/org/chromium/chrome/browser/keyboard_accessory/ManualFillingComponent.java

@@ -19,6 +19,15 @@ import org.chromium.ui.base.WindowAndroid;
  * This component handles the new, non-popup filling UI.
  */
 public interface ManualFillingComponent {
+    /**
+     * Observers are added with {@link #addObserver} and removed with {@link #removeObserver}.
+     * They are notified when the {@link ManualFillingComponent} is destroyed.
+     */
+    interface Observer {
+        /** Called if the ManualFillingComponent is destroyed. */
+        void onDestroy();
+    }
+
     /**
      * Initializes the manual filling component. Calls to this class are NoOps until this method
      * is called.
@@ -116,4 +125,16 @@ public interface ManualFillingComponent {
      * @param view A {@link View} that is used to find the window root.
      */
     boolean isFillingViewShown(View view);
+
+    /**
+     * @param observer An {@link Observer} to add.
+     * @return True iff the observer could be added.
+     */
+    boolean addObserver(Observer observer);
+
+    /**
+     * @param observer An {@link Observer} to add.
+     * @return True iff the observer could be remove.
+     */
+    boolean removeObserver(Observer observer);
 }

chrome/browser/autofill/manual_filling_controller_impl.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/password_manager/android/password_accessory_metrics_util.h"
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/autofill/core/browser/ui/accessory_sheet_data.h"
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/autofill/core/common/autofill_util.h"
 #include "components/keyed_service/core/service_access_type.h"
@@ -108,6 +109,8 @@ void ManualFillingControllerImpl::OnAutomaticGenerationStatusChanged(
 void ManualFillingControllerImpl::RefreshSuggestions(
     const AccessorySheetData& accessory_sheet_data) {
   view_->OnItemsAvailable(accessory_sheet_data);
+  available_sheets_.emplace(GetSourceForTab(accessory_sheet_data),
+                            accessory_sheet_data);
   UpdateSourceAvailability(GetSourceForTab(accessory_sheet_data),
                            !accessory_sheet_data.user_info_list().empty());
 }
@@ -268,6 +271,11 @@ bool ManualFillingControllerImpl::ShouldShowAccessory() const {
 
 void ManualFillingControllerImpl::UpdateVisibility() {
   if (ShouldShowAccessory()) {
+    for (const FillingSource& source : available_sources_) {
+      if (!available_sheets_.contains(source))
+        continue;
+      view_->OnItemsAvailable(available_sheets_.find(source)->second);
+    }
     view_->ShowWhenKeyboardIsVisible();
   } else {
     view_->Hide();

chrome/browser/autofill/manual_filling_controller_impl.h

@@ -21,7 +21,6 @@ class AddressAccessoryController;
 class CreditCardAccessoryController;
 }  // namespace autofill
 
-
 class AccessoryController;
 class PasswordAccessoryController;
 
@@ -112,6 +111,9 @@ class ManualFillingControllerImpl
   // This set contains sources to be shown to the user.
   base::flat_set<FillingSource> available_sources_;
 
+  // This map contains sheets for each sources to be shown to the user.
+  base::flat_map<FillingSource, autofill::AccessorySheetData> available_sheets_;
+
   // Type of the last known selected field. Helps to determine UI visibility.
   autofill::mojom::FocusedFieldType focused_field_type_ =
       autofill::mojom::FocusedFieldType::kUnknown;

chrome/browser/ui/android/passwords/manual_filling_view_android.cc

@@ -44,58 +44,61 @@ using base::android::ScopedJavaLocalRef;
 
 ManualFillingViewAndroid::ManualFillingViewAndroid(
     ManualFillingController* controller)
-    : controller_(controller) {
-  ui::ViewAndroid* view_android = controller_->container_view();
-  DCHECK(view_android);
-  java_object_.Reset(Java_ManualFillingComponentBridge_create(
-      base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this),
-      view_android->GetWindowAndroid()->GetJavaObject()));
-}
+    : controller_(controller) {}
 
 ManualFillingViewAndroid::~ManualFillingViewAndroid() {
-  DCHECK(!java_object_.is_null());
+  if (!java_object_internal_)
+    return;  // No work to do.
   Java_ManualFillingComponentBridge_destroy(
-      base::android::AttachCurrentThread(), java_object_);
-  java_object_.Reset(nullptr);
+      base::android::AttachCurrentThread(), java_object_internal_);
+  java_object_internal_.Reset(nullptr);
 }
 
 void ManualFillingViewAndroid::OnItemsAvailable(
     const AccessorySheetData& data) {
-  DCHECK(!java_object_.is_null());
-
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_ManualFillingComponentBridge_onItemsAvailable(
-      env, java_object_, ConvertAccessorySheetDataToJavaObject(env, data));
+  if (auto obj = GetOrCreateJavaObject()) {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    Java_ManualFillingComponentBridge_onItemsAvailable(
+        env, obj, ConvertAccessorySheetDataToJavaObject(env, data));
+  }
 }
 
 void ManualFillingViewAndroid::CloseAccessorySheet() {
-  Java_ManualFillingComponentBridge_closeAccessorySheet(
-      base::android::AttachCurrentThread(), java_object_);
+  if (auto obj = GetOrCreateJavaObject()) {
+    Java_ManualFillingComponentBridge_closeAccessorySheet(
+        base::android::AttachCurrentThread(), obj);
+  }
 }
 
 void ManualFillingViewAndroid::SwapSheetWithKeyboard() {
-  Java_ManualFillingComponentBridge_swapSheetWithKeyboard(
-      base::android::AttachCurrentThread(), java_object_);
+  if (auto obj = GetOrCreateJavaObject()) {
+    Java_ManualFillingComponentBridge_swapSheetWithKeyboard(
+        base::android::AttachCurrentThread(), obj);
+  }
 }
 
 void ManualFillingViewAndroid::ShowWhenKeyboardIsVisible() {
-  Java_ManualFillingComponentBridge_showWhenKeyboardIsVisible(
-      base::android::AttachCurrentThread(), java_object_);
+  if (auto obj = GetOrCreateJavaObject()) {
+    Java_ManualFillingComponentBridge_showWhenKeyboardIsVisible(
+        base::android::AttachCurrentThread(), obj);
+  }
 }
 
 void ManualFillingViewAndroid::Hide() {
-  Java_ManualFillingComponentBridge_hide(base::android::AttachCurrentThread(),
-                                         java_object_);
+  if (auto obj = GetOrCreateJavaObject()) {
+    Java_ManualFillingComponentBridge_hide(base::android::AttachCurrentThread(),
+                                           obj);
+  }
 }
 
 void ManualFillingViewAndroid::OnAutomaticGenerationStatusChanged(
     bool available) {
-  if (!available && java_object_.is_null())
+  if (!available && java_object_internal_.is_null())
     return;
-
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_ManualFillingComponentBridge_onAutomaticGenerationStatusChanged(
-      env, java_object_, available);
+  if (auto obj = GetOrCreateJavaObject()) {
+    Java_ManualFillingComponentBridge_onAutomaticGenerationStatusChanged(
+        base::android::AttachCurrentThread(), obj, available);
+  }
 }
 
 void ManualFillingViewAndroid::OnFillingTriggered(
@@ -125,10 +128,17 @@ void ManualFillingViewAndroid::OnToggleChanged(
       static_cast<autofill::AccessoryAction>(selected_action), enabled);
 }
 
+void ManualFillingViewAndroid::OnViewDestroyed(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj) {
+  java_object_internal_.Reset(nullptr);
+}
+
 ScopedJavaLocalRef<jobject>
 ManualFillingViewAndroid::ConvertAccessorySheetDataToJavaObject(
     JNIEnv* env,
     const AccessorySheetData& tab_data) {
+  DCHECK(java_object_internal_);
   ScopedJavaLocalRef<jobject> j_tab_data =
       Java_ManualFillingComponentBridge_createAccessorySheetData(
           env, static_cast<int>(tab_data.get_sheet_type()),
@@ -138,7 +148,7 @@ ManualFillingViewAndroid::ConvertAccessorySheetDataToJavaObject(
   if (tab_data.option_toggle().has_value()) {
     autofill::OptionToggle toggle = tab_data.option_toggle().value();
     Java_ManualFillingComponentBridge_addOptionToggleToAccessorySheetData(
-        env, java_object_, j_tab_data,
+        env, java_object_internal_, j_tab_data,
         ConvertUTF16ToJavaString(env, toggle.display_text()),
         toggle.is_enabled(), static_cast<int>(toggle.accessory_action()));
   }
@@ -146,12 +156,12 @@ ManualFillingViewAndroid::ConvertAccessorySheetDataToJavaObject(
   for (const UserInfo& user_info : tab_data.user_info_list()) {
     ScopedJavaLocalRef<jobject> j_user_info =
         Java_ManualFillingComponentBridge_addUserInfoToAccessorySheetData(
-            env, java_object_, j_tab_data,
+            env, java_object_internal_, j_tab_data,
             ConvertUTF8ToJavaString(env, user_info.origin()),
             user_info.is_psl_match().value());
     for (const UserInfo::Field& field : user_info.fields()) {
       Java_ManualFillingComponentBridge_addFieldToUserInfo(
-          env, java_object_, j_user_info,
+          env, java_object_internal_, j_user_info,
           static_cast<int>(tab_data.get_sheet_type()),
           ConvertUTF16ToJavaString(env, field.display_text()),
           ConvertUTF16ToJavaString(env, field.a11y_description()),
@@ -162,7 +172,7 @@ ManualFillingViewAndroid::ConvertAccessorySheetDataToJavaObject(
 
   for (const FooterCommand& footer_command : tab_data.footer_commands()) {
     Java_ManualFillingComponentBridge_addFooterCommandToAccessorySheetData(
-        env, java_object_, j_tab_data,
+        env, java_object_internal_, j_tab_data,
         ConvertUTF16ToJavaString(env, footer_command.display_text()),
         static_cast<int>(footer_command.accessory_action()));
   }
@@ -184,6 +194,18 @@ UserInfo::Field ManualFillingViewAndroid::ConvertJavaUserInfoField(
                          selectable);
 }
 
+base::android::ScopedJavaGlobalRef<jobject>
+ManualFillingViewAndroid::GetOrCreateJavaObject() {
+  if (java_object_internal_)
+    return java_object_internal_;
+  ui::ViewAndroid* view_android = controller_->container_view();
+  DCHECK(view_android);
+  java_object_internal_.Reset(Java_ManualFillingComponentBridge_create(
+      base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this),
+      view_android->GetWindowAndroid()->GetJavaObject()));
+  return java_object_internal_;
+}
+
 // static
 void JNI_ManualFillingComponentBridge_CachePasswordSheetDataForTesting(
     JNIEnv* env,

chrome/browser/ui/android/passwords/manual_filling_view_android.h

@@ -54,6 +54,8 @@ class ManualFillingViewAndroid : public ManualFillingViewInterface {
                        const base::android::JavaParamRef<jobject>& obj,
                        jint selected_action,
                        jboolean enabled);
+  void OnViewDestroyed(JNIEnv* env,
+                       const base::android::JavaParamRef<jobject>& obj);
 
  private:
   void OnImageFetched(base::android::ScopedJavaGlobalRef<jstring> j_origin,
@@ -69,11 +71,13 @@ class ManualFillingViewAndroid : public ManualFillingViewInterface {
       JNIEnv* env,
       const base::android::JavaRef<jobject>& j_field_to_convert);
 
+  base::android::ScopedJavaGlobalRef<jobject> GetOrCreateJavaObject();
+
   // The controller provides data for this view and owns it.
   ManualFillingController* controller_;
 
-  // The corresponding java object.
-  base::android::ScopedJavaGlobalRef<jobject> java_object_;
+  // The corresponding java object. Use `GetOrCreateJavaObject()` to access.
+  base::android::ScopedJavaGlobalRef<jobject> java_object_internal_;
 
   DISALLOW_COPY_AND_ASSIGN(ManualFillingViewAndroid);
 };