[Sync] Make BookmarkChangeProcessor resilient against updates before add

Other bookmarks observer might make their own changes on add, triggering updates
before the add call arrives. Be resilient against this.

BUG=369728

Review URL: https://codereview.chromium.org/277033002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269645 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/glue/bookmark_change_processor.cc

@@ -205,29 +205,27 @@ void BookmarkChangeProcessor::RemoveAllChildNodes(
   }
 }
 
-void BookmarkChangeProcessor::BookmarkModelLoaded(BookmarkModel* model,
-                                                  bool ids_reassigned) {
-  NOTREACHED();
-}
-
-void BookmarkChangeProcessor::BookmarkModelBeingDeleted(
-    BookmarkModel* model) {
-  NOTREACHED();
-  bookmark_model_ = NULL;
-}
-
-void BookmarkChangeProcessor::BookmarkNodeAdded(BookmarkModel* model,
-                                                const BookmarkNode* parent,
-                                                int index) {
-  DCHECK(share_handle());
+void BookmarkChangeProcessor::CreateOrUpdateSyncNode(const BookmarkNode* node) {
+  const BookmarkNode* parent = node->parent();
+  int index = node->parent()->GetIndexOf(node);
 
   int64 new_version = syncer::syncable::kInvalidTransactionVersion;
   int64 sync_id = syncer::kInvalidId;
   {
     // Acquire a scoped write lock via a transaction.
     syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
-    sync_id = CreateSyncNode(parent, model, index, &trans,
-                             model_associator_, error_handler());
+    sync_id = model_associator_->GetSyncIdFromChromeId(node->id());
+    if (sync_id != syncer::kInvalidId) {
+      UpdateSyncNode(
+          node, bookmark_model_, &trans, model_associator_, error_handler());
+    } else {
+      sync_id = CreateSyncNode(parent,
+                               bookmark_model_,
+                               index,
+                               &trans,
+                               model_associator_,
+                               error_handler());
+    }
   }
 
   if (syncer::kInvalidId != sync_id) {
@@ -236,11 +234,29 @@ void BookmarkChangeProcessor::BookmarkNodeAdded(BookmarkModel* model,
     // of added node here. After switching to ordinals for positioning,
     // PREV_ID/NEXT_ID will be deprecated and siblings will not be updated.
     UpdateTransactionVersion(
-        new_version, model,
+        new_version,
+        bookmark_model_,
         std::vector<const BookmarkNode*>(1, parent->GetChild(index)));
   }
 }
 
+void BookmarkChangeProcessor::BookmarkModelLoaded(BookmarkModel* model,
+                                                  bool ids_reassigned) {
+  NOTREACHED();
+}
+
+void BookmarkChangeProcessor::BookmarkModelBeingDeleted(BookmarkModel* model) {
+  NOTREACHED();
+  bookmark_model_ = NULL;
+}
+
+void BookmarkChangeProcessor::BookmarkNodeAdded(BookmarkModel* model,
+                                                const BookmarkNode* parent,
+                                                int index) {
+  DCHECK(share_handle());
+  CreateOrUpdateSyncNode(parent->GetChild(index));
+}
+
 // static
 int64 BookmarkChangeProcessor::CreateSyncNode(const BookmarkNode* parent,
     BookmarkModel* model, int index, syncer::WriteTransaction* trans,
@@ -290,77 +306,29 @@ void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model,
     NOTREACHED() << "Saw update to permanent node!";
     return;
   }
+  CreateOrUpdateSyncNode(node);
+}
 
-  int64 new_version = syncer::syncable::kInvalidTransactionVersion;
-  {
-    // Acquire a scoped write lock via a transaction.
-    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
-
-    // Lookup the sync node that's associated with |node|.
-    syncer::WriteNode sync_node(&trans);
-    if (!model_associator_->InitSyncNodeFromChromeId(node->id(), &sync_node)) {
-      // TODO(tim): Investigating bug 121587.
-      if (model_associator_->GetSyncIdFromChromeId(node->id()) ==
-                                                   syncer::kInvalidId) {
-        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Bookmark id not found in model associator on BookmarkNodeChanged");
-        LOG(ERROR) << "Bad id.";
-      } else if (!sync_node.GetEntry()->good()) {
-        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, good() failed");
-        LOG(ERROR) << "Bad entry.";
-      } else if (sync_node.GetEntry()->GetIsDel()) {
-        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, is_del true");
-        LOG(ERROR) << "Deleted entry.";
-      } else {
-        syncer::Cryptographer* crypto = trans.GetCryptographer();
-        syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
-        const sync_pb::EntitySpecifics& specifics =
-            sync_node.GetEntry()->GetSpecifics();
-        CHECK(specifics.has_encrypted());
-        const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
-        const bool agreement = encrypted_types.Has(syncer::BOOKMARKS);
-        if (!agreement && !can_decrypt) {
-          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-              "Could not InitByIdLookup on BookmarkNodeChanged, "
-              " Cryptographer thinks bookmarks not encrypted, and CanDecrypt"
-              " failed.");
-          LOG(ERROR) << "Case 1.";
-        } else if (agreement && can_decrypt) {
-          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-              "Could not InitByIdLookup on BookmarkNodeChanged, "
-              " Cryptographer thinks bookmarks are encrypted, and CanDecrypt"
-              " succeeded (?!), but DecryptIfNecessary failed.");
-          LOG(ERROR) << "Case 2.";
-        } else if (agreement) {
-          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-              "Could not InitByIdLookup on BookmarkNodeChanged, "
-              " Cryptographer thinks bookmarks are encrypted, but CanDecrypt"
-              " failed.");
-          LOG(ERROR) << "Case 3.";
-        } else {
-          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-              "Could not InitByIdLookup on BookmarkNodeChanged, "
-              " Cryptographer thinks bookmarks not encrypted, but CanDecrypt"
-              " succeeded (super weird, btw)");
-          LOG(ERROR) << "Case 4.";
-        }
-      }
-      return;
-    }
-
-    UpdateSyncNodeProperties(node, model, &sync_node);
-
-    DCHECK_EQ(sync_node.GetIsFolder(), node->is_folder());
-    DCHECK_EQ(model_associator_->GetChromeNodeFromSyncId(
-              sync_node.GetParentId()),
-              node->parent());
-    DCHECK_EQ(node->parent()->GetIndexOf(node), sync_node.GetPositionIndex());
+// Static.
+int64 BookmarkChangeProcessor::UpdateSyncNode(
+    const BookmarkNode* node,
+    BookmarkModel* model,
+    syncer::WriteTransaction* trans,
+    BookmarkModelAssociator* associator,
+    DataTypeErrorHandler* error_handler) {
+  // Lookup the sync node that's associated with |node|.
+  syncer::WriteNode sync_node(trans);
+  if (!associator->InitSyncNodeFromChromeId(node->id(), &sync_node)) {
+    error_handler->OnSingleDatatypeUnrecoverableError(
+        FROM_HERE, "Could not load bookmark node on update.");
+    return syncer::kInvalidId;
   }
-
-  UpdateTransactionVersion(new_version, model,
-                           std::vector<const BookmarkNode*>(1, node));
+  UpdateSyncNodeProperties(node, model, &sync_node);
+  DCHECK_EQ(sync_node.GetIsFolder(), node->is_folder());
+  DCHECK_EQ(associator->GetChromeNodeFromSyncId(sync_node.GetParentId()),
+            node->parent());
+  DCHECK_EQ(node->parent()->GetIndexOf(node), sync_node.GetPositionIndex());
+  return sync_node.GetId();
 }
 
 void BookmarkChangeProcessor::BookmarkMetaInfoChanged(

chrome/browser/sync/glue/bookmark_change_processor.h

@@ -133,6 +133,13 @@ class BookmarkChangeProcessor : public BookmarkModelObserver,
                               BookmarkModelAssociator* associator,
                               DataTypeErrorHandler* error_handler);
 
+  // Update |bookmark_node|'s sync node.
+  static int64 UpdateSyncNode(const BookmarkNode* bookmark_node,
+                              BookmarkModel* model,
+                              syncer::WriteTransaction* trans,
+                              BookmarkModelAssociator* associator,
+                              DataTypeErrorHandler* error_handler);
+
   // Update transaction version of |model| and |nodes| to |new_version| if
   // it's valid.
   static void UpdateTransactionVersion(
@@ -196,6 +203,9 @@ class BookmarkChangeProcessor : public BookmarkModelObserver,
   // Remove all the sync nodes associated with |node| and its children.
   void RemoveSyncNodeHierarchy(const BookmarkNode* node);
 
+  // Creates or updates a sync node associated with |node|.
+  void CreateOrUpdateSyncNode(const BookmarkNode* node);
+
   // The bookmark model we are processing changes from.  Non-NULL when
   // |running_| is true.
   BookmarkModel* bookmark_model_;

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

@@ -725,6 +725,7 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
   }
 
  protected:
+  TestingProfile profile_;
   BookmarkModel* model_;
   syncer::TestUserShare test_user_share_;
   scoped_ptr<BookmarkChangeProcessor> change_processor_;
@@ -740,8 +741,6 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
 
   syncer::SyncMergeResult local_merge_result_;
   syncer::SyncMergeResult syncer_merge_result_;
-
-  TestingProfile profile_;
 };
 
 TEST_F(ProfileSyncServiceBookmarkTest, InitialState) {
@@ -2136,6 +2135,39 @@ TEST_F(ProfileSyncServiceBookmarkTestWithData, PersistenceError) {
   EXPECT_FALSE(AssociateModels());
 }
 
+// It's possible for update/add calls from the bookmark model to be out of
+// order, or asynchronous. Handle that without triggering an error.
+TEST_F(ProfileSyncServiceBookmarkTest, UpdateThenAdd) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  StartSync();
+
+  EXPECT_TRUE(other_bookmarks_id());
+  EXPECT_TRUE(bookmark_bar_id());
+  EXPECT_TRUE(mobile_bookmarks_id());
+
+  ExpectModelMatch();
+
+  // Now destroy the change processor then add a bookmark, to simulate
+  // missing the Update call.
+  change_processor_.reset();
+  const BookmarkNode* node = model_->AddURL(model_->bookmark_bar_node(),
+                                            0,
+                                            base::ASCIIToUTF16("title"),
+                                            GURL("http://www.url.com"));
+
+  // Recreate the change processor then update that bookmark. Sync should
+  // receive the update call and gracefully treat that as if it were an add.
+  change_processor_.reset(new BookmarkChangeProcessor(
+      &profile_, model_associator_.get(), &mock_error_handler_));
+  change_processor_->Start(test_user_share_.user_share());
+  model_->SetTitle(node, base::ASCIIToUTF16("title2"));
+  ExpectModelMatch();
+
+  // Then simulate the add call arriving late.
+  change_processor_->BookmarkNodeAdded(model_, model_->bookmark_bar_node(), 0);
+  ExpectModelMatch();
+}
+
 }  // namespace
 
 }  // namespace browser_sync