[iOS][MF] Disconnect the mediator on |stop|.

Now the mediator stops observing objects when the incognito BVC is destroyed. Waiting for dealloc was causing a race condition with the autorelease pool, and some times a DCHECK will be hit. A minor strong block reference is also fixed in this CL. Bug: 934628, 939727 Change-Id: Ic65a7297eabf7ffad19119832a11125698215ea9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1515572 Commit-Queue: Javier Ernesto Flores Robles <[email protected]> Reviewed-by: Olivier Robin <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#639885}(cherry picked from commit 5cb6233cc8e90faf1e47b6fd1201a04c80206430) Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1520675 Reviewed-by: Javier Ernesto Flores Robles <[email protected]> Cr-Commit-Position: refs/branch-heads/3729@{#84} Cr-Branched-From: d4a8972-refs/heads/master@{#638880}
nwjs · Mar 13, 2019 · f45c768 · f45c768
1 parent 79cb489
commit f45c768
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 5 deletions.
diff --git a/ios/chrome/browser/ui/autofill/form_input_accessory_coordinator.mm b/ios/chrome/browser/ui/autofill/form_input_accessory_coordinator.mm
@@ -99,6 +99,7 @@ - (instancetype)initWithBaseViewController:(UIViewController*)viewController
 - (void)stop {
   [self stopChildren];
   [self.formInputAccessoryViewController restoreOriginalKeyboardView];
+  [self.formInputAccessoryMediator disconnect];
 }
 
 #pragma mark - Presenting Children

diff --git a/ios/chrome/browser/ui/autofill/form_input_accessory_mediator.h b/ios/chrome/browser/ui/autofill/form_input_accessory_mediator.h
@@ -52,6 +52,9 @@ class WebStateList;
 // consumer.
 - (void)enableSuggestions;
 
+// Stops observing all objects.
+- (void)disconnect;
+
 @end
 
 // Methods to allow injection in tests.

diff --git a/ios/chrome/browser/ui/autofill/form_input_accessory_mediator.mm b/ios/chrome/browser/ui/autofill/form_input_accessory_mediator.mm
@@ -204,6 +204,15 @@ @implementation FormInputAccessoryMediator {
 }
 
 - (void)dealloc {
+  [self disconnect];
+}
+
+- (void)disconnect {
+  _formActivityObserverBridge.reset();
+  if (_personalDataManager && _personalDataManagerObserver.get()) {
+    _personalDataManager->RemoveObserver(_personalDataManagerObserver.get());
+    _personalDataManagerObserver.reset();
+  }
   if (_webState) {
     _webState->RemoveObserver(_webStateObserverBridge.get());
     _webStateObserverBridge.reset();
@@ -214,10 +223,6 @@ - (void)dealloc {
     _webStateListObserver.reset();
     _webStateList = nullptr;
   }
-  _formActivityObserverBridge.reset();
-  if (_personalDataManager) {
-    _personalDataManager->RemoveObserver(_personalDataManagerObserver.get());
-  }
 }
 
 - (void)detachFromWebState {
@@ -519,7 +524,7 @@ - (void)retrieveSuggestionsForForm:(const autofill::FormActivityParams&)params
       return;
     }
     FormSuggestionsReadyCompletion formSuggestionsReadyCompletion =
-        [self accessoryViewReadyBlockWithCompletion:completion];
+        [strongSelf accessoryViewReadyBlockWithCompletion:completion];
     [provider retrieveSuggestionsForForm:params
                                 webState:strongSelf.webState
                 accessoryViewUpdateBlock:formSuggestionsReadyCompletion];

diff --git a/ios/chrome/browser/ui/autofill/manual_fill/fallback_coordinator_egtest.mm b/ios/chrome/browser/ui/autofill/manual_fill/fallback_coordinator_egtest.mm
@@ -597,6 +597,73 @@ - (void)testIncognitoManualFallbackMenu {
       assertWithMatcher:grey_sufficientlyVisible()];
 }
 
+// Tests the mediator stops observing objects when the incognito BVC is
+// destroyed. Waiting for dealloc was causing a race condition with the
+// autorelease pool, and some times a DCHECK will be hit.
+- (void)testOpeningIncognitoTabsDoNotLeak {
+  const GURL URL = self.testServer->GetURL(kFormHTMLFile);
+  NSString* omniboxText = base::SysUTF8ToNSString(URL.spec() + "\n");
+  std::string webViewText("Profile form");
+  AddAutofillProfile(_personalDataManager);
+
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewMatcher()]
+      performAction:chrome_test_util::TapWebElement(kFormElementCity)];
+
+  // Verify the profiles icon is visible.
+  [[EarlGrey selectElementWithMatcher:ProfilesIconMatcher()]
+      assertWithMatcher:grey_sufficientlyVisible()];
+
+  // Open a tab in incognito.
+  [ChromeEarlGrey openNewIncognitoTab];
+  [ChromeEarlGreyUI focusOmniboxAndType:omniboxText];
+  [ChromeEarlGrey waitForWebViewContainingText:webViewText];
+
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewMatcher()]
+      performAction:chrome_test_util::TapWebElement(kFormElementCity)];
+
+  // Verify the profiles icon is visible.
+  [[EarlGrey selectElementWithMatcher:ProfilesIconMatcher()]
+      assertWithMatcher:grey_sufficientlyVisible()];
+
+  [ChromeEarlGrey closeCurrentTab];
+  [ChromeEarlGrey openNewTab];
+  [ChromeEarlGrey loadURL:URL];
+  [ChromeEarlGrey waitForWebViewContainingText:webViewText];
+
+  // Bring up the keyboard by tapping the city, which is the element before the
+  // picker.
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewMatcher()]
+      performAction:chrome_test_util::TapWebElement(kFormElementCity)];
+
+  // Verify the profiles icon is visible.
+  [[EarlGrey selectElementWithMatcher:ProfilesIconMatcher()]
+      assertWithMatcher:grey_sufficientlyVisible()];
+
+  // Open a tab in incognito.
+  [ChromeEarlGrey openNewIncognitoTab];
+  [ChromeEarlGreyUI focusOmniboxAndType:omniboxText];
+  [ChromeEarlGrey waitForWebViewContainingText:webViewText];
+
+  // Bring up the keyboard by tapping the city, which is the element before the
+  // picker.
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewMatcher()]
+      performAction:chrome_test_util::TapWebElement(kFormElementCity)];
+
+  // Open a  regular tab.
+  [ChromeEarlGrey openNewTab];
+  [ChromeEarlGrey loadURL:URL];
+  [ChromeEarlGrey waitForWebViewContainingText:webViewText];
+
+  // Bring up the keyboard by tapping the city, which is the element before the
+  // picker.
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewMatcher()]
+      performAction:chrome_test_util::TapWebElement(kFormElementCity)];
+
+  // This will fail if there is more than one profiles icon in the hierarchy.
+  [[EarlGrey selectElementWithMatcher:ProfilesIconMatcher()]
+      assertWithMatcher:grey_sufficientlyVisible()];
+}
+
 // Tests that the manual fallback view is not duplicated after incognito.
 - (void)testReturningFromIncognitoDoesNotDuplicatesManualFallbackMenu {
   // Add the profile to use for verification.