Don't crash when editing the title of a conversation not in-memory

and log database title because of historical bad values due to ref expiring
brave · Dec 12, 2024 · f086917 · f086917
1 parent 0347bc6
commit f086917
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 29 deletions.
diff --git a/components/ai_chat/core/browser/ai_chat_database.cc b/components/ai_chat/core/browser/ai_chat_database.cc
@@ -654,6 +654,8 @@ bool AIChatDatabase::AddConversationEntry(
 bool AIChatDatabase::UpdateConversationTitle(std::string_view conversation_uuid,
                                              std::string_view title) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DVLOG(4) << __func__ << " for " << conversation_uuid << " with title "
+           << title;
   if (!LazyInit()) {
     return false;
   }

diff --git a/components/ai_chat/core/browser/ai_chat_service.cc b/components/ai_chat/core/browser/ai_chat_service.cc
@@ -61,7 +61,8 @@ constexpr auto kAllowedSchemes = base::MakeFixedFlatSet<std::string_view>(
     {url::kHttpsScheme, url::kHttpScheme, url::kFileScheme, url::kDataScheme});
 
 std::vector<mojom::Conversation*> FilterVisibleConversations(
-    std::map<std::string, mojom::ConversationPtr>& conversations_map) {
+    std::map<std::string, mojom::ConversationPtr, std::less<>>&
+        conversations_map) {
   std::vector<mojom::Conversation*> conversations;
   for (const auto& kv : conversations_map) {
     auto& conversation = kv.second;
@@ -598,14 +599,7 @@ void AIChatService::DeleteConversation(const std::string& id) {
 
 void AIChatService::RenameConversation(const std::string& id,
                                        const std::string& new_name) {
-  ConversationHandler* conversation_handler =
-      conversation_handlers_.at(id).get();
-  if (!conversation_handler) {
-    return;
-  }
-
-  DVLOG(1) << "Renamed conversation " << id << " to '" << new_name << "'";
-  OnConversationTitleChanged(conversation_handler, new_name);
+  OnConversationTitleChanged(id, new_name);
 }
 
 void AIChatService::ConversationExists(const std::string& conversation_uuid,
@@ -814,20 +808,25 @@ void AIChatService::OnClientConnectionChanged(ConversationHandler* handler) {
   MaybeUnloadConversation(handler);
 }
 
-void AIChatService::OnConversationTitleChanged(ConversationHandler* handler,
-                                               std::string title) {
-  auto conversation_it = conversations_.find(handler->get_conversation_uuid());
-  CHECK(conversation_it != conversations_.end());
-  auto& conversation = conversation_it->second;
-  conversation->title = title;
+void AIChatService::OnConversationTitleChanged(
+    const std::string& conversation_uuid,
+    const std::string& new_title) {
+  auto conversation_it = conversations_.find(conversation_uuid);
+  if (conversation_it == conversations_.end()) {
+    DLOG(ERROR) << "Conversation not found for title change";
+    return;
+  }
+
+  auto& conversation_metadata = conversation_it->second;
+  conversation_metadata->title = new_title;
 
   OnConversationListChanged();
 
   // Persist the change
   if (ai_chat_db_) {
     ai_chat_db_
         .AsyncCall(base::IgnoreResult(&AIChatDatabase::UpdateConversationTitle))
-        .WithArgs(handler->get_conversation_uuid(), std::move(title));
+        .WithArgs(conversation_uuid, new_title);
   }
 }
 

diff --git a/components/ai_chat/core/browser/ai_chat_service.h b/components/ai_chat/core/browser/ai_chat_service.h
@@ -8,6 +8,7 @@
 
 #include <stddef.h>
 
+#include <functional>
 #include <map>
 #include <memory>
 #include <ostream>
@@ -93,8 +94,8 @@ class AIChatService : public KeyedService,
   void OnConversationEntryRemoved(ConversationHandler* handler,
                                   std::string entry_uuid) override;
   void OnClientConnectionChanged(ConversationHandler* handler) override;
-  void OnConversationTitleChanged(ConversationHandler* handler,
-                                  std::string title) override;
+  void OnConversationTitleChanged(const std::string& conversation_uuid,
+                                  const std::string& title) override;
   void OnAssociatedContentDestroyed(ConversationHandler* handler,
                                     int content_id) override;
 
@@ -175,7 +176,8 @@ class AIChatService : public KeyedService,
 
  private:
   // Key is uuid
-  using ConversationMap = std::map<std::string, mojom::ConversationPtr>;
+  using ConversationMap =
+      std::map<std::string, mojom::ConversationPtr, std::less<>>;
   using ConversationMapCallback = base::OnceCallback<void(ConversationMap&)>;
 
   void MaybeInitStorage();

diff --git a/components/ai_chat/core/browser/conversation_handler.cc b/components/ai_chat/core/browser/conversation_handler.cc
@@ -1685,7 +1685,7 @@ void ConversationHandler::OnClientConnectionChanged() {
 
 void ConversationHandler::OnConversationTitleChanged(std::string title) {
   for (auto& observer : observers_) {
-    observer.OnConversationTitleChanged(this, title);
+    observer.OnConversationTitleChanged(metadata_->uuid, title);
   }
 }
 

diff --git a/components/ai_chat/core/browser/conversation_handler.h b/components/ai_chat/core/browser/conversation_handler.h
@@ -166,8 +166,9 @@ class ConversationHandler : public mojom::ConversationHandler,
 
     // Called when a mojo client connects or disconnects
     virtual void OnClientConnectionChanged(ConversationHandler* handler) {}
-    virtual void OnConversationTitleChanged(ConversationHandler* handler,
-                                            std::string title) {}
+    virtual void OnConversationTitleChanged(
+        const std::string& conversation_uuid,
+        const std::string& title) {}
     virtual void OnSelectedLanguageChanged(
         ConversationHandler* handler,
         const std::string& selected_language) {}

diff --git a/components/ai_chat/core/browser/mock_conversation_handler_observer.h b/components/ai_chat/core/browser/mock_conversation_handler_observer.h
@@ -25,24 +25,24 @@ class MockConversationHandlerObserver : public ConversationHandler::Observer {
 
   MOCK_METHOD(void,
               OnRequestInProgressChanged,
-              (ConversationHandler * handler, bool in_progress),
+              (ConversationHandler*, bool),
               (override));
 
   MOCK_METHOD(void,
               OnConversationEntryAdded,
-              (ConversationHandler * handler,
-               mojom::ConversationTurnPtr& entry,
-               std::optional<std::string_view> associated_content_value),
+              (ConversationHandler*,
+               mojom::ConversationTurnPtr&,
+               std::optional<std::string_view>),
               (override));
 
   MOCK_METHOD(void,
               OnConversationEntryRemoved,
-              (ConversationHandler * handler, std::string turn_uuid),
+              (ConversationHandler*, std::string),
               (override));
 
   MOCK_METHOD(void,
               OnConversationEntryUpdated,
-              (ConversationHandler * handler, mojom::ConversationTurnPtr entry),
+              (ConversationHandler*, mojom::ConversationTurnPtr),
               (override));
 
   MOCK_METHOD(void,
@@ -52,7 +52,7 @@ class MockConversationHandlerObserver : public ConversationHandler::Observer {
 
   MOCK_METHOD(void,
               OnConversationTitleChanged,
-              (ConversationHandler * handler, std::string title),
+              (const std::string&, const std::string&),
               (override));
 
  private: