From 190dd31dbc31b2248ca68173a819cf593f3687f0 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Wed, 3 Nov 2021 12:17:06 -0500 Subject: [PATCH] refactor: use upstream AutofillDriverFactory diffs (#31676) * refactor: use upstream AutofillDriverFactory diffs Update our copy of AutofillDriver and AutofillDriverFactory to match chromium. --- shell/browser/electron_autofill_driver.cc | 15 ++-- shell/browser/electron_autofill_driver.h | 13 ++-- .../electron_autofill_driver_factory.cc | 73 +++++++++++-------- .../electron_autofill_driver_factory.h | 3 +- shell/browser/electron_browser_client.cc | 3 +- 5 files changed, 62 insertions(+), 45 deletions(-) diff --git a/shell/browser/electron_autofill_driver.cc b/shell/browser/electron_autofill_driver.cc index fb541ac9f75e4..45b6aadd53ea7 100644 --- a/shell/browser/electron_autofill_driver.cc +++ b/shell/browser/electron_autofill_driver.cc @@ -15,16 +15,19 @@ namespace electron { -AutofillDriver::AutofillDriver( - content::RenderFrameHost* render_frame_host, - mojo::PendingAssociatedReceiver request) - : render_frame_host_(render_frame_host), - receiver_(this, std::move(request)) { +AutofillDriver::AutofillDriver(content::RenderFrameHost* render_frame_host) + : render_frame_host_(render_frame_host) { autofill_popup_ = std::make_unique(); -} +} // namespace electron AutofillDriver::~AutofillDriver() = default; +void AutofillDriver::BindPendingReceiver( + mojo::PendingAssociatedReceiver + pending_receiver) { + receiver_.Bind(std::move(pending_receiver)); +} + void AutofillDriver::ShowAutofillPopup( const gfx::RectF& bounds, const std::vector& values, diff --git a/shell/browser/electron_autofill_driver.h b/shell/browser/electron_autofill_driver.h index 2e3b0c489df90..1a2fe40440503 100644 --- a/shell/browser/electron_autofill_driver.h +++ b/shell/browser/electron_autofill_driver.h @@ -20,12 +20,15 @@ namespace electron { class AutofillDriver : public mojom::ElectronAutofillDriver { public: - AutofillDriver( - content::RenderFrameHost* render_frame_host, - mojo::PendingAssociatedReceiver request); - + explicit AutofillDriver(content::RenderFrameHost* render_frame_host); + AutofillDriver(const AutofillDriver&) = delete; + AutofillDriver& operator=(const AutofillDriver&) = delete; ~AutofillDriver() override; + void BindPendingReceiver( + mojo::PendingAssociatedReceiver + pending_receiver); + void ShowAutofillPopup(const gfx::RectF& bounds, const std::vector& values, const std::vector& labels) override; @@ -38,7 +41,7 @@ class AutofillDriver : public mojom::ElectronAutofillDriver { std::unique_ptr autofill_popup_; #endif - mojo::AssociatedReceiver receiver_; + mojo::AssociatedReceiver receiver_{this}; }; } // namespace electron diff --git a/shell/browser/electron_autofill_driver_factory.cc b/shell/browser/electron_autofill_driver_factory.cc index 3c9e9a13f904f..81277c8ad6400 100644 --- a/shell/browser/electron_autofill_driver_factory.cc +++ b/shell/browser/electron_autofill_driver_factory.cc @@ -17,49 +17,32 @@ namespace electron { -namespace { - -std::unique_ptr CreateDriver( - content::RenderFrameHost* render_frame_host, - mojom::ElectronAutofillDriverAssociatedRequest request) { - return std::make_unique(render_frame_host, - std::move(request)); -} - -} // namespace - AutofillDriverFactory::~AutofillDriverFactory() = default; // static void AutofillDriverFactory::BindAutofillDriver( - mojom::ElectronAutofillDriverAssociatedRequest request, + mojo::PendingAssociatedReceiver + pending_receiver, content::RenderFrameHost* render_frame_host) { + DCHECK(render_frame_host); + content::WebContents* web_contents = content::WebContents::FromRenderFrameHost(render_frame_host); - if (!web_contents) - return; + DCHECK(web_contents); - AutofillDriverFactory* factory = - AutofillDriverFactory::FromWebContents(web_contents); - if (!factory) + AutofillDriverFactory* factory = FromWebContents(web_contents); + if (!factory) { + // The message pipe will be closed and raise a connection error to peer + // side. The peer side can reconnect later when needed. return; + } - AutofillDriver* driver = factory->DriverForFrame(render_frame_host); - if (!driver) - factory->AddDriverForFrame( - render_frame_host, - base::BindOnce(CreateDriver, render_frame_host, std::move(request))); + if (auto* driver = factory->DriverForFrame(render_frame_host)) + driver->BindPendingReceiver(std::move(pending_receiver)); } AutofillDriverFactory::AutofillDriverFactory(content::WebContents* web_contents) - : content::WebContentsObserver(web_contents) { - const std::vector frames = - web_contents->GetAllFrames(); - for (content::RenderFrameHost* frame : frames) { - if (frame->IsRenderFrameLive()) - RenderFrameCreated(frame); - } -} + : content::WebContentsObserver(web_contents) {} void AutofillDriverFactory::RenderFrameDeleted( content::RenderFrameHost* render_frame_host) { @@ -80,8 +63,34 @@ void AutofillDriverFactory::DidFinishNavigation( AutofillDriver* AutofillDriverFactory::DriverForFrame( content::RenderFrameHost* render_frame_host) { - auto mapping = driver_map_.find(render_frame_host); - return mapping == driver_map_.end() ? nullptr : mapping->second.get(); + auto insertion_result = driver_map_.emplace(render_frame_host, nullptr); + std::unique_ptr& driver = insertion_result.first->second; + bool insertion_happened = insertion_result.second; + if (insertion_happened) { + // The `render_frame_host` may already be deleted (or be in the process of + // being deleted). In this case, we must not create a new driver. Otherwise, + // a driver might hold a deallocated RFH. + // + // For example, `render_frame_host` is deleted in the following sequence: + // 1. `render_frame_host->~RenderFrameHostImpl()` starts and marks + // `render_frame_host` as deleted. + // 2. `ContentAutofillDriverFactory::RenderFrameDeleted(render_frame_host)` + // destroys the driver of `render_frame_host`. + // 3. `SomeOtherWebContentsObserver::RenderFrameDeleted(render_frame_host)` + // calls `DriverForFrame(render_frame_host)`. + // 5. `render_frame_host->~RenderFrameHostImpl()` finishes. + if (render_frame_host->IsRenderFrameCreated()) { + driver = std::make_unique(render_frame_host); + DCHECK_EQ(driver_map_.find(render_frame_host)->second.get(), + driver.get()); + } else { + driver_map_.erase(insertion_result.first); + DCHECK_EQ(driver_map_.count(render_frame_host), 0u); + return nullptr; + } + } + DCHECK(driver.get()); + return driver.get(); } void AutofillDriverFactory::AddDriverForFrame( diff --git a/shell/browser/electron_autofill_driver_factory.h b/shell/browser/electron_autofill_driver_factory.h index 359387f6040ac..2c0a53afa52f2 100644 --- a/shell/browser/electron_autofill_driver_factory.h +++ b/shell/browser/electron_autofill_driver_factory.h @@ -27,7 +27,8 @@ class AutofillDriverFactory ~AutofillDriverFactory() override; static void BindAutofillDriver( - mojom::ElectronAutofillDriverAssociatedRequest request, + mojo::PendingAssociatedReceiver + pending_receiver, content::RenderFrameHost* render_frame_host); // content::WebContentsObserver: diff --git a/shell/browser/electron_browser_client.cc b/shell/browser/electron_browser_client.cc index b072c92e226fa..f0d262e5f875e 100644 --- a/shell/browser/electron_browser_client.cc +++ b/shell/browser/electron_browser_client.cc @@ -1453,7 +1453,8 @@ bool ElectronBrowserClient::BindAssociatedReceiverFromFrame( mojo::ScopedInterfaceEndpointHandle* handle) { if (interface_name == mojom::ElectronAutofillDriver::Name_) { AutofillDriverFactory::BindAutofillDriver( - mojom::ElectronAutofillDriverAssociatedRequest(std::move(*handle)), + mojo::PendingAssociatedReceiver( + std::move(*handle)), render_frame_host); return true; }