Commit 647b83d2 authored by arthursonzogni's avatar arthursonzogni Committed by Commit Bot

[bfcache] Make password_manager to create driver on demand.

This is 100% similar to:
https://chromium-review.googlesource.com/c/chromium/src/+/1826885

The password_manager component is associating a password_manager driver on every
RenderFrameHost by observing:
 - WebContentsObserver::RenderFrameCreated(render_frame_host)
 - WebContentsObserver::RenderFrameDeleted(render_frame_host)

The problem is that it starts observing when the WebContent is being
added to the tab strip. It means some RenderFrameCreated events are
missing.

To resolve that, this component was simulating receiving
RenderFrameCreated() for every active frame. This is not sufficient,
because it misses RenderFrameHost pending deletion and the ones in the
BackForwardCache.

To fix that, create the password_manager driver on demand when they are needed.

(+ some small refactor when they were needed.)

Bug: 999842
Change-Id: If558a5541d2a6e831d4cf20d7322e655d553da25
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843971
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704172}
parent d2aa2206
...@@ -828,7 +828,6 @@ void ChromePasswordManagerClient::AutomaticGenerationAvailable( ...@@ -828,7 +828,6 @@ void ChromePasswordManagerClient::AutomaticGenerationAvailable(
if (PasswordGenerationController::AllowedForWebContents(web_contents())) { if (PasswordGenerationController::AllowedForWebContents(web_contents())) {
PasswordManagerDriver* driver = driver_factory_->GetDriverForFrame( PasswordManagerDriver* driver = driver_factory_->GetDriverForFrame(
password_generation_driver_bindings_.GetCurrentTargetFrame()); password_generation_driver_bindings_.GetCurrentTargetFrame());
DCHECK(driver);
PasswordGenerationController* generation_controller = PasswordGenerationController* generation_controller =
PasswordGenerationController::GetIfExisting(web_contents()); PasswordGenerationController::GetIfExisting(web_contents());
...@@ -860,7 +859,6 @@ void ChromePasswordManagerClient::ShowPasswordEditingPopup( ...@@ -860,7 +859,6 @@ void ChromePasswordManagerClient::ShowPasswordEditingPopup(
return; return;
auto* driver = driver_factory_->GetDriverForFrame( auto* driver = driver_factory_->GetDriverForFrame(
password_generation_driver_bindings_.GetCurrentTargetFrame()); password_generation_driver_bindings_.GetCurrentTargetFrame());
DCHECK(driver);
gfx::RectF element_bounds_in_screen_space = gfx::RectF element_bounds_in_screen_space =
GetBoundsInScreenSpace(TransformToRootCoordinates( GetBoundsInScreenSpace(TransformToRootCoordinates(
password_generation_driver_bindings_.GetCurrentTargetFrame(), password_generation_driver_bindings_.GetCurrentTargetFrame(),
...@@ -1158,7 +1156,6 @@ void ChromePasswordManagerClient::ShowPasswordGenerationPopup( ...@@ -1158,7 +1156,6 @@ void ChromePasswordManagerClient::ShowPasswordGenerationPopup(
password_manager::ContentPasswordManagerDriver* driver, password_manager::ContentPasswordManagerDriver* driver,
const autofill::password_generation::PasswordGenerationUIData& ui_data, const autofill::password_generation::PasswordGenerationUIData& ui_data,
bool is_manually_triggered) { bool is_manually_triggered) {
DCHECK(driver);
gfx::RectF element_bounds_in_top_frame_space = gfx::RectF element_bounds_in_top_frame_space =
TransformToRootCoordinates(driver->render_frame_host(), ui_data.bounds); TransformToRootCoordinates(driver->render_frame_host(), ui_data.bounds);
if (!is_manually_triggered && if (!is_manually_triggered &&
......
...@@ -98,12 +98,8 @@ void PasswordAccessoryControllerImpl::OnFillingTriggered( ...@@ -98,12 +98,8 @@ void PasswordAccessoryControllerImpl::OnFillingTriggered(
password_manager::ContentPasswordManagerDriverFactory* factory = password_manager::ContentPasswordManagerDriverFactory* factory =
password_manager::ContentPasswordManagerDriverFactory::FromWebContents( password_manager::ContentPasswordManagerDriverFactory::FromWebContents(
web_contents_); web_contents_);
DCHECK(factory);
password_manager::ContentPasswordManagerDriver* driver = password_manager::ContentPasswordManagerDriver* driver =
factory->GetDriverForFrame(web_contents_->GetFocusedFrame()); factory->GetDriverForFrame(web_contents_->GetFocusedFrame());
if (!driver) {
return;
} // |driver| can be NULL if the tab is being closed.
driver->FillIntoFocusedField(selection.is_obfuscated(), driver->FillIntoFocusedField(selection.is_obfuscated(),
selection.display_text()); selection.display_text());
} }
......
...@@ -1900,7 +1900,6 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, ...@@ -1900,7 +1900,6 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
ObservingAutofillClient::FromWebContents(WebContents()); ObservingAutofillClient::FromWebContents(WebContents());
password_manager::ContentPasswordManagerDriver* driver = password_manager::ContentPasswordManagerDriver* driver =
driver_factory->GetDriverForFrame(WebContents()->GetMainFrame()); driver_factory->GetDriverForFrame(WebContents()->GetMainFrame());
DCHECK(driver);
driver->GetPasswordAutofillManager()->set_autofill_client( driver->GetPasswordAutofillManager()->set_autofill_client(
observing_autofill_client); observing_autofill_client);
......
...@@ -544,9 +544,6 @@ TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswords) { ...@@ -544,9 +544,6 @@ TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswords) {
// Set up password manager stuff. // Set up password manager stuff.
ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient( ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
web_contents(), nullptr); web_contents(), nullptr);
password_manager::ContentPasswordManagerDriverFactory::FromWebContents(
web_contents())
->RenderFrameCreated(web_contents()->GetMainFrame());
NavigateAndCommit(GURL("http://www.foo.com/")); NavigateAndCommit(GURL("http://www.foo.com/"));
content::ContextMenuParams params = CreateParams(MenuItem::EDITABLE); content::ContextMenuParams params = CreateParams(MenuItem::EDITABLE);
...@@ -568,9 +565,6 @@ TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswordsIncognito) { ...@@ -568,9 +565,6 @@ TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswordsIncognito) {
// Set up password manager stuff. // Set up password manager stuff.
ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient( ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
incognito_web_contents.get(), nullptr); incognito_web_contents.get(), nullptr);
password_manager::ContentPasswordManagerDriverFactory::FromWebContents(
incognito_web_contents.get())
->RenderFrameCreated(incognito_web_contents->GetMainFrame());
content::WebContentsTester::For(incognito_web_contents.get()) content::WebContentsTester::For(incognito_web_contents.get())
->NavigateAndCommit(GURL("http://www.foo.com/")); ->NavigateAndCommit(GURL("http://www.foo.com/"));
......
...@@ -14,7 +14,6 @@ ...@@ -14,7 +14,6 @@
#include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/password_form.h"
#include "components/password_manager/content/browser/bad_message.h" #include "components/password_manager/content/browser/bad_message.h"
#include "components/password_manager/content/browser/content_password_manager_driver_factory.h" #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
#include "components/password_manager/content/browser/form_submission_tracker_util.h"
#include "components/password_manager/core/browser/password_manager.h" #include "components/password_manager/core/browser/password_manager.h"
#include "components/password_manager/core/browser/password_manager_client.h" #include "components/password_manager/core/browser/password_manager_client.h"
#include "components/password_manager/core/browser/password_manager_metrics_recorder.h" #include "components/password_manager/core/browser/password_manager_metrics_recorder.h"
...@@ -185,19 +184,6 @@ const GURL& ContentPasswordManagerDriver::GetLastCommittedURL() const { ...@@ -185,19 +184,6 @@ const GURL& ContentPasswordManagerDriver::GetLastCommittedURL() const {
return render_frame_host_->GetLastCommittedURL(); return render_frame_host_->GetLastCommittedURL();
} }
void ContentPasswordManagerDriver::DidNavigateFrame(
content::NavigationHandle* navigation_handle) {
// Clear page specific data after main frame navigation.
if (navigation_handle->IsInMainFrame() &&
!navigation_handle->IsSameDocument()) {
NotifyDidNavigateMainFrame(navigation_handle->IsRendererInitiated(),
navigation_handle->GetPageTransition(),
navigation_handle->WasInitiatedByLinkClick(),
GetPasswordManager());
GetPasswordAutofillManager()->DidNavigateMainFrame();
}
}
void ContentPasswordManagerDriver::GeneratePassword( void ContentPasswordManagerDriver::GeneratePassword(
autofill::mojom::PasswordGenerationAgent:: autofill::mojom::PasswordGenerationAgent::
UserTriggeredGeneratePasswordCallback callback) { UserTriggeredGeneratePasswordCallback callback) {
......
...@@ -27,7 +27,6 @@ struct PasswordForm; ...@@ -27,7 +27,6 @@ struct PasswordForm;
} }
namespace content { namespace content {
class NavigationHandle;
class RenderFrameHost; class RenderFrameHost;
} }
...@@ -78,7 +77,6 @@ class ContentPasswordManagerDriver ...@@ -78,7 +77,6 @@ class ContentPasswordManagerDriver
bool IsMainFrame() const override; bool IsMainFrame() const override;
const GURL& GetLastCommittedURL() const override; const GURL& GetLastCommittedURL() const override;
void DidNavigateFrame(content::NavigationHandle* navigation_handle);
// Notify the renderer that the user wants to generate password manually. // Notify the renderer that the user wants to generate password manually.
void GeneratePassword(autofill::mojom::PasswordGenerationAgent:: void GeneratePassword(autofill::mojom::PasswordGenerationAgent::
UserTriggeredGeneratePasswordCallback callback); UserTriggeredGeneratePasswordCallback callback);
......
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
#include "components/autofill/core/common/form_data.h" #include "components/autofill/core/common/form_data.h"
#include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/password_form.h"
#include "components/password_manager/content/browser/content_password_manager_driver.h" #include "components/password_manager/content/browser/content_password_manager_driver.h"
#include "components/password_manager/content/browser/form_submission_tracker_util.h"
#include "components/password_manager/core/browser/password_manager_client.h" #include "components/password_manager/core/browser/password_manager_client.h"
#include "content/public/browser/browser_context.h" #include "content/public/browser/browser_context.h"
#include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_entry.h"
...@@ -42,18 +43,10 @@ void ContentPasswordManagerDriverFactory::CreateForWebContents( ...@@ -42,18 +43,10 @@ void ContentPasswordManagerDriverFactory::CreateForWebContents(
return; return;
// NOTE: Can't use |std::make_unique| due to private constructor. // NOTE: Can't use |std::make_unique| due to private constructor.
auto new_factory = base::WrapUnique(new ContentPasswordManagerDriverFactory(
web_contents, password_client, autofill_client));
const std::vector<content::RenderFrameHost*> frames =
web_contents->GetAllFrames();
for (content::RenderFrameHost* frame : frames) {
if (frame->IsRenderFrameLive())
new_factory->RenderFrameCreated(frame);
}
web_contents->SetUserData( web_contents->SetUserData(
kContentPasswordManagerDriverFactoryWebContentsUserDataKey, kContentPasswordManagerDriverFactoryWebContentsUserDataKey,
std::move(new_factory)); base::WrapUnique(new ContentPasswordManagerDriverFactory(
web_contents, password_client, autofill_client)));
} }
ContentPasswordManagerDriverFactory::ContentPasswordManagerDriverFactory( ContentPasswordManagerDriverFactory::ContentPasswordManagerDriverFactory(
...@@ -95,29 +88,23 @@ void ContentPasswordManagerDriverFactory::BindPasswordManagerDriver( ...@@ -95,29 +88,23 @@ void ContentPasswordManagerDriverFactory::BindPasswordManagerDriver(
if (!factory) if (!factory)
return; return;
ContentPasswordManagerDriver* driver = factory->GetDriverForFrame(render_frame_host)
factory->GetDriverForFrame(render_frame_host); ->BindPendingReceiver(std::move(pending_receiver));
if (driver)
driver->BindPendingReceiver(std::move(pending_receiver));
} }
ContentPasswordManagerDriver* ContentPasswordManagerDriver*
ContentPasswordManagerDriverFactory::GetDriverForFrame( ContentPasswordManagerDriverFactory::GetDriverForFrame(
content::RenderFrameHost* render_frame_host) { content::RenderFrameHost* render_frame_host) {
auto mapping = frame_driver_map_.find(render_frame_host); DCHECK_EQ(web_contents(),
return mapping == frame_driver_map_.end() ? nullptr : mapping->second.get(); content::WebContents::FromRenderFrameHost(render_frame_host));
} DCHECK(render_frame_host->IsRenderFrameCreated());
void ContentPasswordManagerDriverFactory::RenderFrameCreated( auto& driver = frame_driver_map_[render_frame_host];
content::RenderFrameHost* render_frame_host) { if (!driver) {
auto insertion_result = driver = std::make_unique<ContentPasswordManagerDriver>(
frame_driver_map_.insert(std::make_pair(render_frame_host, nullptr)); render_frame_host, password_client_, autofill_client_);
// This is called twice for the main frame.
if (insertion_result.second) { // This was the first time.
insertion_result.first->second =
std::make_unique<ContentPasswordManagerDriver>(
render_frame_host, password_client_, autofill_client_);
} }
return driver.get();
} }
void ContentPasswordManagerDriverFactory::RenderFrameDeleted( void ContentPasswordManagerDriverFactory::RenderFrameDeleted(
...@@ -126,12 +113,20 @@ void ContentPasswordManagerDriverFactory::RenderFrameDeleted( ...@@ -126,12 +113,20 @@ void ContentPasswordManagerDriverFactory::RenderFrameDeleted(
} }
void ContentPasswordManagerDriverFactory::DidFinishNavigation( void ContentPasswordManagerDriverFactory::DidFinishNavigation(
content::NavigationHandle* navigation_handle) { content::NavigationHandle* navigation) {
if (!navigation_handle->HasCommitted()) if (!navigation->IsInMainFrame() || navigation->IsSameDocument() ||
!navigation->HasCommitted()) {
return; return;
}
if (auto* driver = GetDriverForFrame(navigation_handle->GetRenderFrameHost())) // Clear page specific data after main frame navigation.
driver->DidNavigateFrame(navigation_handle); NotifyDidNavigateMainFrame(navigation->IsRendererInitiated(),
navigation->GetPageTransition(),
navigation->WasInitiatedByLinkClick(),
password_client_->GetPasswordManager());
GetDriverForFrame(navigation->GetRenderFrameHost())
->GetPasswordAutofillManager()
->DidNavigateMainFrame();
} }
void ContentPasswordManagerDriverFactory::RequestSendLoggingAvailability() { void ContentPasswordManagerDriverFactory::RequestSendLoggingAvailability() {
......
...@@ -54,18 +54,17 @@ class ContentPasswordManagerDriverFactory ...@@ -54,18 +54,17 @@ class ContentPasswordManagerDriverFactory
// chrome://password-manager-internals is available. // chrome://password-manager-internals is available.
void RequestSendLoggingAvailability(); void RequestSendLoggingAvailability();
// content::WebContentsObserver:
void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;
private: private:
ContentPasswordManagerDriverFactory( ContentPasswordManagerDriverFactory(
content::WebContents* web_contents, content::WebContents* web_contents,
PasswordManagerClient* client, PasswordManagerClient* client,
autofill::AutofillClient* autofill_client); autofill::AutofillClient* autofill_client);
// content::WebContentsObserver:
void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;
std::map<content::RenderFrameHost*, std::map<content::RenderFrameHost*,
std::unique_ptr<ContentPasswordManagerDriver>> std::unique_ptr<ContentPasswordManagerDriver>>
frame_driver_map_; frame_driver_map_;
......
...@@ -25,10 +25,10 @@ ...@@ -25,10 +25,10 @@
-org.chromium.chrome.browser.notifications.NotificationPlatformBridgeIntentTest.testLaunchNotificationPreferencesForCategory -org.chromium.chrome.browser.notifications.NotificationPlatformBridgeIntentTest.testLaunchNotificationPreferencesForCategory
-org.chromium.chrome.browser.notifications.NotificationPlatformBridgeIntentTest.testLaunchNotificationPreferencesForWebsite -org.chromium.chrome.browser.notifications.NotificationPlatformBridgeIntentTest.testLaunchNotificationPreferencesForWebsite
-org.chromium.chrome.browser.offlinepages.* -org.chromium.chrome.browser.offlinepages.*
-org.chromium.chrome.browser.password_manager.OnboardingDialogIntegrationTest.testOnboardingAccepted
-org.chromium.chrome.browser.password_manager.OnboardingDialogIntegrationTest.testOnboardingDismissedPressedBack
-org.chromium.chrome.browser.password_manager.OnboardingDialogIntegrationTest.testOnboardingIsShown
-org.chromium.chrome.browser.password_manager.OnboardingDialogIntegrationTest.testOnboardingRejected
-org.chromium.chrome.browser.profiling_host.ProfilingProcessHostAndroidTest.testModeBrowser -org.chromium.chrome.browser.profiling_host.ProfilingProcessHostAndroidTest.testModeBrowser
-org.chromium.chrome.browser.tabmodel.IncognitoTabModelTest.testRecreateInIncognito -org.chromium.chrome.browser.tabmodel.IncognitoTabModelTest.testRecreateInIncognito
-org.chromium.chrome.browser.toolbar.top.BrandColorTest.testBrandColorInterstitial -org.chromium.chrome.browser.toolbar.top.BrandColorTest.testBrandColorInterstitial
# Autofill / password manager.
# https://crbug.com/1011799
-org.chromium.chrome.browser.password_manager.OnboardingDialogIntegrationTest.testOnboardingDismissedPressedBack
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