Clarify ownership of PasswordAutofillAgent and co

This CL makes AutofillAgent own PasswordAutofillAgent and PasswordGenerationAgent. The motivation for this CL is adding more references to AutofillAgent from PasswordAutofillAgent in a subsequent CL BUG=1495584 Change-Id: Id03ca97956653c1234b25842bff29b5eefca72ac Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4974958 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Bo Liu <boliu@chromium.org> Reviewed-by: Christoph Schwering <schwering@google.com> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/main@{#1217043}
Denger-Network · Oct 30, 2023 · 54f41c7 · 54f41c7
1 parent bc3345e
commit 54f41c7
Show file tree

Hide file tree

Showing 10 changed files with 61 additions and 68 deletions.
diff --git a/android_webview/renderer/aw_render_frame_ext.cc b/android_webview/renderer/aw_render_frame_ext.cc
@@ -5,13 +5,15 @@
 #include "android_webview/renderer/aw_render_frame_ext.h"
 
 #include <memory>
+#include <utility>
 
 #include "android_webview/common/aw_features.h"
 #include "android_webview/common/mojom/frame.mojom.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/content/renderer/autofill_agent.h"
 #include "components/autofill/content/renderer/password_autofill_agent.h"
+#include "components/autofill/content/renderer/password_generation_agent.h"
 #include "components/content_capture/common/content_capture_features.h"
 #include "components/content_capture/renderer/content_capture_sender.h"
 #include "content/public/renderer/render_frame.h"
@@ -143,12 +145,11 @@ void PopulateHitTestData(const GURL& absolute_link_url,
 
 AwRenderFrameExt::AwRenderFrameExt(content::RenderFrame* render_frame)
     : content::RenderFrameObserver(render_frame) {
-  // TODO(sgurun) do not create a password autofill agent (change
-  // autofill agent to store a weakptr).
-  autofill::PasswordAutofillAgent* password_autofill_agent =
-      new autofill::PasswordAutofillAgent(render_frame, &registry_);
-  new autofill::AutofillAgent(render_frame, password_autofill_agent, nullptr,
-                              &registry_);
+  auto password_autofill_agent =
+      std::make_unique<autofill::PasswordAutofillAgent>(render_frame,
+                                                        &registry_);
+  new autofill::AutofillAgent(render_frame, std::move(password_autofill_agent),
+                              nullptr, &registry_);
   if (content_capture::features::IsContentCaptureEnabled())
     new content_capture::ContentCaptureSender(render_frame, &registry_);
 

diff --git a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
@@ -377,7 +377,7 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
 
     // TODO(crbug/862989): Remove workaround preventing non-test classes to bind
     // fake_driver_ or fake_pw_client_.
-    password_autofill_agent_->SetAutofillAgent(autofill_agent_);
+    password_autofill_agent_->Init(autofill_agent_);
     password_autofill_agent_->GetPasswordManagerDriver();
     password_generation_->RequestPasswordManagerClientForTesting();
     base::RunLoop().RunUntilIdle();  // Executes binding the interfaces.

diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
@@ -677,13 +677,13 @@ void ChromeContentRendererClient::RenderFrameCreated(
 
   if (!render_frame->IsInFencedFrameTree() ||
       base::FeatureList::IsEnabled(blink::features::kFencedFramesAPIChanges)) {
-    PasswordAutofillAgent* password_autofill_agent =
-        new PasswordAutofillAgent(render_frame, associated_interfaces);
-    PasswordGenerationAgent* password_generation_agent =
-        new PasswordGenerationAgent(render_frame, password_autofill_agent,
-                                    associated_interfaces);
-    new AutofillAgent(render_frame, password_autofill_agent,
-                      password_generation_agent, associated_interfaces);
+    auto password_autofill_agent = std::make_unique<PasswordAutofillAgent>(
+        render_frame, associated_interfaces);
+    auto password_generation_agent = std::make_unique<PasswordGenerationAgent>(
+        render_frame, password_autofill_agent.get(), associated_interfaces);
+    new AutofillAgent(render_frame, std::move(password_autofill_agent),
+                      std::move(password_generation_agent),
+                      associated_interfaces);
   }
 
   if (content_capture::features::IsContentCaptureEnabled()) {

diff --git a/chrome/test/base/chrome_render_view_test.cc b/chrome/test/base/chrome_render_view_test.cc
@@ -71,13 +71,18 @@ void ChromeRenderViewTest::SetUp() {
   // RenderFrame doesn't expose its Agent objects, because it has no need to
   // store them directly (they're stored as RenderFrameObserver*).  So just
   // create another set. They destroy themselves in OnDestruct().
-  password_autofill_agent_ = new autofill::TestPasswordAutofillAgent(
-      GetMainRenderFrame(), &associated_interfaces_);
-  password_generation_ = new autofill::PasswordGenerationAgent(
-      GetMainRenderFrame(), password_autofill_agent_, &associated_interfaces_);
-  autofill_agent_ =
-      new AutofillAgent(GetMainRenderFrame(), password_autofill_agent_,
-                        password_generation_, &associated_interfaces_);
+  auto unique_password_autofill_agent =
+      std::make_unique<autofill::TestPasswordAutofillAgent>(
+          GetMainRenderFrame(), &associated_interfaces_);
+  password_autofill_agent_ = unique_password_autofill_agent.get();
+  auto unique_password_generation =
+      std::make_unique<autofill::PasswordGenerationAgent>(
+          GetMainRenderFrame(), password_autofill_agent_.get(),
+          &associated_interfaces_);
+  password_generation_ = unique_password_generation.get();
+  autofill_agent_ = new AutofillAgent(
+      GetMainRenderFrame(), std::move(unique_password_autofill_agent),
+      std::move(unique_password_generation), &associated_interfaces_);
 }
 
 void ChromeRenderViewTest::TearDown() {

diff --git a/components/autofill/content/renderer/autofill_agent.cc b/components/autofill/content/renderer/autofill_agent.cc
@@ -299,24 +299,26 @@ void AutofillAgent::FocusStateNotifier::NotifyIfChanged(
   focused_field_id_ = new_focused_field_id;
 }
 
-AutofillAgent::AutofillAgent(content::RenderFrame* render_frame,
-                             PasswordAutofillAgent* password_autofill_agent,
-                             PasswordGenerationAgent* password_generation_agent,
-                             blink::AssociatedInterfaceRegistry* registry)
+AutofillAgent::AutofillAgent(
+    content::RenderFrame* render_frame,
+    std::unique_ptr<PasswordAutofillAgent> password_autofill_agent,
+    std::unique_ptr<PasswordGenerationAgent> password_generation_agent,
+    blink::AssociatedInterfaceRegistry* registry)
     : content::RenderFrameObserver(render_frame),
       form_cache_(std::make_unique<FormCache>(render_frame->GetWebFrame())),
-      password_autofill_agent_(password_autofill_agent),
-      password_generation_agent_(password_generation_agent),
+      password_autofill_agent_(std::move(password_autofill_agent)),
+      password_generation_agent_(std::move(password_generation_agent)),
       query_node_autofill_state_(WebAutofillState::kNotFilled),
       is_popup_possibly_visible_(false),
       is_user_gesture_required_(true),
       is_secure_context_required_(false),
       form_tracker_(render_frame),
-      field_data_manager_(password_autofill_agent->GetFieldDataManager()),
+      field_data_manager_(password_autofill_agent_->GetFieldDataManager()),
       focus_state_notifier_(this) {
   render_frame->GetWebFrame()->SetAutofillClient(this);
-  password_autofill_agent->SetAutofillAgent(this);
+  password_autofill_agent_->Init(this);
   AddFormObserver(this);
+  AddFormObserver(password_autofill_agent_.get());
   registry->AddInterface<mojom::AutofillAgent>(base::BindRepeating(
       &AutofillAgent::BindPendingReceiver, base::Unretained(this)));
 }
@@ -326,6 +328,7 @@ AutofillAgent::AutofillAgent(content::RenderFrame* render_frame,
 // The process may be killed before this deletion can happen.
 AutofillAgent::~AutofillAgent() {
   RemoveFormObserver(this);
+  RemoveFormObserver(password_autofill_agent_.get());
 }
 
 void AutofillAgent::BindPendingReceiver(
@@ -1202,7 +1205,7 @@ void AutofillAgent::DidAddOrRemoveFormRelatedElementsDynamically() {
               password_autofill_agent->OnDynamicFormsSeen();
             }
           },
-          base::Unretained(password_autofill_agent_)));
+          base::Unretained(password_autofill_agent_.get())));
 }
 
 void AutofillAgent::DidCompleteFocusChangeInFrame() {

diff --git a/components/autofill/content/renderer/autofill_agent.h b/components/autofill/content/renderer/autofill_agent.h
@@ -82,10 +82,11 @@ class AutofillAgent : public content::RenderFrameObserver,
   // PasswordAutofillAgent is guaranteed to outlive AutofillAgent.
   // PasswordGenerationAgent and AutofillAssistantAgent may be nullptr. If they
   // are not, then they are also guaranteed to outlive AutofillAgent.
-  AutofillAgent(content::RenderFrame* render_frame,
-                PasswordAutofillAgent* password_autofill_agent,
-                PasswordGenerationAgent* password_generation_agent,
-                blink::AssociatedInterfaceRegistry* registry);
+  AutofillAgent(
+      content::RenderFrame* render_frame,
+      std::unique_ptr<PasswordAutofillAgent> password_autofill_agent,
+      std::unique_ptr<PasswordGenerationAgent> password_generation_agent,
+      blink::AssociatedInterfaceRegistry* registry);
 
   AutofillAgent(const AutofillAgent&) = delete;
   AutofillAgent& operator=(const AutofillAgent&) = delete;
@@ -367,10 +368,8 @@ class AutofillAgent : public content::RenderFrameObserver,
   // reset when the AutofillAgent is pending deletion.
   std::unique_ptr<FormCache> form_cache_;
 
-  raw_ptr<PasswordAutofillAgent, DanglingUntriaged>
-      password_autofill_agent_;  // Weak reference.
-  raw_ptr<PasswordGenerationAgent, DanglingUntriaged>
-      password_generation_agent_;  // Weak reference.
+  std::unique_ptr<PasswordAutofillAgent> password_autofill_agent_;
+  std::unique_ptr<PasswordGenerationAgent> password_generation_agent_;
 
   // The element corresponding to the last request sent for form field Autofill.
   blink::WebFormControlElement last_queried_element_;

diff --git a/components/autofill/content/renderer/autofill_agent_browsertest.cc b/components/autofill/content/renderer/autofill_agent_browsertest.cc
@@ -175,20 +175,18 @@ class AutofillAgentTest : public content::RenderViewTest {
         base::BindRepeating(&MockAutofillDriver::BindPendingReceiver,
                             base::Unretained(&autofill_driver_)));
 
-    password_autofill_agent_ = std::make_unique<TestPasswordAutofillAgent>(
+    auto password_autofill_agent = std::make_unique<TestPasswordAutofillAgent>(
         GetMainRenderFrame(), &associated_interfaces_);
-    password_generation_ = std::make_unique<PasswordGenerationAgent>(
-        GetMainRenderFrame(), password_autofill_agent_.get(),
+    auto password_generation = std::make_unique<PasswordGenerationAgent>(
+        GetMainRenderFrame(), password_autofill_agent.get(),
         &associated_interfaces_);
     autofill_agent_ = std::make_unique<AutofillAgent>(
-        GetMainRenderFrame(), password_autofill_agent_.get(),
-        password_generation_.get(), &associated_interfaces_);
+        GetMainRenderFrame(), std::move(password_autofill_agent),
+        std::move(password_generation), &associated_interfaces_);
   }
 
   void TearDown() override {
     autofill_agent_.reset();
-    password_generation_.reset();
-    password_autofill_agent_.reset();
     RenderViewTest::TearDown();
   }
 
@@ -205,8 +203,6 @@ class AutofillAgentTest : public content::RenderViewTest {
 
  private:
   blink::AssociatedInterfaceRegistry associated_interfaces_;
-  std::unique_ptr<PasswordAutofillAgent> password_autofill_agent_;
-  std::unique_ptr<PasswordGenerationAgent> password_generation_;
 };
 
 class AutofillAgentTestWithFeatures : public AutofillAgentTest {

diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -687,10 +687,10 @@ PasswordAutofillAgent::PasswordAutofillAgent(
       &PasswordAutofillAgent::BindPendingReceiver, base::Unretained(this)));
 }
 
-PasswordAutofillAgent::~PasswordAutofillAgent() {
-  AutofillAgent* agent = autofill_agent_.get();
-  if (agent)
-    agent->RemoveFormObserver(this);
+PasswordAutofillAgent::~PasswordAutofillAgent() = default;
+
+void PasswordAutofillAgent::Init(AutofillAgent* autofill_agent) {
+  autofill_agent_ = autofill_agent;
 }
 
 void PasswordAutofillAgent::BindPendingReceiver(
@@ -699,14 +699,6 @@ void PasswordAutofillAgent::BindPendingReceiver(
   receiver_.Bind(std::move(pending_receiver));
 }
 
-void PasswordAutofillAgent::SetAutofillAgent(AutofillAgent* autofill_agent) {
-  AutofillAgent* agent = autofill_agent_.get();
-  if (agent)
-    agent->RemoveFormObserver(this);
-  autofill_agent_ = autofill_agent->GetWeakPtr();
-  autofill_agent->AddFormObserver(this);
-}
-
 void PasswordAutofillAgent::SetPasswordGenerationAgent(
     PasswordGenerationAgent* generation_agent) {
   password_generation_agent_ = generation_agent;
@@ -1421,8 +1413,6 @@ void PasswordAutofillAgent::OnFrameDetached() {
 
 void PasswordAutofillAgent::OnDestruct() {
   receiver_.reset();
-  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE,
-                                                                this);
 }
 
 bool PasswordAutofillAgent::IsPrerendering() const {
@@ -1612,7 +1602,7 @@ PasswordAutofillAgent::GetFormDataFromUnownedInputElements() {
     return nullptr;
   return CreateFormDataFromUnownedInputElements(
       *web_frame, field_data_manager_.get(), &username_detector_cache_,
-      autofill_agent_ && autofill_agent_->is_heavy_form_data_scraping_enabled()
+      autofill_agent_->is_heavy_form_data_scraping_enabled()
           ? &button_titles_cache_
           : nullptr);
 }
@@ -1975,7 +1965,7 @@ void PasswordAutofillAgent::OnInferredFormSubmission(SubmissionSource source) {
 }
 
 void PasswordAutofillAgent::HidePopup() {
-  if (autofill_agent_ && autofill_agent_->unsafe_autofill_driver()) {
+  if (autofill_agent_->unsafe_autofill_driver()) {
     autofill_agent_->unsafe_autofill_driver()->HidePopup();
   }
 }

diff --git a/components/autofill/content/renderer/password_autofill_agent.h b/components/autofill/content/renderer/password_autofill_agent.h
@@ -119,12 +119,13 @@ class PasswordAutofillAgent : public content::RenderFrameObserver,
 
   ~PasswordAutofillAgent() override;
 
+  // Must be called prior to calling other methods.
+  void Init(AutofillAgent* autofill_agent);
+
   void BindPendingReceiver(
       mojo::PendingAssociatedReceiver<mojom::PasswordAutofillAgent>
           pending_receiver);
 
-  void SetAutofillAgent(AutofillAgent* autofill_agent);
-
   void SetPasswordGenerationAgent(PasswordGenerationAgent* generation_agent);
 
   // Callers should not store the returned value longer than a function scope.
@@ -518,7 +519,7 @@ class PasswordAutofillAgent : public content::RenderFrameObserver,
   // Records the username typed before suggestions preview.
   std::u16string username_query_prefix_;
 
-  base::WeakPtr<AutofillAgent> autofill_agent_;
+  raw_ptr<AutofillAgent> autofill_agent_;
 
   raw_ptr<PasswordGenerationAgent, ExperimentalRenderer>
       password_generation_agent_;  // Weak reference.

diff --git a/components/autofill/content/renderer/password_generation_agent.cc b/components/autofill/content/renderer/password_generation_agent.cc
@@ -281,8 +281,6 @@ void PasswordGenerationAgent::DidChangeScrollOffset() {
 
 void PasswordGenerationAgent::OnDestruct() {
   receiver_.reset();
-  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE,
-                                                                this);
 }
 
 void PasswordGenerationAgent::OnFieldAutofilled(