Commit 079002c2 authored by Vasilii Sukhanov's avatar Vasilii Sukhanov Committed by Commit Bot

Fix a crash in PasswordGenerationPopupControllerImpl caused by double deletion.

The flow of the crash
- PasswordManager::OnGeneratedPasswordAccepted is called when a generated password is accepted.
- It can bring up the "Update Password?" bubble
- Focus lost event is happening, the dropdown is closed, the controller is destroyed.
- PasswordManager::OnGeneratedPasswordAccepted finishes.
- PasswordGenerationPopupControllerImpl tries to hide the UI again but everything is gone already.

Bug: 995321
Change-Id: I791c969ea6af1b6fc305219a5c22d49424ab38ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762214Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688556}
parent cf262c5e
...@@ -193,10 +193,13 @@ void PasswordGenerationPopupControllerImpl::PasswordAccepted() { ...@@ -193,10 +193,13 @@ void PasswordGenerationPopupControllerImpl::PasswordAccepted() {
if (state_ != kOfferGeneration) if (state_ != kOfferGeneration)
return; return;
driver_->GetPasswordManager()->OnGeneratedPasswordAccepted( base::WeakPtr<PasswordGenerationPopupControllerImpl> weak_this = GetWeakPtr();
driver_.get(), form_.form_data, generation_element_id_, driver_->GeneratedPasswordAccepted(form_.form_data, generation_element_id_,
current_password_); current_password_);
Hide(); // |this| can be destroyed here because GeneratedPasswordAccepted pops up
// another UI and generates some event to close the dropdown.
if (weak_this)
weak_this->Hide();
} }
void PasswordGenerationPopupControllerImpl::Show(GenerationUIState state) { void PasswordGenerationPopupControllerImpl::Show(GenerationUIState state) {
......
...@@ -10,20 +10,33 @@ ...@@ -10,20 +10,33 @@
#include "components/autofill/core/common/password_generation_util.h" #include "components/autofill/core/common/password_generation_util.h"
#include "components/password_manager/core/browser/stub_password_manager_driver.h" #include "components/password_manager/core/browser/stub_password_manager_driver.h"
#include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/geometry/rect_f.h" #include "ui/gfx/geometry/rect_f.h"
namespace { namespace {
using ::testing::_;
class MockPasswordManagerDriver
: public password_manager::StubPasswordManagerDriver {
public:
MOCK_METHOD1(GeneratedPasswordAccepted, void(const base::string16&));
MOCK_METHOD3(GeneratedPasswordAccepted,
void(const autofill::FormData&,
uint32_t,
const base::string16&));
};
class PasswordGenerationPopupControllerImplTest class PasswordGenerationPopupControllerImplTest
: public ChromeRenderViewHostTestHarness { : public ChromeRenderViewHostTestHarness {
public: public:
std::unique_ptr<password_manager::PasswordManagerDriver> CreateDriver(); std::unique_ptr<MockPasswordManagerDriver> CreateDriver();
}; };
std::unique_ptr<password_manager::PasswordManagerDriver> std::unique_ptr<MockPasswordManagerDriver>
PasswordGenerationPopupControllerImplTest::CreateDriver() { PasswordGenerationPopupControllerImplTest::CreateDriver() {
return std::make_unique<password_manager::StubPasswordManagerDriver>(); return std::make_unique<MockPasswordManagerDriver>();
} }
TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateTheSame) { TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateTheSame) {
...@@ -144,4 +157,24 @@ TEST_F(PasswordGenerationPopupControllerImplTest, ...@@ -144,4 +157,24 @@ TEST_F(PasswordGenerationPopupControllerImplTest,
EXPECT_TRUE(controller2); EXPECT_TRUE(controller2);
} }
TEST_F(PasswordGenerationPopupControllerImplTest, DestroyInPasswordAccepted) {
autofill::password_generation::PasswordGenerationUIData ui_data(
gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
100 /*generation_element_id*/, base::i18n::TextDirection(),
autofill::PasswordForm());
auto driver = CreateDriver();
std::unique_ptr<content::WebContents> web_contents = CreateTestWebContents();
base::WeakPtr<PasswordGenerationPopupController> controller =
PasswordGenerationPopupControllerImpl::GetOrCreate(
nullptr /*previous*/, ui_data.bounds, ui_data, driver->AsWeakPtr(),
nullptr, web_contents.get(), main_rfh());
// Destroying the controller in GeneratedPasswordAccepted() should not cause a
// crash.
EXPECT_CALL(*driver,
GeneratedPasswordAccepted(_, 100 /*generation_element_id*/, _))
.WillOnce([controller](auto, auto, auto) { controller->Hide(); });
controller->PasswordAccepted();
}
} // namespace } // namespace
...@@ -124,6 +124,14 @@ void ContentPasswordManagerDriver::GeneratedPasswordAccepted( ...@@ -124,6 +124,14 @@ void ContentPasswordManagerDriver::GeneratedPasswordAccepted(
GetPasswordGenerationAgent()->GeneratedPasswordAccepted(password); GetPasswordGenerationAgent()->GeneratedPasswordAccepted(password);
} }
void ContentPasswordManagerDriver::GeneratedPasswordAccepted(
const autofill::FormData& form_data,
uint32_t generation_element_id,
const base::string16& password) {
GetPasswordManager()->OnGeneratedPasswordAccepted(
this, form_data, generation_element_id, password);
}
void ContentPasswordManagerDriver::FillSuggestion( void ContentPasswordManagerDriver::FillSuggestion(
const base::string16& username, const base::string16& username,
const base::string16& password) { const base::string16& password) {
......
...@@ -62,6 +62,9 @@ class ContentPasswordManagerDriver ...@@ -62,6 +62,9 @@ class ContentPasswordManagerDriver
void AutofillDataReceived( void AutofillDataReceived(
const autofill::FormsPredictionsMap& predictions) override; const autofill::FormsPredictionsMap& predictions) override;
void GeneratedPasswordAccepted(const base::string16& password) override; void GeneratedPasswordAccepted(const base::string16& password) override;
void GeneratedPasswordAccepted(const autofill::FormData& form_data,
uint32_t generation_element_id,
const base::string16& password) override;
void FillSuggestion(const base::string16& username, void FillSuggestion(const base::string16& username,
const base::string16& password) override; const base::string16& password) override;
void FillIntoFocusedField(bool is_password, void FillIntoFocusedField(bool is_password,
......
...@@ -55,8 +55,16 @@ class PasswordManagerDriver ...@@ -55,8 +55,16 @@ class PasswordManagerDriver
const autofill::FormsPredictionsMap& predictions) {} const autofill::FormsPredictionsMap& predictions) {}
// Notifies the driver that the user has accepted a generated password. // Notifies the driver that the user has accepted a generated password.
// TODO(crbug/936011): delete this method. The UI should call the one below.
virtual void GeneratedPasswordAccepted(const base::string16& password) = 0; virtual void GeneratedPasswordAccepted(const base::string16& password) = 0;
// Notifies the password manager that the user has accepted a generated
// password. The password manager can bring up some disambiguation UI in
// response.
virtual void GeneratedPasswordAccepted(const autofill::FormData& form_data,
uint32_t generation_element_id,
const base::string16& password) {}
// Tells the driver to fill the form with the |username| and |password|. // Tells the driver to fill the form with the |username| and |password|.
virtual void FillSuggestion(const base::string16& username, virtual void FillSuggestion(const base::string16& username,
const base::string16& password) = 0; const base::string16& password) = 0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment