[iOS] Fix cleanup when opening Password Bottom Sheet multiple times

A few things are addressed in this CL: 1) When the bottom sheet is closed, it needs to stop the coordinator. A new coordinator is created each time the bottom sheet opens, so when it closes, it needs to do a proper cleanup. "stop" commands are now issued when the bottom sheet is dismissed and the mediator will disconnect itself after completing the refocus of select suggestion actions. 2) Added missing metric for the Password Details menu. 3) Used the same browser state for profilePasswordStore and accountPasswordStore that we use for everything else (this only affects incognito mode). (cherry picked from commit 84eb47b) Bug: 1443136, 1446136 Change-Id: I573679b697fa8a07800c705b53c10b5e92e86829 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4572829 Reviewed-by: Tommy Martino <tmartino@chromium.org> Commit-Queue: Alexis Hétu <sugoi@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1150769} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4582151 Cr-Commit-Position: refs/branch-heads/5790@{#274} Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
chromium · Jun 2, 2023 · 602cb71 · 602cb71
1 parent 89ad78b
commit 602cb71
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 37 deletions.
diff --git a/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_coordinator.mm b/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_coordinator.mm
@@ -27,6 +27,7 @@
 #error "This file requires ARC support."
 #endif
 
+using PasswordSuggestionBottomSheetExitReason::kShowPasswordDetails;
 using PasswordSuggestionBottomSheetExitReason::kShowPasswordManager;
 
 @interface PasswordSuggestionBottomSheetCoordinator () {
@@ -64,12 +65,10 @@ @implementation PasswordSuggestionBottomSheetCoordinator
 
     auto profilePasswordStore =
         IOSChromePasswordStoreFactory::GetForBrowserState(
-            self.browser->GetBrowserState(),
-            ServiceAccessType::EXPLICIT_ACCESS);
+            browserState, ServiceAccessType::EXPLICIT_ACCESS);
     auto accountPasswordStore =
         IOSChromeAccountPasswordStoreFactory::GetForBrowserState(
-            self.browser->GetBrowserState(),
-            ServiceAccessType::EXPLICIT_ACCESS);
+            browserState, ServiceAccessType::EXPLICIT_ACCESS);
 
     WebStateList* webStateList = browser->GetWebStateList();
     const GURL& URL = webStateList->GetActiveWebState()->GetLastCommittedURL();
@@ -123,22 +122,31 @@ - (void)stop {
 #pragma mark - PasswordSuggestionBottomSheetHandler
 
 - (void)displayPasswordManager {
+  [self.mediator logExitReason:kShowPasswordManager];
+
+  __weak __typeof(self) weakSelf = self;
   [self.baseViewController.presentedViewController
       dismissViewControllerAnimated:NO
-                         completion:nil];
+                         completion:^{
+                           [weakSelf stop];
+                         }];
 
-  [_mediator logExitReason:kShowPasswordManager];
   [_passwordControllerDelegate displaySavedPasswordList];
 }
 
 - (void)displayPasswordDetailsForFormSuggestion:
     (FormSuggestion*)formSuggestion {
+  [self.mediator logExitReason:kShowPasswordDetails];
+  absl::optional<password_manager::CredentialUIEntry> credential =
+      [self.mediator getCredentialForFormSuggestion:formSuggestion];
+
+  __weak __typeof(self) weakSelf = self;
   [self.baseViewController.presentedViewController
       dismissViewControllerAnimated:NO
-                         completion:nil];
+                         completion:^{
+                           [weakSelf stop];
+                         }];
 
-  absl::optional<password_manager::CredentialUIEntry> credential =
-      [self.mediator getCredentialForFormSuggestion:formSuggestion];
   if (credential.has_value()) {
     [_passwordControllerDelegate
         showPasswordDetailsForCredential:credential.value()];

diff --git a/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_handler.h b/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_handler.h
@@ -16,6 +16,9 @@
 // Displays the password details menu.
 - (void)displayPasswordDetailsForFormSuggestion:(FormSuggestion*)formSuggestion;
 
+// Cleanup bottom sheet after it has been dismissed.
+- (void)stop;
+
 @end
 
 #endif  // IOS_CHROME_BROWSER_UI_PASSWORDS_BOTTOM_SHEET_PASSWORD_SUGGESTION_BOTTOM_SHEET_HANDLER_H_
diff --git a/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_mediator.mm b/ios/chrome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_mediator.mm
@@ -240,12 +240,7 @@ - (void)didSelectSuggestion:(NSInteger)row {
   if ([_reauthenticationModule canAttemptReauth]) {
     __weak __typeof(self) weakSelf = self;
     auto completionHandler = ^(ReauthenticationResult result) {
-      if (result != ReauthenticationResult::kFailure) {
-        [self logReauthEvent:kSuccess];
-        [weakSelf selectSuggestion:suggestion];
-      } else {
-        [self logReauthEvent:kFailure];
-      }
+      [weakSelf selectSuggestion:suggestion reauthenticationResult:result];
     };
 
     NSString* reason = l10n_util::GetNSString(IDS_IOS_AUTOFILL_REAUTH_REASON);
@@ -267,10 +262,14 @@ - (void)refocus {
     web::WebState* activeWebState = _webStateList->GetActiveWebState();
     password_manager::PasswordManagerJavaScriptFeature* feature =
         password_manager::PasswordManagerJavaScriptFeature::GetInstance();
-    web::WebFrame* frame =
-        feature->GetWebFramesManager(activeWebState)->GetFrameWithId(_frameId);
-    AutofillBottomSheetTabHelper::FromWebState(activeWebState)
-        ->DetachListenersAndRefocus(frame);
+    web::WebFramesManager* framesManager =
+        feature->GetWebFramesManager(activeWebState);
+    if (framesManager) {
+      web::WebFrame* frame = framesManager->GetFrameWithId(_frameId);
+      AutofillBottomSheetTabHelper::FromWebState(activeWebState)
+          ->DetachListenersAndRefocus(frame);
+      [self disconnect];
+    }
   }
 }
 
@@ -360,6 +359,19 @@ - (void)passwordFetcher:(PasswordFetcher*)passwordFetcher
 - (void)selectSuggestion:(FormSuggestion*)suggestion {
   LogLikelyInterestedDefaultBrowserUserActivity(DefaultPromoTypeStaySafe);
   [self.suggestionsProvider didSelectSuggestion:suggestion];
+  [self disconnect];
+}
+
+// Perform suggestion selection based on the reauthentication result.
+- (void)selectSuggestion:(FormSuggestion*)suggestion
+    reauthenticationResult:(ReauthenticationResult)result {
+  if (result != ReauthenticationResult::kFailure) {
+    [self logReauthEvent:kSuccess];
+    [self selectSuggestion:suggestion];
+  } else {
+    [self logReauthEvent:kFailure];
+    [self disconnect];
+  }
 }
 
 // Returns the default favicon attributes after making sure they are

diff --git a/...ome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_view_controller.mm b/...ome/browser/ui/passwords/bottom_sheet/password_suggestion_bottom_sheet_view_controller.mm
@@ -86,11 +86,11 @@ @interface PasswordSuggestionBottomSheetViewController () <
   // The current's page domain. This is used for the password bottom sheet
   // description label.
   NSString* _domain;
-
-  // The password controller handler used to open the password manager.
-  id<PasswordSuggestionBottomSheetHandler> _handler;
 }
 
+// The password controller handler used to open the password manager.
+@property(nonatomic, weak) id<PasswordSuggestionBottomSheetHandler> handler;
+
 @end
 
 @implementation PasswordSuggestionBottomSheetViewController
@@ -99,7 +99,7 @@ - (instancetype)initWithHandler:
     (id<PasswordSuggestionBottomSheetHandler>)handler {
   self = [super init];
   if (self) {
-    _handler = handler;
+    self.handler = handler;
   }
   return self;
 }
@@ -181,7 +181,11 @@ - (void)setSuggestions:(NSArray<FormSuggestion*>*)suggestions
 }
 
 - (void)dismiss {
-  [self dismissViewControllerAnimated:NO completion:NULL];
+  __weak __typeof(self) weakSelf = self;
+  [self dismissViewControllerAnimated:NO
+                           completion:^{
+                             [weakSelf.handler stop];
+                           }];
 }
 
 #pragma mark - UITableViewDelegate
@@ -533,24 +537,13 @@ - (void)expand {
   }
 }
 
-// Opens the password manager settings page.
-- (void)displayPasswordManager {
-  [_handler displayPasswordManager];
-}
-
-// Opens the password details for form suggestion.
-- (void)displayPasswordDetailsForFormSuggestion:
-    (FormSuggestion*)formSuggestion {
-  [_handler displayPasswordDetailsForFormSuggestion:formSuggestion];
-}
-
 // Creates the UI action used to open the password manager.
 - (UIAction*)openPasswordManagerAction {
   __weak __typeof(self) weakSelf = self;
   void (^passwordManagerButtonTapHandler)(UIAction*) = ^(UIAction* action) {
     // Open Password Manager.
     [weakSelf.delegate disableRefocus];
-    [weakSelf displayPasswordManager];
+    [weakSelf.handler displayPasswordManager];
   };
   UIImage* keyIcon =
       CustomSymbolWithPointSize(kPasswordSymbol, kSymbolActionPointSize);
@@ -570,7 +563,7 @@ - (UIAction*)openPasswordDetailsForIndexPath:(NSIndexPath*)indexPath {
   void (^showDetailsButtonTapHandler)(UIAction*) = ^(UIAction* action) {
     // Open Password Details.
     [weakSelf.delegate disableRefocus];
-    [weakSelf displayPasswordDetailsForFormSuggestion:formSuggestion];
+    [weakSelf.handler displayPasswordDetailsForFormSuggestion:formSuggestion];
   };
 
   UIImage* infoIcon =