Merge pull request #4687 from brave/pr4588_bookmarks_model_not_loaded…

…_1.5.x

Bookmarks model not loaded (uplift to 1.5.x)

chromium_src/components/bookmarks/browser/bookmark_model.cc

@@ -10,7 +10,9 @@ namespace bookmarks {
 // Move bookmarks under "Other Bookmarks" permanent node to a same name folder
 // at the end of "Bookmark Bar" permanent node
 void BraveMigrateOtherNode(BookmarkModel* model) {
-  DCHECK(model);
+  CHECK(model);
+  CHECK(model->loaded());
+  // Model must be loaded at this point
   if (!model->other_node()->children().empty()) {
     const bookmarks::BookmarkNode* new_other_node =
         model->AddFolder(model->bookmark_bar_node(),

chromium_src/components/bookmarks/browser/bookmark_model.h

@@ -6,6 +6,12 @@
 #ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_MODEL_H_
 #define BRAVE_CHROMIUM_SRC_COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_MODEL_H_
 
+class BraveSyncServiceTestDelayedLoadModel;
+
+#define BRAVE_BOOKMARK_MODEL_H \
+ private: \
+  friend class ::BraveSyncServiceTestDelayedLoadModel;
+
 #include "../../../../../components/bookmarks/browser/bookmark_model.h"
 
 namespace bookmarks {

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -200,11 +200,19 @@ BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile,
                  base::Unretained(this)));
 
   model_ = BookmarkModelFactory::GetForBrowserContext(profile);
+  // model_ can be null in some tests
 
   network_connection_tracker_->AddNetworkConnectionObserver(this);
   RecordSyncStateP3A();
 }
 
+void BraveProfileSyncServiceImpl::BookmarkModelLoaded(BookmarkModel* model,
+                                                      bool ids_reassigned) {
+  VLOG(2) << "[BraveSync] bookmarks model just loaded, "
+          << "resuming pending sync ready callback";
+  OnSyncReadyBookmarksModelLoaded();
+}
+
 void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) {
   if (!brave_sync_prefs_->GetSyncEnabled())
     return;
@@ -219,6 +227,12 @@ void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) {
 
 BraveProfileSyncServiceImpl::~BraveProfileSyncServiceImpl() {
   network_connection_tracker_->RemoveNetworkConnectionObserver(this);
+  // Tests which use ProfileSyncService and are not configured to run on UI
+  // thread, fire DCHECK on BookmarkModel::RemoveObserver at a wrong sequence.
+  // Remove observer only if we have set it.
+  if (is_model_loaded_observer_set_) {
+      model_->RemoveObserver(this);
+  }
 }
 
 void BraveProfileSyncServiceImpl::OnSetupSyncHaveCode(
@@ -470,6 +484,19 @@ void BraveProfileSyncServiceImpl::OnSyncReady() {
   DCHECK(false == brave_sync_ready_);
   brave_sync_ready_ = true;
 
+  DCHECK(model_);
+  if (model_->loaded()) {
+    OnSyncReadyBookmarksModelLoaded();
+  } else {
+    // Will call OnSyncReadyBookmarksModelLoaded once model is loaded
+    VLOG(2) << "[BraveSync] bookmarks model is not yet loaded, "
+            << "OnSyncReady will be delayed";
+    model_->AddObserver(this);
+    is_model_loaded_observer_set_ = true;
+  }
+}
+
+void BraveProfileSyncServiceImpl::OnSyncReadyBookmarksModelLoaded() {
   // For launching from legacy sync profile and also brand new profile
   if (brave_sync_prefs_->GetMigratedBookmarksVersion() < 2)
     SetPermanentNodesOrder(brave_sync_prefs_->GetBookmarksBaseOrder());
@@ -525,6 +552,7 @@ void BraveProfileSyncServiceImpl::OnGetExistingObjects(
   // TODO(bridiver) - what do we do with is_truncated ?
   // It appears to be ignored in b-l
   if (category_name == kBookmarks) {
+    DCHECK(model_->loaded());
     if (!IsTimeEmpty(last_record_time_stamp)) {
       brave_sync_prefs_->SetLatestRecordTime(last_record_time_stamp);
     }
@@ -680,6 +708,7 @@ void BraveProfileSyncServiceImpl::ResetSyncInternal() {
 void BraveProfileSyncServiceImpl::SetPermanentNodesOrder(
     const std::string& base_order) {
   DCHECK(model_);
+  DCHECK(model_->loaded());
   DCHECK(!base_order.empty());
   std::string order;
   model_->bookmark_bar_node()->GetMetaInfo("order", &order);
@@ -786,6 +815,8 @@ void BraveProfileSyncServiceImpl::LoadSyncEntityInfo(
 void BraveProfileSyncServiceImpl::CreateResolveList(
     const std::vector<std::unique_ptr<SyncRecord>>& records,
     SyncRecordAndExistingList* records_and_existing_objects) {
+  DCHECK(model_);
+  DCHECK(model_->loaded());
   const auto& this_device_id = brave_sync_prefs_->GetThisDeviceId();
   for (const auto& record : records) {
     // Ignore records from ourselves to avoid mess on merge
@@ -1038,8 +1069,11 @@ BraveSyncService* BraveProfileSyncServiceImpl::GetSyncService() const {
 void BraveProfileSyncServiceImpl::SendSyncRecords(
     const std::string& category_name,
     RecordsListPtr records) {
+  DCHECK(brave_sync_client_);
+  DCHECK(model_->loaded());
   brave_sync_client_->SendSyncRecords(category_name, *records);
   if (category_name == kBookmarks) {
+    DCHECK(model_->loaded());
     for (auto& record : *records) {
       SaveSyncEntityInfo(record.get());
       std::unique_ptr<base::DictionaryValue> meta =
@@ -1060,6 +1094,10 @@ void BraveProfileSyncServiceImpl::ResendSyncRecords(
         brave_sync_prefs_->GetRecordsToResend();
     if (records_to_resend.empty())
       return;
+
+    DCHECK(model_);
+    DCHECK(model_->loaded());
+
     for (auto& object_id : records_to_resend) {
       auto* node = FindByObjectId(model_, object_id);
 

components/brave_sync/brave_profile_sync_service_impl.h

@@ -7,6 +7,7 @@
 #define BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_PROFILE_SYNC_SERVICE_IMPL_H_
 
 #include <memory>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -15,6 +16,7 @@
 #include "brave/components/brave_sync/jslib_messages_fwd.h"
 #include "brave/components/brave_sync/public/brave_profile_sync_service.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "services/network/public/cpp/network_connection_tracker.h"
@@ -49,19 +51,30 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetThisDeviceCreatedTime);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, InitialFetchesStartWithZero);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2Migration);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId);
+FORWARD_DECLARE_TEST(BraveSyncServiceTestDelayedLoadModel,
+                     OnSyncReadyModelNotYetLoaded);
 
 class BraveSyncServiceTest;
 
+namespace bookmarks {
+class BookmarkModel;
+class BookmarkNode;
+}  // namespace bookmarks
+
 namespace brave_sync {
 namespace prefs {
 class Prefs;
 }  // namespace prefs
 
+using bookmarks::BookmarkModel;
+using bookmarks::BookmarkNode;
+
 class BraveProfileSyncServiceImpl
     : public BraveProfileSyncService,
       public BraveSyncService,
       public network::NetworkConnectionTracker::NetworkConnectionObserver,
-      public SyncMessageHandler {
+      public SyncMessageHandler,
+      public bookmarks::BookmarkModelObserver {
  public:
   explicit BraveProfileSyncServiceImpl(Profile* profile,
                                        InitParams init_params);
@@ -116,10 +129,43 @@ class BraveProfileSyncServiceImpl
   // NetworkConnectionTracker::NetworkConnectionObserver implementation.
   void OnConnectionChanged(network::mojom::ConnectionType type) override;
 
-  // KeyedService implementation.  This must be called exactly
+  // KeyedService implementation. This must be called exactly
   // once (before this object is destroyed).
   void Shutdown() override;
 
+  // bookmarks::BookmarkModelObserver implementation
+  void BookmarkModelLoaded(BookmarkModel* model, bool ids_reassigned) override;
+
+  void BookmarkNodeMoved(BookmarkModel* model,
+                         const BookmarkNode* old_parent,
+                         size_t old_index,
+                         const BookmarkNode* new_parent,
+                         size_t new_index) override {}
+
+  void BookmarkNodeAdded(BookmarkModel* model,
+                         const BookmarkNode* parent,
+                         size_t index) override {}
+
+  void BookmarkNodeRemoved(
+      BookmarkModel* model,
+      const BookmarkNode* parent,
+      size_t old_index,
+      const BookmarkNode* node,
+      const std::set<GURL>& no_longer_bookmarked) override {}
+
+  void BookmarkNodeChanged(BookmarkModel* model,
+                           const BookmarkNode* node) override {}
+
+  void BookmarkNodeFaviconChanged(BookmarkModel* model,
+                                  const BookmarkNode* node) override {}
+
+  void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                     const BookmarkNode* node) override {}
+
+  void BookmarkAllUserNodesRemoved(
+      BookmarkModel* model,
+      const std::set<GURL>& removed_urls) override {}
+
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   BraveSyncClient* GetBraveSyncClient() override;
 #endif
@@ -168,6 +214,8 @@ class BraveProfileSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, DeviceIdV2Migration);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest,
                            DeviceIdV2MigrationDupDeviceId);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTestDelayedLoadModel,
+                           OnSyncReadyModelNotYetLoaded);
 
   friend class ::BraveSyncServiceTest;
 
@@ -215,6 +263,9 @@ class BraveProfileSyncServiceImpl
 
   bool IsSQSReady() const;
 
+  // Method to be run right after bookmark model will be loaded
+  void OnSyncReadyBookmarksModelLoaded();
+
   static base::TimeDelta GetRetryExponentialWaitAmount(int retry_number);
   static std::vector<unsigned> GetExponentialWaitsForTests();
   static const std::vector<unsigned> kExponentialWaits;
@@ -255,6 +306,8 @@ class BraveProfileSyncServiceImpl
 
   bool pending_self_reset_ = false;
 
+  bool is_model_loaded_observer_set_ = false;
+
   // Used to ensure that certain operations are performed on the sequence that
   // this object was created on.
   SEQUENCE_CHECKER(sequence_checker_);

components/brave_sync/brave_sync_service_unittest.cc

@@ -185,12 +185,7 @@ class BraveSyncServiceTest : public testing::Test {
 
     sync_prefs_ = std::make_unique<syncer::SyncPrefs>(profile_->GetPrefs());
 
-    // TODO(bridiver) - this is temporary until some changes are made to
-    // to bookmark_change_processor to allow `set_for_testing` like
-    // BraveSyncClient
-    BookmarkModelFactory::GetInstance()->SetTestingFactory(
-        profile(),
-        base::BindRepeating(&brave_sync::BuildFakeBookmarkModelForTests));
+    SetBookmarkModelFactory();
 
     model_ = BookmarkModelFactory::GetForBrowserContext(
         Profile::FromBrowserContext(profile_.get()));
@@ -218,6 +213,15 @@ class BraveSyncServiceTest : public testing::Test {
     profile_.reset();
   }
 
+  virtual void SetBookmarkModelFactory() {
+    // TODO(bridiver) - this is temporary until some changes are made to
+    // to bookmark_change_processor to allow `set_for_testing` like
+    // BraveSyncClient
+    BookmarkModelFactory::GetInstance()->SetTestingFactory(
+        profile(),
+        base::BindRepeating(&brave_sync::BuildFakeBookmarkModelForTests));
+  }
+
   Profile* profile() { return profile_.get(); }
   BraveProfileSyncServiceImpl* sync_service() { return sync_service_; }
   MockBraveSyncClient* sync_client() { return sync_client_; }
@@ -246,6 +250,37 @@ class BraveSyncServiceTest : public testing::Test {
   base::ScopedTempDir temp_dir_;
 };
 
+class BraveSyncServiceTestDelayedLoadModel : public BraveSyncServiceTest {
+ public:
+  void SetBookmarkModelFactory() override {
+    BookmarkModelFactory::GetInstance()->SetTestingFactory(
+        profile(),
+        base::BindRepeating(&BraveSyncServiceTestDelayedLoadModel::BuildModel,
+                            base::Unretained(this)));
+  }
+  void ModelDoneLoading() {
+    model()->DoneLoading(std::move(bookmark_load_details_));
+  }
+
+ private:
+  std::unique_ptr<KeyedService> BuildModel(content::BrowserContext* context) {
+    using bookmarks::BookmarkClient;
+    using bookmarks::BookmarkLoadDetails;
+    using bookmarks::BookmarkModel;
+    using bookmarks::TestBookmarkClient;
+    std::unique_ptr<TestBookmarkClient> client(new TestBookmarkClient());
+    BookmarkClient* client_ptr = client.get();
+    std::unique_ptr<BookmarkModel> bookmark_model(
+        new BookmarkModel(std::move(client)));
+    bookmark_load_details_ = std::make_unique<BookmarkLoadDetails>(client_ptr);
+    bookmark_load_details_->LoadManagedNode();
+    bookmark_load_details_->CreateUrlIndex();
+    // By intent don't call BookmarkModel::DoneLoading
+    return bookmark_model;
+  }
+  std::unique_ptr<bookmarks::BookmarkLoadDetails> bookmark_load_details_;
+};
+
 TEST_F(BraveSyncServiceTest, SetSyncEnabled) {
   EXPECT_CALL(*sync_client(), OnSyncEnabledChanged);
   EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
@@ -826,6 +861,22 @@ TEST_F(BraveSyncServiceTest, OnSyncReadyNewToSync) {
   EXPECT_TRUE(profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks));
 }
 
+TEST_F(BraveSyncServiceTestDelayedLoadModel, OnSyncReadyModelNotYetLoaded) {
+  EXPECT_NE(sync_service()->model_, nullptr);
+  EXPECT_FALSE(model()->loaded());
+  profile()->GetPrefs()->SetString(brave_sync::prefs::kSyncBookmarksBaseOrder,
+                                   "1.1.");
+
+  EXPECT_FALSE(sync_service()->is_model_loaded_observer_set_);
+  sync_service()->OnSyncReady();
+  EXPECT_TRUE(sync_service()->is_model_loaded_observer_set_);
+  EXPECT_CALL(*observer(), OnSyncStateChanged);
+  ModelDoneLoading();
+
+  // This pref is enabled in observer, check wether the observer job done
+  EXPECT_TRUE(profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks));
+}
+
 TEST_F(BraveSyncServiceTest, OnGetExistingObjects) {
   EXPECT_CALL(*sync_client(), SendResolveSyncRecords).Times(1);
 

patches/components-bookmarks-browser-bookmark_model.h.patch

@@ -0,0 +1,12 @@
+diff --git a/components/bookmarks/browser/bookmark_model.h b/components/bookmarks/browser/bookmark_model.h
+index ae1ef2997adabedb362c525b58c724a9114899b9..d1d4d7091b83d3cb31740a1ee4d22c24b15fcad0 100644
+--- a/components/bookmarks/browser/bookmark_model.h
++++ b/components/bookmarks/browser/bookmark_model.h
+@@ -324,6 +324,7 @@ class BookmarkModel : public BookmarkUndoProvider,
+     return weak_factory_.GetWeakPtr();
+   }
+
++  BRAVE_BOOKMARK_MODEL_H
+  private:
+   friend class BookmarkCodecTest;
+   friend class BookmarkModelFaviconTest;