From 394eed87683d1605c639ed37060b26afbac84491 Mon Sep 17 00:00:00 2001
From: Bryant Chandler <[email protected]>
Date: Fri, 15 May 2026 17:11:30 -0700
Subject: [PATCH] Fix tab restore id matching in GlicInstanceCoordinatorImpl

- In GetOrRestoreInstanceImpl, fail lookup if conversation ID matches
but instance ID differs, preventing dangerous map overwrites.
- In RemoveInstance, verify the pointer matches before erasing.
- In CreateGlicInstance, call metrics before map insertion.

Added a test case to verify the collision handling in restoration.

Bug: b:513163011
Change-Id: I59f3952dff5e19e69a9475549afcf0e76a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7850539
Reviewed-by: Dan H <[email protected]>
Commit-Queue: Bryant Chandler <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1631655}
---

diff --git a/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc b/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
index b5160082..483e0ee3 100644
--- a/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
+++ b/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/glic/fre/glic_fre_controller.h"
 #include "chrome/browser/glic/glic_pref_names.h"
+#include "chrome/browser/glic/glic_tab_restore_data.h"
 #include "chrome/browser/glic/host/glic.mojom-shared.h"
 #include "chrome/browser/glic/host/glic.mojom.h"
 #include "chrome/browser/glic/host/glic_web_client_access.h"
@@ -1002,6 +1003,60 @@
 }
 
 IN_PROC_BROWSER_TEST_F(GlicInstanceCoordinatorBrowserTest,
+                       TabRestoration_ConversationIdMismatchReturnsNull) {
+  // Tab 1: Keep the instance alive.
+  CreateAndActivateTab(GURL("about:blank"));
+  ASSERT_OK_AND_ASSIGN(GlicInstanceImpl * instance, OpenGlicForActiveTab());
+  auto instance_id = instance->id();
+
+  // Set a conversation ID on the instance.
+  const std::string kConvId = "test_conversation_id";
+  auto info = mojom::ConversationInfo::New();
+  info->conversation_id = kConvId;
+  instance->RegisterConversation(std::move(info), base::DoNothing());
+
+  // Create a fake restore state for a new tab.
+  // It will have the SAME instance ID but a DIFFERENT conversation ID.
+  GlicRestoredState state;
+  state.bound_instance.instance_id = instance_id.value();
+  state.bound_instance.conversation_id = "different_conversation_id";
+  state.side_panel_open = true;  // Try to open side panel, should be skipped.
+
+  // Create a WebContents manually.
+  std::unique_ptr<content::WebContents> web_contents =
+      content::WebContents::Create(
+          content::WebContents::CreateParams(GetProfile()));
+
+  // Attach the restore data.
+  GlicTabRestoreData::CreateForWebContents(web_contents.get(),
+                                           std::move(state));
+
+  // Now add it to the tab strip.
+  auto* tab_list = GetTabListInterface();
+  tabs::TabInterface* restored_tab = nullptr;
+  {
+    GlicTestTabAddedWaiter waiter(GetProfile());
+    tab_list->InsertWebContentsAt(-1, std::move(web_contents),
+                                  /*should_pin=*/false, std::nullopt);
+    restored_tab = waiter.Wait();
+  }
+  ASSERT_TRUE(restored_tab);
+
+  // Verify that the restored tab is NOT bound to the instance.
+  EXPECT_EQ(GetInstanceForTab(restored_tab), nullptr);
+
+  // Verify that the side panel is NOT open for the new tab.
+  EXPECT_OK(WaitForSidePanelState(restored_tab,
+                                  GlicSidePanelCoordinator::State::kClosed));
+
+  // Clean up the tab we created and wait for it to be destroyed to avoid
+  // race conditions during test teardown.
+  content::WebContentsDestroyedWatcher destroyer(restored_tab->GetContents());
+  tab_list->CloseTab(restored_tab->GetHandle());
+  destroyer.Wait();
+}
+
+IN_PROC_BROWSER_TEST_F(GlicInstanceCoordinatorBrowserTest,
                        TabRestoration_SidePanelClosed) {
   // Add a new tab so we don't close the browser when we close the tab.
   auto* tab = CreateAndActivateTab(GURL("about:blank"));
diff --git a/chrome/browser/glic/service/glic_instance_coordinator_impl.cc b/chrome/browser/glic/service/glic_instance_coordinator_impl.cc
index ccc5744f..5fadcfcf 100644
--- a/chrome/browser/glic/service/glic_instance_coordinator_impl.cc
+++ b/chrome/browser/glic/service/glic_instance_coordinator_impl.cc
@@ -621,10 +621,9 @@
   ApplyMaxAwakeInstancesLimit();
 
   auto instance = CreateInstanceImpl(instance_id);
+  instance->instance_metrics().OnInstanceCreatedWithoutWarming();
   auto* instance_ptr = instance.get();
   instances_[instance->id()] = std::move(instance);
-  // TODO(harringtond): Figure out what to do about this metric.
-  instance_ptr->instance_metrics().OnInstanceCreatedWithoutWarming();
   return instance_ptr;
 }
 
@@ -716,10 +715,16 @@
 }
 
 void GlicInstanceCoordinatorImpl::RemoveInstance(GlicInstanceImpl* instance) {
-  if (!instances_.contains(instance->id())) {
+  auto it = instances_.find(instance->id());
+  if (it == instances_.end()) {
     // This instance has already been removed, so there's no work to do.
     return;
   }
+  // If an entry exists for this ID, it must be the specific instance we are
+  // removing. We prohibit overwriting instances in the map, so a mismatch
+  // would indicate a logic bug or state corruption (e.g., during restoration).
+  CHECK_EQ(it->second.get(), instance);
+
   OnInstanceActivationChanged(instance, false);
 
   // Remove the instance first, and then delete. This way,
@@ -1005,12 +1010,30 @@
     return nullptr;
   }
 
-  // Prioritize finding an existing instance by conversation ID, then by
-  // instance ID.
-  if (auto* instance =
-          !instance_info.conversation_id.empty()
-              ? GetInstanceImplForConversationId(instance_info.conversation_id)
-              : GetInstanceImplFor(instance_id)) {
+  GlicInstanceImpl* instance = nullptr;
+  if (!instance_info.conversation_id.empty()) {
+    instance = GetInstanceImplForConversationId(instance_info.conversation_id);
+    if (!instance) {
+      // If lookup by conversation ID failed, but an instance with this ID
+      // already exists, it implies an attempt to associate an existing instance
+      // with a different conversation ID. Once an instance is associated with a
+      // conversation ID, it cannot change. This indicates corrupt persisted
+      // data or a logic bug. Return nullptr to avoid dangerously overwriting
+      // the instance.
+      if (GetInstanceImplFor(instance_id)) {
+        LOG(ERROR) << "Instance restoration failed for conversation "
+                   << instance_info.conversation_id
+                   << ": The requested InstanceId " << instance_info.instance_id
+                   << " already exists but is associated with a different "
+                      "conversation.";
+        return nullptr;
+      }
+    }
+  } else {
+    instance = GetInstanceImplFor(instance_id);
+  }
+
+  if (instance) {
     return instance;
   }
 

diff --git a/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc b/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
index b5160082..483e0ee3 100644
--- a/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
+++ b/chrome/browser/glic/service/glic_instance_coordinator_browsertest.cc
@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/glic/fre/glic_fre_controller.h"
 #include "chrome/browser/glic/glic_pref_names.h"
+#include "chrome/browser/glic/glic_tab_restore_data.h"
 #include "chrome/browser/glic/host/glic.mojom-shared.h"
 #include "chrome/browser/glic/host/glic.mojom.h"
 #include "chrome/browser/glic/host/glic_web_client_access.h"
@@ -1002,6 +1003,60 @@
 }
 
 IN_PROC_BROWSER_TEST_F(GlicInstanceCoordinatorBrowserTest,
+                       TabRestoration_ConversationIdMismatchReturnsNull) {
+  // Tab 1: Keep the instance alive.
+  CreateAndActivateTab(GURL("about:blank"));
+  ASSERT_OK_AND_ASSIGN(GlicInstanceImpl * instance, OpenGlicForActiveTab());
+  auto instance_id = instance->id();
+
+  // Set a conversation ID on the instance.
+  const std::string kConvId = "test_conversation_id";
+  auto info = mojom::ConversationInfo::New();
+  info->conversation_id = kConvId;
+  instance->RegisterConversation(std::move(info), base::DoNothing());
+
+  // Create a fake restore state for a new tab.
+  // It will have the SAME instance ID but a DIFFERENT conversation ID.
+  GlicRestoredState state;
+  state.bound_instance.instance_id = instance_id.value();
+  state.bound_instance.conversation_id = "different_conversation_id";
+  state.side_panel_open = true;  // Try to open side panel, should be skipped.
+
+  // Create a WebContents manually.
+  std::unique_ptr<content::WebContents> web_contents =
+      content::WebContents::Create(
+          content::WebContents::CreateParams(GetProfile()));
+
+  // Attach the restore data.
+  GlicTabRestoreData::CreateForWebContents(web_contents.get(),
+                                           std::move(state));
+
+  // Now add it to the tab strip.
+  auto* tab_list = GetTabListInterface();
+  tabs::TabInterface* restored_tab = nullptr;
+  {
+    GlicTestTabAddedWaiter waiter(GetProfile());
+    tab_list->InsertWebContentsAt(-1, std::move(web_contents),
+                                  /*should_pin=*/false, std::nullopt);
+    restored_tab = waiter.Wait();
+  }
+  ASSERT_TRUE(restored_tab);
+
+  // Verify that the restored tab is NOT bound to the instance.
+  EXPECT_EQ(GetInstanceForTab(restored_tab), nullptr);
+
+  // Verify that the side panel is NOT open for the new tab.
+  EXPECT_OK(WaitForSidePanelState(restored_tab,
+                                  GlicSidePanelCoordinator::State::kClosed));
+
+  // Clean up the tab we created and wait for it to be destroyed to avoid
+  // race conditions during test teardown.
+  content::WebContentsDestroyedWatcher destroyer(restored_tab->GetContents());
+  tab_list->CloseTab(restored_tab->GetHandle());
+  destroyer.Wait();
+}
+
+IN_PROC_BROWSER_TEST_F(GlicInstanceCoordinatorBrowserTest,
                        TabRestoration_SidePanelClosed) {
   // Add a new tab so we don't close the browser when we close the tab.
   auto* tab = CreateAndActivateTab(GURL("about:blank"));