Skip to content

Commit

Permalink
[ios] Make Password manager load model respect search mode
Browse files Browse the repository at this point in the history 
This patch fixes a crash in PasswordManagerViewController.
The loadModel method was inserting items that were removed while
the VC was in search mode. In search mode, the VC removes some items
and sections, only leaving the password list that gets filtered as the
user types in the search bar. If an event occurred triggering a data
reload, such as a change in the saved password list, while in search
mode, loadModel was re-adding items/sections that are not supposed to
be visible in search mode. Then when the search controller was
dismissed, a crash was happening while we were adding the
items/sections removed when the search started, as we were duplicating
sections in the model.

This patch adds checks in loadModel to add items/sections depending
on the search state of the VC. Now items that are not visible in
search mode are not added in loadModel.

The patch also removes a redundant flag to determine if the VC is in
search mode and adds a unit test to verify the behavior when an a
saved password update is received while in search mode.

Fixed: 1421230
Change-Id: I386a9a6f0751f7a0ef456feac84dcb4428ad980b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4316822
Commit-Queue: Ernesto Izquierdo Clua <eic@google.com>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114839}
  • Loading branch information
Ernesto Izquierdo Clua authored and Chromium LUCI CQ committed Mar 8, 2023
1 parent c6bd95f commit 754dbed
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 64 deletions.
128 changes: 64 additions & 64 deletions ios/chrome/browser/ui/settings/password/password_manager_view_controller.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,6 @@ @interface PasswordManagerViewController () <
// Shared password auto-fill status manager that contains the most updated
// status of password auto-fill for Chrome.
PasswordAutoFillStatusManager* _sharedPasswordAutoFillStatusManager;
// Boolean containing whether `self` should be updated after dismissing
// the Search Controller.
BOOL _shouldUpdateAfterSearchControllerDismissed;
// Whether the table view is in search mode. That is, it only has the search
// bar potentially saved passwords and blocked sites.
BOOL _tableIsInSearchMode;
Expand Down Expand Up @@ -591,8 +588,7 @@ - (void)loadModel {
TableViewModel* model = self.tableViewModel;

if (ShouldShowSettingsUI()) {
// Save passwords switch and manage account message. Only show this section
// when the searchController is not active.
// Don't show sections hidden when search controller is displayed.
if (!_tableIsInSearchMode) {
[model addSectionWithIdentifier:SectionIdentifierSavePasswordsSwitch];

Expand All @@ -608,61 +604,64 @@ - (void)loadModel {
[model addItem:_savePasswordsItem
toSectionWithIdentifier:SectionIdentifierSavePasswordsSwitch];
}
}

// Passwords in other apps.
[model addSectionWithIdentifier:SectionIdentifierPasswordsInOtherApps];
if (!_passwordsInOtherAppsItem) {
_passwordsInOtherAppsItem = [self passwordsInOtherAppsItem];
// Passwords in other apps.
[model addSectionWithIdentifier:SectionIdentifierPasswordsInOtherApps];
if (!_passwordsInOtherAppsItem) {
_passwordsInOtherAppsItem = [self passwordsInOtherAppsItem];
}
[model addItem:_passwordsInOtherAppsItem
toSectionWithIdentifier:SectionIdentifierPasswordsInOtherApps];
}
[model addItem:_passwordsInOtherAppsItem
toSectionWithIdentifier:SectionIdentifierPasswordsInOtherApps];
}

// Password check.
[model addSectionWithIdentifier:SectionIdentifierPasswordCheck];
if (!_passwordProblemsItem) {
_passwordProblemsItem = [self passwordProblemsItem];
}

[self updatePasswordCheckStatusLabelWithState:_passwordCheckState];
[model addItem:_passwordProblemsItem
toSectionWithIdentifier:SectionIdentifierPasswordCheck];
// Don't show sections hidden when search controller is displayed.
if (!_tableIsInSearchMode) {
// Password check.
[model addSectionWithIdentifier:SectionIdentifierPasswordCheck];
if (!_passwordProblemsItem) {
_passwordProblemsItem = [self passwordProblemsItem];
}

if (!_checkForProblemsItem) {
_checkForProblemsItem = [self checkForProblemsItem];
}
[self updatePasswordCheckStatusLabelWithState:_passwordCheckState];
[model addItem:_passwordProblemsItem
toSectionWithIdentifier:SectionIdentifierPasswordCheck];

[self updatePasswordCheckButtonWithState:_passwordCheckState];
if (!_checkForProblemsItem) {
_checkForProblemsItem = [self checkForProblemsItem];
}

// Only add check button if kIOSPasswordCheckup is disabled, or if it is
// enabled and the current PasswordCheckUIState requires the button to be
// shown.
if (!IsPasswordCheckupEnabled() || self.shouldShowCheckButton) {
[model addItem:_checkForProblemsItem
toSectionWithIdentifier:SectionIdentifierPasswordCheck];
}
[self updatePasswordCheckButtonWithState:_passwordCheckState];

// When the Password Checkup feature is enabled, this timestamp only appears
// in the detail text of the Password Checkup status cell. It is therefore
// managed in `updatePasswordCheckStatusLabelWithState`.
if (!IsPasswordCheckupEnabled()) {
[self updateLastCheckTimestampWithState:_passwordCheckState
fromState:_passwordCheckState
update:NO];
}
// Only add check button if kIOSPasswordCheckup is disabled, or if it is
// enabled and the current PasswordCheckUIState requires the button to be
// shown.
if (!IsPasswordCheckupEnabled() || self.shouldShowCheckButton) {
[model addItem:_checkForProblemsItem
toSectionWithIdentifier:SectionIdentifierPasswordCheck];
}

// On-device encryption.
[self updateOnDeviceEncryptionSessionWithUpdateTableView:NO
withRowAnimation:
UITableViewRowAnimationNone];
// When the Password Checkup feature is enabled, this timestamp only appears
// in the detail text of the Password Checkup status cell. It is therefore
// managed in `updatePasswordCheckStatusLabelWithState`.
if (!IsPasswordCheckupEnabled()) {
[self updateLastCheckTimestampWithState:_passwordCheckState
fromState:_passwordCheckState
update:NO];
}

// Add Password button.
if (!ShouldShowSettingsUI() && [self allowsAddPassword]) {
[model addSectionWithIdentifier:SectionIdentifierAddPasswordButton];
_addPasswordItem = [self addPasswordItem];
[model addItem:_addPasswordItem
toSectionWithIdentifier:SectionIdentifierAddPasswordButton];
// On-device encryption.
[self updateOnDeviceEncryptionSessionWithUpdateTableView:NO
withRowAnimation:
UITableViewRowAnimationNone];

// Add Password button.
if (!ShouldShowSettingsUI() && [self allowsAddPassword]) {
[model addSectionWithIdentifier:SectionIdentifierAddPasswordButton];
_addPasswordItem = [self addPasswordItem];
[model addItem:_addPasswordItem
toSectionWithIdentifier:SectionIdentifierAddPasswordButton];
}
}

// Saved passwords.
Expand Down Expand Up @@ -695,14 +694,16 @@ - (void)loadModel {
toSectionWithIdentifier:SectionIdentifierExportPasswordsButton];
}

// Add the descriptive text at the top of the screen. Do this at the end to
// ensure the section to which it's being attached already exists.
// Add the descriptive text at the top of the screen. The section for this
// header is not visible in while in search mode. Adding it to the model only
// when not in search mode.
_manageAccountLinkItem = [self manageAccountLinkItem];
[model setHeader:_manageAccountLinkItem
forSectionWithIdentifier:[self sectionForManageAccountLinkHeader]];
if (!_tableIsInSearchMode) {
[model setHeader:_manageAccountLinkItem
forSectionWithIdentifier:[self sectionForManageAccountLinkHeader]];
}

[self filterItems:self.searchTerm];
_tableIsInSearchMode = NO;
}

// Returns YES if the array of index path contains a saved password. This is to
Expand Down Expand Up @@ -1365,8 +1366,13 @@ - (void)updatePasswordsInOtherAppsDetailedText {
_sharedPasswordAutoFillStatusManager.autoFillEnabled
? l10n_util::GetNSString(IDS_IOS_SETTING_ON)
: l10n_util::GetNSString(IDS_IOS_SETTING_OFF);
[self reloadCellsForItems:@[ _passwordsInOtherAppsItem ]
withRowAnimation:UITableViewRowAnimationNone];

// Item is only visible when search is not active.
// Only update corresponding cell when visible.
if (!_tableIsInSearchMode) {
[self reloadCellsForItems:@[ _passwordsInOtherAppsItem ]
withRowAnimation:UITableViewRowAnimationNone];
}
}
}

Expand Down Expand Up @@ -1394,7 +1400,6 @@ - (void)willPresentSearchController:(UISearchController*)searchController {
self.navigationController.navigationBar.backgroundColor =
[UIColor colorNamed:kGroupedPrimaryBackgroundColor];

_shouldUpdateAfterSearchControllerDismissed = YES;
[self showScrim];
// Remove save passwords switch section, password check section and
// on device encryption.
Expand Down Expand Up @@ -1432,14 +1437,9 @@ - (void)willDismissSearchController:(UISearchController*)searchController {

// No need to restore UI if the Password Manager is being dismissed or if a
// previous call to `willDismissSearchController` already restored the UI.
if (self.navigationController.isBeingDismissed ||
!_shouldUpdateAfterSearchControllerDismissed) {
if (self.navigationController.isBeingDismissed || !_tableIsInSearchMode) {
return;
}
// If `willDismissSearchController` is invoked again before the search
// controller is presented, we don't want to do any updates because they are
// only needed once the search controller is presented and dismissed again.
_shouldUpdateAfterSearchControllerDismissed = NO;

[self hideScrim];
[self searchForTerm:@""];
Expand Down
80 changes: 80 additions & 0 deletions ios/chrome/browser/ui/settings/password/password_manager_view_controller_unittest.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,86 @@ void SetUp() override {
}));
}

// Tests that the password manager is updated when passwords change while in
// search mode.
TEST_F(PasswordManagerViewControllerTest, TestChangePasswordsWhileSearching) {
root_view_controller_ = [[UIViewController alloc] init];
scoped_window_.Get().rootViewController = root_view_controller_;

PasswordManagerViewController* passwords_controller =
GetPasswordManagerViewController();

// Present the view controller.
__block bool presentation_finished = NO;
UINavigationController* navigation_controller =
[[UINavigationController alloc]
initWithRootViewController:passwords_controller];
[root_view_controller_ presentViewController:navigation_controller
animated:NO
completion:^{
presentation_finished = YES;
}];
EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(
base::test::ios::kWaitForUIElementTimeout, ^bool {
return presentation_finished;
}));

EXPECT_EQ(4, NumberOfSections());
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierSavePasswordsSwitch]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierPasswordCheck]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierPasswordsInOtherApps]);
// TODO(crbug.com/1361357): Update test after Export button is moved to
// Password Settings.
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierExportPasswordsButton]);

passwords_controller.navigationItem.searchController.active = YES;

// Add a password update.
AddSavedForm1();

// Simulate an update to passwords in other apps button that shouldn't be
// visible while in search.
[passwords_controller updatePasswordsInOtherAppsDetailedText];

EXPECT_EQ(2, NumberOfSections());
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierSavedPasswords]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierExportPasswordsButton]);
EXPECT_EQ(1, NumberOfItemsInSection(0));

passwords_controller.navigationItem.searchController.active = NO;

// Sections are restored after search is over.
EXPECT_EQ(5, NumberOfSections());
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierSavePasswordsSwitch]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierPasswordCheck]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierPasswordsInOtherApps]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierSavedPasswords]);
EXPECT_TRUE([passwords_controller.tableViewModel
hasSectionForSectionIdentifier:SectionIdentifierExportPasswordsButton]);

// Dismiss `view_controller_` and waits for the dismissal to finish.
__block bool dismissal_finished = NO;
[passwords_controller settingsWillBeDismissed];
[root_view_controller_ dismissViewControllerAnimated:NO
completion:^{
dismissal_finished = YES;
}];
EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(
base::test::ios::kWaitForUIElementTimeout, ^bool {
return dismissal_finished;
}));
}

// Tests that dismissing the Search Controller multiple times without presenting
// it again doesn't cause a crash.
TEST_F(PasswordManagerViewControllerTest,
Expand Down

0 comments on commit 754dbed

Please sign in to comment.