Browse Source

refactor: use upstream AutofillDriverFactory diffs (#31676)

* refactor: use upstream AutofillDriverFactory diffs

Update our copy of AutofillDriver and AutofillDriverFactory to match chromium.
Charles Kerr 3 years ago
parent
commit
190dd31dbc

+ 9 - 6
shell/browser/electron_autofill_driver.cc

@@ -15,16 +15,19 @@
 
 namespace electron {
 
-AutofillDriver::AutofillDriver(
-    content::RenderFrameHost* render_frame_host,
-    mojo::PendingAssociatedReceiver<mojom::ElectronAutofillDriver> 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<AutofillPopup>();
-}
+}  // namespace electron
 
 AutofillDriver::~AutofillDriver() = default;
 
+void AutofillDriver::BindPendingReceiver(
+    mojo::PendingAssociatedReceiver<mojom::ElectronAutofillDriver>
+        pending_receiver) {
+  receiver_.Bind(std::move(pending_receiver));
+}
+
 void AutofillDriver::ShowAutofillPopup(
     const gfx::RectF& bounds,
     const std::vector<std::u16string>& values,

+ 8 - 5
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<mojom::ElectronAutofillDriver> request);
-
+  explicit AutofillDriver(content::RenderFrameHost* render_frame_host);
+  AutofillDriver(const AutofillDriver&) = delete;
+  AutofillDriver& operator=(const AutofillDriver&) = delete;
   ~AutofillDriver() override;
 
+  void BindPendingReceiver(
+      mojo::PendingAssociatedReceiver<mojom::ElectronAutofillDriver>
+          pending_receiver);
+
   void ShowAutofillPopup(const gfx::RectF& bounds,
                          const std::vector<std::u16string>& values,
                          const std::vector<std::u16string>& labels) override;
@@ -38,7 +41,7 @@ class AutofillDriver : public mojom::ElectronAutofillDriver {
   std::unique_ptr<AutofillPopup> autofill_popup_;
 #endif
 
-  mojo::AssociatedReceiver<mojom::ElectronAutofillDriver> receiver_;
+  mojo::AssociatedReceiver<mojom::ElectronAutofillDriver> receiver_{this};
 };
 
 }  // namespace electron

+ 41 - 32
shell/browser/electron_autofill_driver_factory.cc

@@ -17,49 +17,32 @@
 
 namespace electron {
 
-namespace {
-
-std::unique_ptr<AutofillDriver> CreateDriver(
-    content::RenderFrameHost* render_frame_host,
-    mojom::ElectronAutofillDriverAssociatedRequest request) {
-  return std::make_unique<AutofillDriver>(render_frame_host,
-                                          std::move(request));
-}
-
-}  // namespace
-
 AutofillDriverFactory::~AutofillDriverFactory() = default;
 
 // static
 void AutofillDriverFactory::BindAutofillDriver(
-    mojom::ElectronAutofillDriverAssociatedRequest request,
+    mojo::PendingAssociatedReceiver<mojom::ElectronAutofillDriver>
+        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<content::RenderFrameHost*> 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<AutofillDriver>& 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<AutofillDriver>(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(

+ 2 - 1
shell/browser/electron_autofill_driver_factory.h

@@ -27,7 +27,8 @@ class AutofillDriverFactory
   ~AutofillDriverFactory() override;
 
   static void BindAutofillDriver(
-      mojom::ElectronAutofillDriverAssociatedRequest request,
+      mojo::PendingAssociatedReceiver<mojom::ElectronAutofillDriver>
+          pending_receiver,
       content::RenderFrameHost* render_frame_host);
 
   // content::WebContentsObserver:

+ 2 - 1
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<mojom::ElectronAutofillDriver>(
+            std::move(*handle)),
         render_frame_host);
     return true;
   }