Do not open the password bubble if the state of the controller has changed.

Previously the following happened:
- a background tab decided that it should pop up a password bubble.
- the bubble was suppressed as the tab is inactive.
- another event changed the state of the controller but the flag to open the bubble was still on.
- the tab became active and the bubble was popped up despite the state was unexpected.

With this CL the bubble is not gonna open automatically when the state has changed in the mean time.

Bug: 805556
Change-Id: Ia66054889550b1b26b3abeb5926acbb8d7ceea05
Reviewed-on: https://chromium-review.googlesource.com/893381
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532916}

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

@@ -145,6 +145,7 @@ void ManagePasswordsUIController::OnHideManualFallbackForSaving() {
 
   save_fallback_timer_.Stop();
 
+  ClearPopUpFlagForBubble();
   if (passwords_data_.GetCurrentForms().empty())
     passwords_data_.OnInactive();
   else
@@ -218,6 +219,7 @@ void ManagePasswordsUIController::OnPasswordAutofilled(
   // for the user that the current state.
   if (passwords_data_.state() == password_manager::ui::INACTIVE_STATE ||
       passwords_data_.state() == password_manager::ui::MANAGE_STATE) {
+    ClearPopUpFlagForBubble();
     passwords_data_.OnPasswordAutofilled(password_form_map, origin,
                                          federated_matches);
     // Don't close the existing bubble. Update the icon later.
@@ -232,8 +234,10 @@ void ManagePasswordsUIController::OnLoginsChanged(
     const password_manager::PasswordStoreChangeList& changes) {
   password_manager::ui::State current_state = GetState();
   passwords_data_.ProcessLoginsChanged(changes);
-  if (current_state != GetState())
+  if (current_state != GetState()) {
+    ClearPopUpFlagForBubble();
     UpdateBubbleAndIconVisibility();
+  }
 }
 
 void ManagePasswordsUIController::UpdateIconAndBubbleState(
@@ -247,8 +251,7 @@ void ManagePasswordsUIController::UpdateIconAndBubbleState(
     icon->SetState(GetState());
     ShowBubbleWithoutUserInteraction();
     // If the bubble appeared then the status is updated in OnBubbleShown().
-    if (ShouldBubblePopUp())
-      bubble_status_ = NOT_SHOWN;
+    ClearPopUpFlagForBubble();
   } else {
     password_manager::ui::State state = GetState();
     // The dialog should hide the icon.
@@ -439,6 +442,7 @@ void ManagePasswordsUIController::UpdatePassword(
 
   save_fallback_timer_.Stop();
   UpdatePasswordInternal(password_form);
+  ClearPopUpFlagForBubble();
   passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
   UpdateBubbleAndIconVisibility();
 }
@@ -454,6 +458,7 @@ void ManagePasswordsUIController::ChooseCredential(
   autofill::PasswordForm copy_form = form;
   dialog_controller_.reset();
   passwords_data_.ChooseCredential(&copy_form);
+  ClearPopUpFlagForBubble();
   passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
   UpdateBubbleAndIconVisibility();
 }
@@ -491,6 +496,7 @@ void ManagePasswordsUIController::NavigateToChromeSignIn() {
 void ManagePasswordsUIController::OnDialogHidden() {
   dialog_controller_.reset();
   if (GetState() == password_manager::ui::CREDENTIAL_REQUEST_STATE) {
+    ClearPopUpFlagForBubble();
     passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
     UpdateBubbleAndIconVisibility();
   }
@@ -545,12 +551,14 @@ void ManagePasswordsUIController::UpdateBubbleAndIconVisibility() {
   // display either the bubble or the icon.
   if (!ChromePasswordManagerClient::CanShowBubbleOnURL(
           web_contents()->GetLastCommittedURL())) {
+    ClearPopUpFlagForBubble();
     passwords_data_.OnInactive();
   }
 
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
   if (!browser)
     return;
+
   LocationBar* location_bar = browser->window()->GetLocationBar();
   DCHECK(location_bar);
   location_bar->UpdateManagePasswordsIconAndBubble();
@@ -588,6 +596,7 @@ void ManagePasswordsUIController::DidFinishNavigation(
 
   // Otherwise, reset the password manager.
   DestroyAccountChooser();
+  ClearPopUpFlagForBubble();
   passwords_data_.OnInactive();
   UpdateBubbleAndIconVisibility();
 }
@@ -605,12 +614,18 @@ base::TimeDelta ManagePasswordsUIController::GetTimeoutForSaveFallback() {
 void ManagePasswordsUIController::ShowBubbleWithoutUserInteraction() {
   DCHECK(ShouldBubblePopUp());
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
-  if (!browser || browser->toolbar_model()->input_in_progress())
+  // Can be zero in the tests.
+  if (!browser)
     return;
 
   chrome::ExecuteCommand(browser, IDC_MANAGE_PASSWORDS_FOR_PAGE);
 }
 
+void ManagePasswordsUIController::ClearPopUpFlagForBubble() {
+  if (ShouldBubblePopUp())
+    bubble_status_ = NOT_SHOWN;
+}
+
 void ManagePasswordsUIController::DestroyAccountChooser() {
   if (dialog_controller_ && dialog_controller_->IsShowingAccountChooser()) {
     dialog_controller_.reset();

chrome/browser/ui/passwords/manage_passwords_ui_controller.h

@@ -132,6 +132,13 @@ class ManagePasswordsUIController
   bool AuthenticateUser() override;
   bool ArePasswordsRevealedWhenBubbleIsOpened() const override;
 
+#if defined(UNIT_TEST)
+  // Overwrites the client for |passwords_data_|.
+  void set_client(password_manager::PasswordManagerClient* client) {
+    passwords_data_.set_client(client);
+  }
+#endif  // defined(UNIT_TEST)
+
  protected:
   explicit ManagePasswordsUIController(
       content::WebContents* web_contents);
@@ -165,11 +172,6 @@ class ManagePasswordsUIController
            bubble_status_ == SHOULD_POP_UP_AFTER_REAUTH;
   }
 
-  // Overwrites the client for |passwords_data_|.
-  void set_client(password_manager::PasswordManagerClient* client) {
-    passwords_data_.set_client(client);
-  }
-
   // content::WebContentsObserver:
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
@@ -196,6 +198,10 @@ class ManagePasswordsUIController
   // Shows the password bubble without user interaction.
   void ShowBubbleWithoutUserInteraction();
 
+  // Resets |bubble_status_| signalling that if the bubble was due to pop up,
+  // it shouldn't anymore.
+  void ClearPopUpFlagForBubble();
+
   // Closes the account chooser gracefully so the callback is called. Then sets
   // the state to MANAGE_STATE.
   void DestroyAccountChooser();

chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc

@@ -181,6 +181,7 @@ class ManagePasswordsUIControllerTest : public ChromeRenderViewHostTestHarness {
 
   void SetUp() override;
 
+  password_manager::StubPasswordManagerClient& client() { return client_; }
   password_manager::FakeFormFetcher& fetcher() { return fetcher_; }
   autofill::PasswordForm& test_local_form() { return test_local_form_; }
   autofill::PasswordForm& test_federated_form() { return test_federated_form_; }
@@ -429,6 +430,33 @@ TEST_F(ManagePasswordsUIControllerTest, PasswordSubmittedBubbleNotSuppressed) {
   variations::testing::ClearAllVariationParams();
 }
 
+TEST_F(ManagePasswordsUIControllerTest, PasswordSubmittedBubbleCancelled) {
+  // Test on the real controller.
+  std::unique_ptr<content::WebContents> web_content(CreateTestWebContents());
+  content::WebContentsTester::For(web_content.get())
+      ->NavigateAndCommit(GURL("http://example.com"));
+  ManagePasswordsUIController::CreateForWebContents(web_content.get());
+  ManagePasswordsUIController* controller =
+      ManagePasswordsUIController::FromWebContents(web_content.get());
+  controller->set_client(&client());
+
+  std::unique_ptr<password_manager::PasswordFormManager> test_form_manager(
+      CreateFormManager());
+  test_form_manager->ProvisionallySave(
+      test_local_form(),
+      password_manager::PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  // The bubble is ready to open but the tab is inactive. Therefore, we don't
+  // call UpdateIconAndBubbleState here.
+  controller->OnPasswordSubmitted(std::move(test_form_manager));
+  EXPECT_TRUE(controller->IsAutomaticallyOpeningBubble());
+
+  // The tab navigated in background. Because the controller's state has changed
+  // the bubble shouldn't pop up anymore.
+  content::WebContentsTester::For(web_content.get())
+      ->NavigateAndCommit(GURL("http://google.com"));
+  EXPECT_FALSE(controller->IsAutomaticallyOpeningBubble());
+}
+
 TEST_F(ManagePasswordsUIControllerTest, PasswordSaved) {
   std::unique_ptr<password_manager::PasswordFormManager> test_form_manager(
       CreateFormManager());