Store GlobalRenderFrameHostId instead of ptr

Storing the pointer is discouraged. One should store
the GlobalRenderFrameHostId instead and resole the
pointer when used.

Bug: b:218825270
Bug: 1306968
Change-Id: Ib5b825362e0030baf79c1460089e4545e26e15b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532485
Reviewed-by: Florian Gauger <fga@google.com>
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/main@{#985234}

components/autofill_assistant/browser/actions/action_delegate_util_unittest.cc

@@ -216,7 +216,7 @@ TEST_F(ActionDelegateUtilTest, PerformWithPasswordManagerValue) {
   auto element = std::make_unique<ElementFinder::Result>();
   content::WebContentsTester::For(web_contents_.get())
       ->NavigateAndCommit(GURL("https://www.example.com"));
-  element->container_frame_host = web_contents_->GetMainFrame();
+  element->SetRenderFrameHost(web_contents_->GetMainFrame());
 
   user_data_.selected_login_ = absl::make_optional<WebsiteLoginManager::Login>(
       GURL("https://www.example.com"), "username");
@@ -239,8 +239,7 @@ TEST_F(ActionDelegateUtilTest, PerformWithPasswordManagerValue) {
 
 TEST_F(ActionDelegateUtilTest, PerformWithFailingPasswordManagerValue) {
   auto element = std::make_unique<ElementFinder::Result>();
-
-  element->container_frame_host = web_contents_->GetMainFrame();
+  element->SetRenderFrameHost(web_contents_->GetMainFrame());
 
   user_data_.selected_login_ = absl::make_optional<WebsiteLoginManager::Login>(
       GURL("https://www.example.com"), "username");

components/autofill_assistant/browser/actions/get_element_status_action_unittest.cc

@@ -702,7 +702,7 @@ TEST_F(GetElementStatusActionTest, SucceedsWithPasswordManagerValue) {
       .WillOnce(WithArgs<1>([this](auto&& callback) {
         std::unique_ptr<ElementFinder::Result> element =
             std::make_unique<ElementFinder::Result>();
-        element->container_frame_host = web_contents_->GetMainFrame();
+        element->SetRenderFrameHost(web_contents_->GetMainFrame());
         std::move(callback).Run(OkClientStatus(), std::move(element));
       }));
   EXPECT_CALL(mock_website_login_manager_, GetPasswordForLogin(_, _))

components/autofill_assistant/browser/actions/send_keystroke_events_action.cc

@@ -41,14 +41,14 @@ void SendKeystrokeEventsAction::InternalProcessAction(
   // When dealing with a password field we also want to know the last time that
   // a user has used it.
   auto selected_login_opt = delegate_->GetUserData()->selected_login_;
+  auto* target_render_frame_host = element_.render_frame_host();
   if (proto_.send_keystroke_events().value().value_case() ==
           TextValue::kPasswordManagerValue &&
-      selected_login_opt) {
+      selected_login_opt && target_render_frame_host) {
     // Origin check is done in PWM based on the
-    // |target_element.container_frame_host->GetLastCommittedURL()|
-    selected_login_opt->origin =
-        element_.container_frame_host->GetLastCommittedURL()
-            .DeprecatedGetOriginAsURL();
+    // |target_render_frame_host->GetLastCommittedURL()|
+    selected_login_opt->origin = target_render_frame_host->GetLastCommittedURL()
+                                     .DeprecatedGetOriginAsURL();
 
     delegate_->GetWebsiteLoginManager()->GetGetLastTimePasswordUsed(
         *selected_login_opt,

components/autofill_assistant/browser/actions/send_keystroke_events_action_unittest.cc

@@ -119,7 +119,7 @@ TEST_F(SendKeystrokeEventsActionTest, PasswordTextValueReturnLastTimeUsed) {
   content::WebContentsTester::For(web_contents_.get())
       ->NavigateAndCommit(GURL(kUrl));
   element.dom_object.object_data.object_id = "id";
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
   mock_action_delegate_.GetElementStore()->AddElement("e", element.dom_object);
 
   EXPECT_CALL(mock_web_controller_,
@@ -159,7 +159,7 @@ TEST_F(SendKeystrokeEventsActionTest,
   content::WebContentsTester::For(web_contents_.get())
       ->NavigateAndCommit(GURL(kUrl));
   element.dom_object.object_data.object_id = "id";
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
   mock_action_delegate_.GetElementStore()->AddElement("e", element.dom_object);
 
   EXPECT_CALL(mock_web_controller_,

components/autofill_assistant/browser/user_data_util.cc

@@ -606,7 +606,8 @@ void GetPasswordManagerValue(
     std::move(callback).Run(ClientStatus(PRECONDITION_FAILED), std::string());
     return;
   }
-  if (!target_element.container_frame_host) {
+  auto* target_render_frame_host = target_element.render_frame_host();
+  if (!target_render_frame_host) {
     std::move(callback).Run(ClientStatus(PASSWORD_ORIGIN_MISMATCH),
                             std::string());
     return;
@@ -616,8 +617,8 @@ void GetPasswordManagerValue(
     case PasswordManagerValue::PASSWORD: {
       auto login = *user_data->selected_login_;
       // Origin check is done in PWM based on the
-      // |target_element.container_frame_host->GetLastCommittedURL()|
-      login.origin = target_element.container_frame_host->GetLastCommittedURL()
+      // |target_render_frame_host->GetLastCommittedURL()|
+      login.origin = target_render_frame_host->GetLastCommittedURL()
                          .DeprecatedGetOriginAsURL();
       website_login_manager->GetPasswordForLogin(
           login, base::BindOnce(&OnGetStoredPassword, std::move(callback)));

components/autofill_assistant/browser/user_data_util_unittest.cc

@@ -1261,7 +1261,7 @@ TEST_F(UserDataUtilTextValueTest, GetUsername) {
       GURL("https://www.example.com"), "username");
 
   ElementFinder::Result element;
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
 
   PasswordManagerValue password_manager_value;
   password_manager_value.set_credential_type(PasswordManagerValue::USERNAME);
@@ -1279,7 +1279,7 @@ TEST_F(UserDataUtilTextValueTest, GetStoredPassword) {
       GURL("https://www.example.com"), "username");
 
   ElementFinder::Result element;
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
 
   PasswordManagerValue password_manager_value;
   password_manager_value.set_credential_type(PasswordManagerValue::PASSWORD);
@@ -1301,7 +1301,7 @@ TEST_F(UserDataUtilTextValueTest, GetStoredPasswordFails) {
   ElementFinder::Result element;
   content::WebContentsTester::For(web_contents_.get())
       ->NavigateAndCommit(GURL("https://www.example.com"));
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
 
   PasswordManagerValue password_manager_value;
   password_manager_value.set_credential_type(PasswordManagerValue::PASSWORD);
@@ -1382,7 +1382,7 @@ TEST_F(UserDataUtilTextValueTest, TextValuePasswordManagerValue) {
   ElementFinder::Result element;
   content::WebContentsTester::For(web_contents_.get())
       ->NavigateAndCommit(GURL("https://www.example.com"));
-  element.container_frame_host = web_contents_->GetMainFrame();
+  element.SetRenderFrameHost(web_contents_->GetMainFrame());
 
   TextValue text_value;
   text_value.mutable_password_manager_value()->set_credential_type(

components/autofill_assistant/browser/web/click_or_tap_worker.cc

@@ -31,7 +31,7 @@ void ClickOrTapWorker::Start(const ElementFinder::Result& element,
       /* check_interval= */ base::Milliseconds(0), node_frame_id_);
 
   element_position_getter_->Start(
-      element.container_frame_host, element.object_id(),
+      element.render_frame_host(), element.object_id(),
       base::BindOnce(&ClickOrTapWorker::OnGetCoordinates,
                      weak_ptr_factory_.GetWeakPtr()));
 }

components/autofill_assistant/browser/web/element_finder.cc

@@ -159,10 +159,9 @@ void ElementFinder::Start(const Result& start_element, Callback callback) {
     return;
   }
 
-  if (start_element.container_frame_host == nullptr) {
+  current_frame_ = start_element.render_frame_host();
+  if (current_frame_ == nullptr) {
     current_frame_ = web_contents_->GetMainFrame();
-  } else {
-    current_frame_ = start_element.container_frame_host;
   }
   current_frame_id_ = start_element.node_frame_id();
   frame_stack_ = start_element.frame_stack();
@@ -244,7 +243,7 @@ void ElementFinder::ResultFound(const std::string& object_id) {
 
 ElementFinder::Result ElementFinder::BuildResult(const std::string& object_id) {
   Result result;
-  result.container_frame_host = current_frame_;
+  result.SetRenderFrameHost(current_frame_);
   result.dom_object.object_data.object_id = object_id;
   result.dom_object.object_data.node_frame_id = current_frame_id_;
   return result;

components/autofill_assistant/browser/web/element_finder.h

@@ -60,8 +60,9 @@ class ElementFinder : public WebControllerWorker {
   };
 
   // Result is the fully resolved element that can be used without limitations.
-  // This means that |container_frame_host| has been found and is not nullptr.
-  struct Result {
+  // This means that |render_frame_host()| has been found and is not nullptr.
+  class Result {
+   public:
     Result();
     ~Result();
     Result(const Result&);
@@ -72,8 +73,12 @@ class ElementFinder : public WebControllerWorker {
 
     DomObjectFrameStack dom_object;
 
-    // The render frame host contains the element.
-    raw_ptr<content::RenderFrameHost> container_frame_host = nullptr;
+    content::RenderFrameHost* render_frame_host() const {
+      if (!render_frame_id_) {
+        return nullptr;
+      }
+      return content::RenderFrameHost::FromID(*render_frame_id_);
+    }
 
     const std::string& object_id() const {
       return dom_object.object_data.object_id;
@@ -90,6 +95,17 @@ class ElementFinder : public WebControllerWorker {
     bool IsEmpty() const {
       return object_id().empty() && node_frame_id().empty();
     }
+
+    void SetRenderFrameHost(content::RenderFrameHost* render_frame_host) {
+      if (!render_frame_host) {
+        return;
+      }
+      render_frame_id_ = render_frame_host->GetGlobalId();
+    }
+
+   private:
+    // The id of the render frame host that contains the element.
+    absl::optional<content::GlobalRenderFrameHostId> render_frame_id_;
   };
 
   // |web_contents|, |devtools_client| and |user_data| must be valid for the

components/autofill_assistant/browser/web/element_store.cc

@@ -44,7 +44,7 @@ ClientStatus ElementStore::RestoreElement(
     VLOG(1) << __func__ << " failed to resolve frame.";
     return ClientStatus(CLIENT_ID_RESOLUTION_FAILED);
   }
-  out_element->container_frame_host = frame;
+  out_element->SetRenderFrameHost(frame);
   return OkClientStatus();
 }
 

components/autofill_assistant/browser/web/element_store_unittest.cc

@@ -87,7 +87,7 @@ TEST_F(ElementStoreTest, GetElementFromStoreWithNoFrameId) {
   ElementFinder::Result result;
   EXPECT_EQ(ACTION_APPLIED,
             element_store_->GetElement("1", &result).proto_status());
-  EXPECT_EQ(web_contents_->GetMainFrame(), result.container_frame_host);
+  EXPECT_EQ(web_contents_->GetMainFrame(), result.render_frame_host());
 }
 
 TEST_F(ElementStoreTest, AddElementToStoreOverwrites) {

components/autofill_assistant/browser/web/fake_element_store.cc

@@ -25,7 +25,7 @@ ClientStatus FakeElementStore::GetElement(
 
   out_element->dom_object = it->second;
   if (web_contents_ != nullptr) {
-    out_element->container_frame_host = web_contents_->GetMainFrame();
+    out_element->SetRenderFrameHost(web_contents_->GetMainFrame());
   }
   return OkClientStatus();
 }

components/autofill_assistant/browser/web/web_controller.cc

@@ -732,7 +732,7 @@ void WebController::WaitUntilElementIsStable(
                                               element.node_frame_id());
   auto* ptr = getter.get();
   pending_workers_.emplace_back(std::move(getter));
-  ptr->Start(element.container_frame_host, element.object_id(),
+  ptr->Start(element.render_frame_host(), element.object_id(),
              base::BindOnce(&WebController::OnWaitUntilElementIsStable,
                             weak_ptr_factory_.GetWeakPtr(), ptr,
                             base::TimeTicks::Now(), std::move(callback)));
@@ -1048,8 +1048,8 @@ void WebController::OnGetBackendNodeIdForFormAndFieldData(
     return;
   }
 
-  ContentAutofillDriver* driver = ContentAutofillDriver::GetForRenderFrameHost(
-      element.container_frame_host);
+  ContentAutofillDriver* driver =
+      ContentAutofillDriver::GetForRenderFrameHost(element.render_frame_host());
   if (driver == nullptr) {
     DVLOG(1) << __func__ << " Failed to get the autofill driver.";
     std::move(callback).Run(UnexpectedErrorStatus(__FILE__, __LINE__), nullptr,

components/autofill_assistant/browser/web/web_controller_browsertest.cc

@@ -602,11 +602,11 @@ class WebControllerBrowserTest : public autofill_assistant::BaseBrowserTest,
                               bool is_main_frame) {
     if (is_main_frame) {
       EXPECT_EQ(shell()->web_contents()->GetMainFrame(),
-                result.container_frame_host);
+                result.render_frame_host());
       EXPECT_EQ(result.frame_stack().size(), 0u);
     } else {
       EXPECT_NE(shell()->web_contents()->GetMainFrame(),
-                result.container_frame_host);
+                result.render_frame_host());
       EXPECT_GE(result.frame_stack().size(), 1u);
     }
     EXPECT_FALSE(result.object_id().empty());
@@ -2758,7 +2758,7 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
   // This makes the devtools action fail.
   ElementFinder::Result element;
   element.dom_object.object_data.node_frame_id = "doesnotexist";
-  element.container_frame_host = web_contents()->GetMainFrame();
+  element.SetRenderFrameHost(web_contents()->GetMainFrame());
 
   EXPECT_EQ(ELEMENT_POSITION_NOT_FOUND,
             WaitUntilElementIsStable(element, 10, base::Milliseconds(100))
@@ -3210,9 +3210,9 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, RunElementFinderFromOOPIF) {
 
   // Create fake element without object id and frame information only.
   ElementFinder::Result fake_frame_element;
-  fake_frame_element.container_frame_host = frame_element.container_frame_host;
+  fake_frame_element.SetRenderFrameHost(frame_element.render_frame_host());
   fake_frame_element.dom_object.object_data.node_frame_id =
-      frame_element.container_frame_host->GetDevToolsFrameToken().ToString();
+      frame_element.render_frame_host()->GetDevToolsFrameToken().ToString();
 
   ClientStatus button_status;
   ElementFinder::Result button_element;