Make VerifyUpdatesCommand a ModelChanging variant for now as we can end

up in AttemptReuniteLostCommitResponse, change IDs, and wind up trying
to move a bookmark as a result from the wrong thread.  I haven't seen
a case where it's actually _moving_, but we still call into the BookmarkModel.
It's a no-op call except for a threading assertion.

Add check to BookmarkChangeProcessor that we're on the UI thread.

BUG=37723
TEST=VerifyUpdatesCommandUnittest (added)

Review URL: http://codereview.chromium.org/689001

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

5 files changed, 120 insertions, 6 deletions

diff --git a/chrome/browser/sync/engine/verify_updates_command.cc b/chrome/browser/sync/engine/verify_updates_command.cc
index 7671722..40fe8d6 100644
--- a/chrome/browser/sync/engine/verify_updates_command.cc
+++ b/chrome/browser/sync/engine/verify_updates_command.cc
@@ -26,7 +26,8 @@ using syncable::SYNCER;
 VerifyUpdatesCommand::VerifyUpdatesCommand() {}
 VerifyUpdatesCommand::~VerifyUpdatesCommand() {}
 
-void VerifyUpdatesCommand::ExecuteImpl(sessions::SyncSession* session) {
+void VerifyUpdatesCommand::ModelChangingExecuteImpl(
+    sessions::SyncSession* session) {
   LOG(INFO) << "Beginning Update Verification";
   ScopedDirLookup dir(session->context()->directory_manager(),
                       session->context()->account_name());
@@ -43,6 +44,11 @@ void VerifyUpdatesCommand::ExecuteImpl(sessions::SyncSession* session) {
   for (int i = 0; i < update_count; i++) {
     const SyncEntity entry =
         *reinterpret_cast<const SyncEntity *>(&(updates.entries(i)));
+    ModelSafeGroup g = GetGroupForModelType(entry.GetModelType(),
+                                            session->routing_info());
+    if (g != status->group_restriction())
+      continue;
+
     // Needs to be done separately in order to make sure the update processing
     // still happens like normal. We should really just use one type of
     // ID in fact, there isn't actually a need for server_knows and not IDs.
@@ -54,8 +60,7 @@ void VerifyUpdatesCommand::ExecuteImpl(sessions::SyncSession* session) {
 
     VerifyUpdateResult result = VerifyUpdate(&trans, entry,
                                              session->routing_info());
-    status->GetUnrestrictedUpdateProgress(
-        result.placement)->AddVerifyResult(result.value, entry);
+    status->mutable_update_progress()->AddVerifyResult(result.value, entry);
   }
 }
 
diff --git a/chrome/browser/sync/engine/verify_updates_command.h b/chrome/browser/sync/engine/verify_updates_command.h
index 4529680..53c79e1 100644
--- a/chrome/browser/sync/engine/verify_updates_command.h
+++ b/chrome/browser/sync/engine/verify_updates_command.h
@@ -8,7 +8,7 @@
 #include "base/basictypes.h"
 
 #include "chrome/browser/sync/engine/model_safe_worker.h"
-#include "chrome/browser/sync/engine/syncer_command.h"
+#include "chrome/browser/sync/engine/model_changing_syncer_command.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/engine/syncer_types.h"
 
@@ -20,13 +20,13 @@ namespace browser_sync {
 
 // Verifies the response from a GetUpdates request. All invalid updates will be
 // noted in the SyncSession after this command is executed.
-class VerifyUpdatesCommand : public SyncerCommand {
+class VerifyUpdatesCommand : public ModelChangingSyncerCommand {
  public:
   VerifyUpdatesCommand();
   virtual ~VerifyUpdatesCommand();
 
   // SyncerCommand implementation.
-  virtual void ExecuteImpl(sessions::SyncSession* session);
+  virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
 
  private:
   struct VerifyUpdateResult {
diff --git a/chrome/browser/sync/engine/verify_updates_command_unittest.cc b/chrome/browser/sync/engine/verify_updates_command_unittest.cc
new file mode 100644
index 0000000..b8693b4
--- /dev/null
+++ b/chrome/browser/sync/engine/verify_updates_command_unittest.cc
@@ -0,0 +1,107 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/engine/verify_updates_command.h"
+#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
+#include "chrome/browser/sync/sessions/sync_session.h"
+#include "chrome/browser/sync/syncable/directory_manager.h"
+#include "chrome/browser/sync/engine/mock_model_safe_workers.h"
+#include "chrome/browser/sync/syncable/syncable.h"
+#include "chrome/browser/sync/syncable/syncable_id.h"
+#include "chrome/test/sync/engine/syncer_command_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_sync {
+
+using sessions::SyncSession;
+using sessions::StatusController;
+using std::string;
+using syncable::Entry;
+using syncable::Id;
+using syncable::MutableEntry;
+using syncable::ReadTransaction;
+using syncable::ScopedDirLookup;
+using syncable::UNITTEST;
+using syncable::WriteTransaction;
+
+class VerifyUpdatesCommandTest : public SyncerCommandTest {
+ public:
+  virtual void SetUp() {
+    workers()->clear();
+    mutable_routing_info()->clear();
+    workers()->push_back(new MockDBModelWorker());
+    workers()->push_back(new MockUIModelWorker());
+    (*mutable_routing_info())[syncable::PREFERENCES] = GROUP_UI;
+    (*mutable_routing_info())[syncable::BOOKMARKS] = GROUP_UI;
+    (*mutable_routing_info())[syncable::AUTOFILL] = GROUP_DB;
+    SyncerCommandTest::SetUp();
+  }
+
+  void CreateLocalItem(const std::string& item_id,
+                       const std::string& parent_id,
+                       const syncable::ModelType& type) {
+    ScopedDirLookup dir(syncdb().manager(), syncdb().name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__);
+    MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM,
+        Id::CreateFromServerId(item_id));
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::BASE_VERSION, 1);
+    entry.Put(syncable::SERVER_VERSION, 1);
+    entry.Put(syncable::NON_UNIQUE_NAME, item_id);
+    entry.Put(syncable::PARENT_ID, Id::CreateFromServerId(parent_id));
+    sync_pb::EntitySpecifics default_specifics;
+    AddDefaultExtensionValue(type, &default_specifics);
+    entry.Put(syncable::SERVER_SPECIFICS, default_specifics);
+  }
+
+  void AddUpdate(GetUpdatesResponse* updates,
+      const std::string& id, const std::string& parent,
+      const syncable::ModelType& type) {
+    sync_pb::SyncEntity* e = updates->add_entries();
+    e->set_id_string("b1");
+    e->set_parent_id_string(parent);
+    e->set_non_unique_name("b1");
+    e->set_name("b1");
+    AddDefaultExtensionValue(type, e->mutable_specifics());
+  }
+
+  VerifyUpdatesCommand command_;
+
+};
+
+TEST_F(VerifyUpdatesCommandTest, AllVerified) {
+  string root = syncable::kNullId.GetServerId();
+
+  CreateLocalItem("b1", root, syncable::BOOKMARKS);
+  CreateLocalItem("b2", root, syncable::BOOKMARKS);
+  CreateLocalItem("p1", root, syncable::PREFERENCES);
+  CreateLocalItem("a1", root, syncable::AUTOFILL);
+
+  GetUpdatesResponse* updates = session()->status_controller()->
+      mutable_updates_response()->mutable_get_updates();
+  AddUpdate(updates, "b1", root, syncable::BOOKMARKS);
+  AddUpdate(updates, "b2", root, syncable::BOOKMARKS);
+  AddUpdate(updates, "p1", root, syncable::PREFERENCES);
+  AddUpdate(updates, "a1", root, syncable::AUTOFILL);
+
+  command_.ExecuteImpl(session());
+
+  StatusController* status = session()->status_controller();
+  {
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+    EXPECT_EQ(3, status->update_progress().VerifiedUpdatesSize());
+  }
+  {
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_DB);
+    EXPECT_EQ(1, status->update_progress().VerifiedUpdatesSize());
+  }
+  {
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
+    EXPECT_EQ(0, status->update_progress().VerifiedUpdatesSize());
+  }
+}
+
+}
diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc
index 4b7d9d6..94a7e6d 100644
--- a/chrome/browser/sync/glue/bookmark_change_processor.cc
+++ b/chrome/browser/sync/glue/bookmark_change_processor.cc
@@ -344,6 +344,7 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel(
     int change_count) {
   if (!running())
     return;
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   // A note about ordering.  Sync backend is responsible for ordering the change
   // records in the following order:
   //
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 908aeec..00ec96e 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1535,6 +1535,7 @@
         'browser/sync/engine/syncer_thread_unittest.cc',
         'browser/sync/engine/syncer_unittest.cc',
         'browser/sync/engine/syncproto_unittest.cc',
+        'browser/sync/engine/verify_updates_command_unittest.cc',
         'browser/sync/glue/bookmark_data_type_controller_unittest.cc',
         'browser/sync/glue/change_processor_mock.h',
         'browser/sync/glue/preference_data_type_controller_unittest.cc',