From d636be3840b26f0b886d2313c810c1f9057bbfdd Mon Sep 17 00:00:00 2001 From: "tim@chromium.org" Date: Fri, 22 Jan 2010 02:15:42 +0000 Subject: Support for multiple sync ModelSafeWorkers. - Introduce an equivalence class enum to group sync model types that live on the same chrome native model together. - Remove ModelSafeWorkerBridge as it is no longer needed. - Rename BookmarkModelWorker -> UIModelWorker, and make it use the new stuff. - Allow syncable entries belonging to an "unsynced" model type to be just processed "passively" from the SyncerThread (rather than dispatching), and allow this to change as a result of enable/disabling. - Experimenting with a way to complete issue 31909 (the CLASS_UNASSOCIATED stuff). BUG=31925,31911,31909 Review URL: http://codereview.chromium.org/553015 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36835 0039d316-1c4b-4281-b951-d872f2087c98 --- .../sync/engine/apply_updates_command_unittest.cc | 9 +- .../sync/engine/model_changing_syncer_command.cc | 10 +- chrome/browser/sync/engine/model_safe_worker.h | 55 ++++- .../browser/sync/engine/process_updates_command.cc | 5 +- chrome/browser/sync/engine/syncapi.cc | 67 +------ chrome/browser/sync/engine/syncapi.h | 34 +--- .../browser/sync/engine/syncer_thread_unittest.cc | 18 +- chrome/browser/sync/engine/syncer_types.h | 5 - chrome/browser/sync/engine/syncer_unittest.cc | 20 +- chrome/browser/sync/engine/syncer_util.cc | 8 +- chrome/browser/sync/engine/syncer_util.h | 7 +- .../browser/sync/engine/verify_updates_command.cc | 2 +- chrome/browser/sync/glue/bookmark_model_worker.cc | 107 ---------- chrome/browser/sync/glue/bookmark_model_worker.h | 138 ------------- .../sync/glue/bookmark_model_worker_unittest.cc | 221 -------------------- chrome/browser/sync/glue/sync_backend_host.cc | 68 ++++++- chrome/browser/sync/glue/sync_backend_host.h | 56 ++++-- chrome/browser/sync/glue/ui_model_worker.cc | 106 ++++++++++ chrome/browser/sync/glue/ui_model_worker.h | 139 +++++++++++++ .../browser/sync/glue/ui_model_worker_unittest.cc | 222 +++++++++++++++++++++ chrome/browser/sync/sessions/sync_session.cc | 17 ++ chrome/browser/sync/sessions/sync_session.h | 34 ++++ .../browser/sync/sessions/sync_session_context.h | 15 +- .../browser/sync/sessions/sync_session_unittest.cc | 11 +- chrome/browser/sync/syncable/model_type.h | 27 +++ chrome/chrome.gyp | 1 + chrome/chrome_browser.gypi | 6 +- chrome/chrome_tests.gypi | 2 +- 28 files changed, 780 insertions(+), 630 deletions(-) delete mode 100644 chrome/browser/sync/glue/bookmark_model_worker.cc delete mode 100644 chrome/browser/sync/glue/bookmark_model_worker.h delete mode 100644 chrome/browser/sync/glue/bookmark_model_worker_unittest.cc create mode 100644 chrome/browser/sync/glue/ui_model_worker.cc create mode 100644 chrome/browser/sync/glue/ui_model_worker.h create mode 100644 chrome/browser/sync/glue/ui_model_worker_unittest.cc create mode 100644 chrome/browser/sync/syncable/model_type.h (limited to 'chrome') diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc index c9f7c99..ed6811f 100755 --- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc +++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc @@ -25,7 +25,8 @@ using sessions::SyncSession; // A test fixture for tests exercising ApplyUpdatesCommand. class ApplyUpdatesCommandTest : public testing::Test, - public SyncSession::Delegate { + public SyncSession::Delegate, + public ModelSafeWorkerRegistrar { public: // SyncSession::Delegate implementation. virtual void OnSilencedUntil(const base::TimeTicks& silenced_until) { @@ -44,12 +45,16 @@ class ApplyUpdatesCommandTest : public testing::Test, FAIL() << "Should not get poll interval update."; } + // ModelSafeWorkerRegistrar implementation. + virtual void GetWorkers(std::vector* out) {} + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {} + protected: ApplyUpdatesCommandTest() : next_revision_(1) {} virtual ~ApplyUpdatesCommandTest() {} virtual void SetUp() { syncdb_.SetUp(); - context_.reset(new SyncSessionContext(NULL, syncdb_.manager(), NULL)); + context_.reset(new SyncSessionContext(NULL, syncdb_.manager(), this)); context_->set_account_name(syncdb_.name()); } virtual void TearDown() { diff --git a/chrome/browser/sync/engine/model_changing_syncer_command.cc b/chrome/browser/sync/engine/model_changing_syncer_command.cc index 2283917..65b1230 100644 --- a/chrome/browser/sync/engine/model_changing_syncer_command.cc +++ b/chrome/browser/sync/engine/model_changing_syncer_command.cc @@ -12,8 +12,14 @@ namespace browser_sync { void ModelChangingSyncerCommand::ExecuteImpl(sessions::SyncSession* session) { work_session_ = session; - session->context()->model_safe_worker()->DoWorkAndWaitUntilDone( - NewCallback(this, &ModelChangingSyncerCommand::StartChangingModel)); + for (size_t i = 0; i < session->workers().size(); ++i) { + ModelSafeWorker* worker = session->workers()[i]; + ModelSafeGroup group = worker->GetModelSafeGroup(); + + sessions::ScopedModelSafeGroupRestriction r(work_session_, group); + worker->DoWorkAndWaitUntilDone(NewCallback(this, + &ModelChangingSyncerCommand::StartChangingModel)); + } } } // namespace browser_sync diff --git a/chrome/browser/sync/engine/model_safe_worker.h b/chrome/browser/sync/engine/model_safe_worker.h index ff470ac..c68218c 100644 --- a/chrome/browser/sync/engine/model_safe_worker.h +++ b/chrome/browser/sync/engine/model_safe_worker.h @@ -5,11 +5,24 @@ #ifndef CHROME_BROWSER_SYNC_ENGINE_MODEL_SAFE_WORKER_H_ #define CHROME_BROWSER_SYNC_ENGINE_MODEL_SAFE_WORKER_H_ +#include +#include + +#include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/util/closure.h" #include "chrome/browser/sync/util/sync_types.h" namespace browser_sync { +enum ModelSafeGroup { + GROUP_PASSIVE = 0, // Models that are just "passively" being synced; e.g. + // changes to these models don't need to be pushed to a + // native model. + GROUP_UI, // Models that live on UI thread and are being synced. + GROUP_DB, // Models that live on DB thread and are being synced. + MODEL_SAFE_GROUP_COUNT, +}; + // The Syncer uses a ModelSafeWorker for all tasks that could potentially // modify syncable entries (e.g under a WriteTransaction). The ModelSafeWorker // only knows how to do one thing, and that is take some work (in a fully @@ -25,21 +38,47 @@ class ModelSafeWorker { // Any time the Syncer performs model modifications (e.g employing a // WriteTransaction), it should be done by this method to ensure it is done // from a model-safe thread. - // - // TODO(timsteele): For now this is non-reentrant, meaning the work being - // done should be at a high enough level in the stack that - // DoWorkAndWaitUntilDone won't be called again by invoking Run() on |work|. - // This is not strictly necessary; it may be best to call - // DoWorkAndWaitUntilDone at lower levels, such as within ApplyUpdates, but - // this is sufficient to simplify and test out our dispatching approach. virtual void DoWorkAndWaitUntilDone(Closure* work) { - work->Run(); // By default, do the work on the current thread. + work->Run(); // For GROUP_PASSIVE, we do the work on the current thread. + } + + virtual ModelSafeGroup GetModelSafeGroup() { + return GROUP_PASSIVE; } private: DISALLOW_COPY_AND_ASSIGN(ModelSafeWorker); }; +// A map that details which ModelSafeGroup each syncable::ModelType +// belongs to. Routing info can change in response to the user enabling / +// disabling sync for certain types, as well as model association completions. +typedef std::map + ModelSafeRoutingInfo; + +// Maintain the up-to-date state regarding which ModelSafeWorkers exist and +// which types get routed to which worker. When a sync session begins, it will +// snapshot the state at that instant, and will use that for the entire +// session. This means if a model becomes synced (or unsynced) by the user +// during a sync session, that session will complete and be unaware of this +// change -- it will only get picked up for the next session. +// TODO(tim): That's really the only way I can make sense of it in the Syncer +// HOWEVER, it is awkward for running ModelAssociation. We need to make sure +// we don't run such a thing until an active session wraps up. +class ModelSafeWorkerRegistrar { + public: + ModelSafeWorkerRegistrar() { } + // Get the current list of active ModelSafeWorkers. Should be threadsafe. + virtual void GetWorkers(std::vector* out) = 0; + + // Get the current routing information for all model types. + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) = 0; + protected: + virtual ~ModelSafeWorkerRegistrar() {} + private: + DISALLOW_COPY_AND_ASSIGN(ModelSafeWorkerRegistrar); +}; + } // namespace browser_sync #endif // CHROME_BROWSER_SYNC_ENGINE_MODEL_SAFE_WORKER_H_ diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc index 8a337f5..eee2565 100755 --- a/chrome/browser/sync/engine/process_updates_command.cc +++ b/chrome/browser/sync/engine/process_updates_command.cc @@ -122,7 +122,7 @@ bool ReverifyEntry(syncable::WriteTransaction* trans, const SyncEntity& entry, const bool deleted = entry.has_deleted() && entry.deleted(); const bool is_directory = entry.IsFolder(); const bool is_bookmark = - SyncerUtil::GetSyncDataType(entry) == SYNC_TYPE_BOOKMARK; + SyncerUtil::GetModelType(entry) == syncable::BOOKMARKS; return VERIFY_SUCCESS == SyncerUtil::VerifyUpdateConsistency(trans, entry, @@ -141,8 +141,7 @@ ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate( const SyncEntity& entry = *static_cast(&pb_entry); using namespace syncable; syncable::Id id = entry.id(); - const std::string name = - SyncerProtoUtil::NameFromSyncEntity(entry); + const std::string name = SyncerProtoUtil::NameFromSyncEntity(entry); WriteTransaction trans(dir, SYNCER, __FILE__, __LINE__); diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index 0c8d9bc..ffedd85 100755 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -67,6 +67,8 @@ using browser_sync::AllStatus; using browser_sync::AllStatusEvent; using browser_sync::AuthWatcher; using browser_sync::AuthWatcherEvent; +using browser_sync::ModelSafeWorker; +using browser_sync::ModelSafeWorkerRegistrar; using browser_sync::Syncer; using browser_sync::SyncerEvent; using browser_sync::SyncerThread; @@ -317,7 +319,6 @@ class AddressWatchTask : public Task { }; namespace sync_api { -class ModelSafeWorkerBridge; static const FilePath::CharType kBookmarkSyncUserSettingsDatabase[] = FILE_PATH_LITERAL("BookmarkSyncSettings.sqlite3"); @@ -732,57 +733,6 @@ syncable::BaseTransaction* WriteTransaction::GetWrappedTrans() const { return transaction_; } -// An implementation of Visitor that we use to "visit" the -// ModelSafeWorkerInterface provided by a client of this API. The object we -// visit is responsible for calling DoWork, which will invoke Run() on it's -// cached work closure. -class ModelSafeWorkerVisitor : public ModelSafeWorkerInterface::Visitor { - public: - explicit ModelSafeWorkerVisitor(Closure* work) : work_(work) { } - virtual ~ModelSafeWorkerVisitor() { } - - // ModelSafeWorkerInterface::Visitor implementation. - virtual void DoWork() { - work_->Run(); - } - - private: - // The work to be done. We run this on DoWork and it cleans itself up - // after it is run. - Closure* work_; - - DISALLOW_COPY_AND_ASSIGN(ModelSafeWorkerVisitor); -}; - -// This class is declared in the cc file to allow inheritance from sync types. -// The ModelSafeWorkerBridge is a liason between a syncapi-client defined -// ModelSafeWorkerInterface and the actual ModelSafeWorker used by the Syncer -// for the current SyncManager. -class ModelSafeWorkerBridge : public browser_sync::ModelSafeWorker { - public: - // Takes ownership of |worker|. - explicit ModelSafeWorkerBridge(ModelSafeWorkerInterface* worker) - : worker_(worker) { - } - virtual ~ModelSafeWorkerBridge() { } - - // Overriding ModelSafeWorker. - virtual void DoWorkAndWaitUntilDone(Closure* work) { - // When the syncer has work to be done, we forward it to our worker who - // will invoke DoWork on |visitor| when appropriate (from model safe - // thread). - ModelSafeWorkerVisitor visitor(work); - worker_->CallDoWorkFromModelSafeThreadAndWait(&visitor); - } - - private: - // The worker that we can forward work requests to, to ensure the work - // is performed on an appropriate model safe thread. - scoped_ptr worker_; - - DISALLOW_COPY_AND_ASSIGN(ModelSafeWorkerBridge); -}; - // A GaiaAuthenticator that uses HttpPostProviders instead of CURL. class BridgedGaiaAuthenticator : public browser_sync::GaiaAuthenticator { public: @@ -849,7 +799,7 @@ class SyncManager::SyncInternal { bool use_ssl, HttpPostProviderFactory* post_factory, HttpPostProviderFactory* auth_post_factory, - ModelSafeWorkerInterface* model_safe_worker, + ModelSafeWorkerRegistrar* model_safe_worker_registrar, bool attempt_last_user_authentication, const char* user_agent, const std::string& lsid); @@ -1079,7 +1029,7 @@ bool SyncManager::Init(const FilePath& database_location, bool use_ssl, HttpPostProviderFactory* post_factory, HttpPostProviderFactory* auth_post_factory, - ModelSafeWorkerInterface* model_safe_worker, + ModelSafeWorkerRegistrar* registrar, bool attempt_last_user_authentication, const char* user_agent, const char* lsid) { @@ -1094,7 +1044,7 @@ bool SyncManager::Init(const FilePath& database_location, use_ssl, post_factory, auth_post_factory, - model_safe_worker, + registrar, attempt_last_user_authentication, user_agent, lsid); @@ -1119,7 +1069,7 @@ bool SyncManager::SyncInternal::Init( const char* gaia_source, bool use_ssl, HttpPostProviderFactory* post_factory, HttpPostProviderFactory* auth_post_factory, - ModelSafeWorkerInterface* model_safe_worker, + ModelSafeWorkerRegistrar* model_safe_worker_registrar, bool attempt_last_user_authentication, const char* user_agent, const std::string& lsid) { @@ -1200,11 +1150,8 @@ bool SyncManager::SyncInternal::Init( this, &SyncInternal::HandleAuthWatcherEvent)); // Build a SyncSessionContext and store the worker in it. - // We set up both sides of the "bridge" here, with the ModelSafeWorkerBridge - // on the Syncer side, and |model_safe_worker| on the API client side. - ModelSafeWorkerBridge* worker = new ModelSafeWorkerBridge(model_safe_worker); SyncSessionContext* context = new SyncSessionContext( - connection_manager_.get(), dir_manager(), worker); + connection_manager_.get(), dir_manager(), model_safe_worker_registrar); // The SyncerThread takes ownership of |context|. syncer_thread_ = new SyncerThread(context, &allstatus_); diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h index 040620b..8b0c73e 100644 --- a/chrome/browser/sync/engine/syncapi.h +++ b/chrome/browser/sync/engine/syncapi.h @@ -52,6 +52,10 @@ // on all platforms. #define SYNC_EXPORT +namespace browser_sync { +class ModelSafeWorkerRegistrar; +} + // Forward declarations of internal class types so that sync API objects // may have opaque pointers to these types. namespace syncable { @@ -69,7 +73,6 @@ namespace sync_api { // Forward declarations of classes to be defined later in this file. class BaseTransaction; class HttpPostProviderFactory; -class ModelSafeWorkerInterface; class SyncManager; class WriteTransaction; struct UserShare; @@ -509,7 +512,7 @@ class SYNC_EXPORT SyncManager { bool use_ssl, HttpPostProviderFactory* post_factory, HttpPostProviderFactory* auth_post_factory, - ModelSafeWorkerInterface* model_safe_worker, + browser_sync::ModelSafeWorkerRegistrar* registrar, bool attempt_last_user_authentication, const char* user_agent, const char* lsid); @@ -639,33 +642,6 @@ class HttpPostProviderFactory { virtual ~HttpPostProviderFactory() { } }; -// A class syncapi clients should use whenever the underlying model is bound to -// a particular thread in the embedding application. This exposes an interface -// by which any model-modifying invocations will be forwarded to the -// appropriate thread in the embedding application. -// "model safe" refers to not allowing an embedding application model to fall -// out of sync with the syncable::Directory due to race conditions. -class ModelSafeWorkerInterface { - public: - virtual ~ModelSafeWorkerInterface() { } - // A Visitor is passed to CallDoWorkFromModelSafeThreadAndWait invocations, - // and it's sole purpose is to provide a way for the ModelSafeWorkerInterface - // implementation to actually _do_ the work required, by calling the only - // method on this class, DoWork(). - class Visitor { - public: - virtual ~Visitor() { } - // When on a model safe thread, this should be called to have the syncapi - // actually perform the work needing to be done. - virtual void DoWork() = 0; - }; - // Subclasses should implement to invoke DoWork on |visitor| once on a thread - // appropriate for data model modifications. - // While it doesn't hurt, the impl does not need to be re-entrant (for now). - // Note: |visitor| is owned by caller. - virtual void CallDoWorkFromModelSafeThreadAndWait(Visitor* visitor) = 0; -}; - } // namespace sync_api #endif // CHROME_BROWSER_SYNC_ENGINE_SYNCAPI_H_ diff --git a/chrome/browser/sync/engine/syncer_thread_unittest.cc b/chrome/browser/sync/engine/syncer_thread_unittest.cc index 319d3df..314237a 100644 --- a/chrome/browser/sync/engine/syncer_thread_unittest.cc +++ b/chrome/browser/sync/engine/syncer_thread_unittest.cc @@ -26,7 +26,8 @@ using sessions::SyncSessionContext; typedef testing::Test SyncerThreadTest; typedef SyncerThread::WaitInterval WaitInterval; -class SyncerThreadWithSyncerTest : public testing::Test { +class SyncerThreadWithSyncerTest : public testing::Test, + public ModelSafeWorkerRegistrar { public: SyncerThreadWithSyncerTest() : sync_cycle_ended_event_(false, false) {} virtual void SetUp() { @@ -34,8 +35,9 @@ class SyncerThreadWithSyncerTest : public testing::Test { connection_.reset(new MockConnectionManager(metadb_.manager(), metadb_.name())); allstatus_.reset(new AllStatus()); + worker_.reset(new ModelSafeWorker()); SyncSessionContext* context = new SyncSessionContext(connection_.get(), - metadb_.manager(), new ModelSafeWorker()); + metadb_.manager(), this); syncer_thread_ = new SyncerThread(context, allstatus_.get()); syncer_event_hookup_.reset( NewEventListenerHookup(syncer_thread_->relay_channel(), this, @@ -50,6 +52,17 @@ class SyncerThreadWithSyncerTest : public testing::Test { metadb_.TearDown(); } + // ModelSafeWorkerRegistrar implementation. + virtual void GetWorkers(std::vector* out) { + out->push_back(worker_.get()); + } + + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) { + // We're just testing the sync engine here, so we shunt everything to + // the SyncerThread. + (*out)[syncable::BOOKMARKS] = GROUP_PASSIVE; + } + ManuallyOpenedTestDirectorySetterUpper* metadb() { return &metadb_; } MockConnectionManager* connection() { return connection_.get(); } SyncerThread* syncer_thread() { return syncer_thread_; } @@ -72,6 +85,7 @@ class SyncerThreadWithSyncerTest : public testing::Test { scoped_ptr connection_; scoped_ptr allstatus_; scoped_refptr syncer_thread_; + scoped_ptr worker_; scoped_ptr syncer_event_hookup_; base::WaitableEvent sync_cycle_ended_event_; DISALLOW_COPY_AND_ASSIGN(SyncerThreadWithSyncerTest); diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h index 925b109..79c53f9 100755 --- a/chrome/browser/sync/engine/syncer_types.h +++ b/chrome/browser/sync/engine/syncer_types.h @@ -69,11 +69,6 @@ enum VerifyCommitResult { VERIFY_OK, }; -enum SyncDataType { - SYNC_TYPE_BOOKMARK, - SYNC_TYPE_UNKNOWN, -}; - struct SyncerEvent { typedef SyncerEvent EventType; diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc index 9eaf809..17ed151 100755 --- a/chrome/browser/sync/engine/syncer_unittest.cc +++ b/chrome/browser/sync/engine/syncer_unittest.cc @@ -96,7 +96,9 @@ const int kTestDataLen = 12; const int64 kTestLogRequestTimestamp = 123456; } // namespace -class SyncerTest : public testing::Test, public SyncSession::Delegate { +class SyncerTest : public testing::Test, + public SyncSession::Delegate, + public ModelSafeWorkerRegistrar { protected: SyncerTest() : syncer_(NULL) {} @@ -116,6 +118,17 @@ class SyncerTest : public testing::Test, public SyncSession::Delegate { last_short_poll_interval_received_ = new_interval; } + // ModelSafeWorkerRegistrar implementation. + virtual void GetWorkers(std::vector* out) { + out->push_back(worker_.get()); + } + + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) { + // We're just testing the sync engine here, so we shunt everything to + // the SyncerThread. + (*out)[syncable::BOOKMARKS] = GROUP_PASSIVE; + } + void HandleSyncerEvent(SyncerEvent event) { LOG(INFO) << "HandleSyncerEvent in unittest " << event.what_happened; // we only test for entry-specific events, not status changed ones. @@ -151,9 +164,9 @@ class SyncerTest : public testing::Test, public SyncSession::Delegate { mock_server_.reset( new MockConnectionManager(syncdb_.manager(), syncdb_.name())); - + worker_.reset(new ModelSafeWorker()); context_.reset(new SyncSessionContext(mock_server_.get(), syncdb_.manager(), - new ModelSafeWorker())); + this)); context_->set_account_name(syncdb_.name()); ASSERT_FALSE(context_->syncer_event_channel()); ASSERT_FALSE(context_->resolver()); @@ -364,6 +377,7 @@ class SyncerTest : public testing::Test, public SyncSession::Delegate { std::set syncer_events_; base::TimeDelta last_short_poll_interval_received_; base::TimeDelta last_long_poll_interval_received_; + scoped_ptr worker_; DISALLOW_COPY_AND_ASSIGN(SyncerTest); }; diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc index 21656be..da451a5 100755 --- a/chrome/browser/sync/engine/syncer_util.cc +++ b/chrome/browser/sync/engine/syncer_util.cc @@ -276,7 +276,7 @@ void SyncerUtil::UpdateServerFieldsFromUpdate( local_entry->Put(SERVER_MTIME, ServerTimeToClientTime(server_entry.mtime())); local_entry->Put(SERVER_IS_BOOKMARK_OBJECT, - GetSyncDataType(server_entry) == SYNC_TYPE_BOOKMARK); + GetModelType(server_entry) == syncable::BOOKMARKS); local_entry->Put(SERVER_IS_DIR, server_entry.IsFolder()); if (server_entry.has_singleton_tag()) { const string& tag = server_entry.singleton_tag(); @@ -767,7 +767,7 @@ syncable::Id SyncerUtil::ComputePrevIdFromServerPosition( return closest_sibling; } -browser_sync::SyncDataType SyncerUtil::GetSyncDataType( +syncable::ModelType SyncerUtil::GetModelType( const SyncEntity& entry) { const bool is_bookmark = @@ -776,10 +776,10 @@ browser_sync::SyncDataType SyncerUtil::GetSyncDataType( entry.has_bookmarkdata(); if (is_bookmark) { - return SYNC_TYPE_BOOKMARK; + return syncable::BOOKMARKS; } - return SYNC_TYPE_UNKNOWN; + return syncable::UNSPECIFIED; } } // namespace browser_sync diff --git a/chrome/browser/sync/engine/syncer_util.h b/chrome/browser/sync/engine/syncer_util.h index 2396706..f42bbe3 100755 --- a/chrome/browser/sync/engine/syncer_util.h +++ b/chrome/browser/sync/engine/syncer_util.h @@ -146,13 +146,10 @@ class SyncerUtil { // if they match. For an up-to-date item, this should be the case. static bool ServerAndLocalOrdersMatch(syncable::Entry* entry); - static browser_sync::SyncDataType GetSyncDataType(const SyncEntity& entry); + static syncable::ModelType GetModelType(const SyncEntity& entry); private: - // Private ctor/dtor since this class shouldn't be instantiated. - SyncerUtil() {} - virtual ~SyncerUtil() {} - DISALLOW_COPY_AND_ASSIGN(SyncerUtil); + DISALLOW_IMPLICIT_CONSTRUCTORS(SyncerUtil); }; #ifndef OS_WIN diff --git a/chrome/browser/sync/engine/verify_updates_command.cc b/chrome/browser/sync/engine/verify_updates_command.cc index 86cd71c..d357c7c 100755 --- a/chrome/browser/sync/engine/verify_updates_command.cc +++ b/chrome/browser/sync/engine/verify_updates_command.cc @@ -60,7 +60,7 @@ VerifyResult VerifyUpdatesCommand::VerifyUpdate( const bool deleted = entry.has_deleted() && entry.deleted(); const bool is_directory = entry.IsFolder(); const bool is_bookmark = - SyncerUtil::GetSyncDataType(entry) == SYNC_TYPE_BOOKMARK; + SyncerUtil::GetModelType(entry) == syncable::BOOKMARKS; if (!id.ServerKnows()) { LOG(ERROR) << "Illegal negative id in received updates"; diff --git a/chrome/browser/sync/glue/bookmark_model_worker.cc b/chrome/browser/sync/glue/bookmark_model_worker.cc deleted file mode 100644 index 423aa2e..0000000 --- a/chrome/browser/sync/glue/bookmark_model_worker.cc +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright (c) 2006-2008 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/glue/bookmark_model_worker.h" - -#include "base/message_loop.h" -#include "base/waitable_event.h" - -namespace browser_sync { - -void BookmarkModelWorker::CallDoWorkFromModelSafeThreadAndWait( - ModelSafeWorkerInterface::Visitor* visitor) { - // It is possible this gets called when we are in the STOPPING state, because - // the UI loop has initiated shutdown but the syncer hasn't got the memo yet. - // This is fine, the work will get scheduled and run normally or run by our - // code handling this case in Stop(). - DCHECK_NE(state_, STOPPED); - if (state_ == STOPPED) - return; - if (MessageLoop::current() == bookmark_model_loop_) { - DLOG(WARNING) << "CallDoWorkFromModelSafeThreadAndWait called from " - << "bookmark_model_loop_. Probably a nested invocation?"; - visitor->DoWork(); - return; - } - - // Create an unsignaled event to wait on. - base::WaitableEvent work_done(false, false); - { - // We lock only to avoid PostTask'ing a NULL pending_work_ (because it - // could get Run() in Stop() and call OnTaskCompleted before we post). - // The task is owned by the message loop as per usual. - AutoLock lock(lock_); - DCHECK(!pending_work_); - pending_work_ = new CallDoWorkAndSignalTask(visitor, &work_done, this); - bookmark_model_loop_->PostTask(FROM_HERE, pending_work_); - } - syncapi_event_.Signal(); // Notify that the syncapi produced work for us. - work_done.Wait(); -} - -BookmarkModelWorker::~BookmarkModelWorker() { - DCHECK_EQ(state_, STOPPED); -} - -void BookmarkModelWorker::OnSyncerShutdownComplete() { - AutoLock lock(lock_); - // The SyncerThread has terminated and we are no longer needed by syncapi. - // The UI loop initiated shutdown and is (or will be) waiting in Stop(). - // We could either be WORKING or RUNNING_MANUAL_SHUTDOWN_PUMP, depending - // on where we timeslice the UI thread in Stop; but we can't be STOPPED, - // because that would imply OnSyncerShutdownComplete already signaled. - DCHECK_NE(state_, STOPPED); - - syncapi_has_shutdown_ = true; - syncapi_event_.Signal(); -} - -void BookmarkModelWorker::Stop() { - DCHECK_EQ(MessageLoop::current(), bookmark_model_loop_); - - AutoLock lock(lock_); - DCHECK_EQ(state_, WORKING); - - // We're on our own now, the beloved UI MessageLoop is no longer running. - // Any tasks scheduled or to be scheduled on the UI MessageLoop will not run. - state_ = RUNNING_MANUAL_SHUTDOWN_PUMP; - - // Drain any final tasks manually until the SyncerThread tells us it has - // totally finished. There should only ever be 0 or 1 tasks Run() here. - while (!syncapi_has_shutdown_) { - if (pending_work_) - pending_work_->Run(); // OnTaskCompleted will set pending_work_ to NULL. - - // Wait for either a new task or SyncerThread termination. - syncapi_event_.Wait(); - } - - state_ = STOPPED; -} - -void BookmarkModelWorker::CallDoWorkAndSignalTask::Run() { - if (!visitor_) { - // This can happen during tests or cases where there are more than just the - // default BookmarkModelWorker in existence and it gets destroyed before - // the main UI loop has terminated. There is no easy way to assert the - // loop is running / not running at the moment, so we just provide cancel - // semantics here and short-circuit. - // TODO(timsteele): Maybe we should have the message loop destruction - // observer fire when the loop has ended, just a bit before it - // actually gets destroyed. - return; - } - visitor_->DoWork(); - - // Sever ties with visitor_ to allow the sanity-checking above that we don't - // get run twice. - visitor_ = NULL; - - // Notify the BookmarkModelWorker that scheduled us that we have run - // successfully. - scheduler_->OnTaskCompleted(); - work_done_->Signal(); // Unblock the syncer thread that scheduled us. -} - -} // namespace browser_sync diff --git a/chrome/browser/sync/glue/bookmark_model_worker.h b/chrome/browser/sync/glue/bookmark_model_worker.h deleted file mode 100644 index e6491f3..0000000 --- a/chrome/browser/sync/glue/bookmark_model_worker.h +++ /dev/null @@ -1,138 +0,0 @@ -// Copyright (c) 2006-2008 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. - -#ifndef CHROME_BROWSER_SYNC_GLUE_BOOKMARK_MODEL_WORKER_H_ -#define CHROME_BROWSER_SYNC_GLUE_BOOKMARK_MODEL_WORKER_H_ - -#include "base/condition_variable.h" -#include "base/lock.h" -#include "base/task.h" -#include "base/waitable_event.h" -#include "chrome/browser/sync/engine/syncapi.h" - -class MessageLoop; - -namespace browser_sync { - -// A ModelSafeWorker for bookmarks that accepts work requests from the syncapi -// that need to be fulfilled from the MessageLoop home to the BookmarkModel -// (this is typically the "main" UI thread). -// -// Lifetime note: Instances of this class will generally be owned by the -// SyncerThread. When the SyncerThread _object_ is destroyed, the -// BookmarkModelWorker will be destroyed. The SyncerThread object is destroyed -// after the actual syncer pthread has exited. -class BookmarkModelWorker - : public sync_api::ModelSafeWorkerInterface { - public: - explicit BookmarkModelWorker(MessageLoop* bookmark_model_loop) - : state_(WORKING), - pending_work_(NULL), - syncapi_has_shutdown_(false), - bookmark_model_loop_(bookmark_model_loop), - syncapi_event_(&lock_) { - } - virtual ~BookmarkModelWorker(); - - // A simple task to signal a waitable event after calling DoWork on a visitor. - class CallDoWorkAndSignalTask : public Task { - public: - CallDoWorkAndSignalTask(ModelSafeWorkerInterface::Visitor* visitor, - base::WaitableEvent* work_done, - BookmarkModelWorker* scheduler) - : visitor_(visitor), work_done_(work_done), scheduler_(scheduler) { - } - virtual ~CallDoWorkAndSignalTask() { } - - // Task implementation. - virtual void Run(); - - private: - // Task data - a visitor that knows how to DoWork, and a waitable event - // to signal after the work has been done. - ModelSafeWorkerInterface::Visitor* visitor_; - base::WaitableEvent* work_done_; - - // The BookmarkModelWorker responsible for scheduling us. - BookmarkModelWorker* const scheduler_; - - DISALLOW_COPY_AND_ASSIGN(CallDoWorkAndSignalTask); - }; - - // Called by the UI thread on shutdown of the sync service. Blocks until - // the BookmarkModelWorker has safely met termination conditions, namely that - // no task scheduled by CallDoWorkFromModelSafeThreadAndWait remains un- - // processed and that syncapi will not schedule any further work for us to do. - void Stop(); - - // ModelSafeWorkerInterface implementation. Called on syncapi SyncerThread. - virtual void CallDoWorkFromModelSafeThreadAndWait( - ModelSafeWorkerInterface::Visitor* visitor); - - // Upon receiving this idempotent call, the ModelSafeWorkerInterface can - // assume no work will ever be scheduled again from now on. If it has any work - // that it has not yet completed, it must make sure to run it as soon as - // possible as the Syncer is trying to shut down. Called from the CoreThread. - void OnSyncerShutdownComplete(); - - // Callback from |pending_work_| to notify us that it has been run. - // Called on |bookmark_model_loop_|. - void OnTaskCompleted() { pending_work_ = NULL; } - - private: - // The life-cycle of a BookmarkModelWorker in three states. - enum State { - // We hit the ground running in this state and remain until - // the UI loop calls Stop(). - WORKING, - // Stop() sequence has been initiated, but we have not received word that - // the SyncerThread has terminated and doesn't need us anymore. Since the - // UI MessageLoop is not running at this point, we manually process any - // last pending_task_ that the Syncer throws at us, effectively dedicating - // the UI thread to terminating the Syncer. - RUNNING_MANUAL_SHUTDOWN_PUMP, - // We have come to a complete stop, no scheduled work remains, and no work - // will be scheduled from now until our destruction. - STOPPED, - }; - - // This is set by the UI thread, but is not explicitly thread safe, so only - // read this value from other threads when you know it is absolutely safe (e.g - // there is _no_ way we can be in CallDoWork with state_ = STOPPED, so it is - // safe to read / compare in this case). - State state_; - - // We keep a reference to any task we have scheduled so we can gracefully - // force them to run if the syncer is trying to shutdown. - Task* pending_work_; - - // Set by the SyncCoreThread when Syncapi shutdown has completed and the - // SyncerThread has terminated, so no more work will be scheduled. Read by - // the UI thread in Stop(). - bool syncapi_has_shutdown_; - - // The BookmarkModel's home-sweet-home MessageLoop. - MessageLoop* const bookmark_model_loop_; - - // We use a Lock for all data members and a ConditionVariable to synchronize. - // We do this instead of using a WaitableEvent and a bool condition in order - // to guard against races that could arise due to the fact that the lack of a - // barrier permits instructions to be reordered by compiler optimizations. - // Possible or not, that route makes for very fragile code due to existence - // of theoretical races. - Lock lock_; - - // Used as a barrier at shutdown to ensure the SyncerThread terminates before - // we allow the UI thread to return from Stop(). This gets signalled whenever - // one of two events occur: a new pending_work_ task was scheduled, or the - // SyncerThread has terminated. We only care about (1) when we are in Stop(), - // because we have to manually Run() the task. - ConditionVariable syncapi_event_; - - DISALLOW_COPY_AND_ASSIGN(BookmarkModelWorker); -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_GLUE_BOOKMARK_MODEL_WORKER_H_ diff --git a/chrome/browser/sync/glue/bookmark_model_worker_unittest.cc b/chrome/browser/sync/glue/bookmark_model_worker_unittest.cc deleted file mode 100644 index 183e2f5..0000000 --- a/chrome/browser/sync/glue/bookmark_model_worker_unittest.cc +++ /dev/null @@ -1,221 +0,0 @@ -// Copyright (c) 2006-2009 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 "base/thread.h" -#include "chrome/browser/sync/engine/syncapi.h" -#include "chrome/browser/sync/glue/bookmark_model_worker.h" -#include "testing/gtest/include/gtest/gtest.h" - -using browser_sync::BookmarkModelWorker; -using namespace sync_api; - -// Various boilerplate, primarily for the StopWithPendingWork test. - -class BookmarkModelWorkerVisitor : public ModelSafeWorkerInterface::Visitor { - public: - BookmarkModelWorkerVisitor(MessageLoop* faux_ui_loop, - base::WaitableEvent* was_run, - bool quit_loop) - : faux_ui_loop_(faux_ui_loop), quit_loop_when_run_(quit_loop), - was_run_(was_run) { } - virtual ~BookmarkModelWorkerVisitor() { } - - virtual void DoWork() { - EXPECT_EQ(MessageLoop::current(), faux_ui_loop_); - was_run_->Signal(); - if (quit_loop_when_run_) - MessageLoop::current()->Quit(); - } - - private: - MessageLoop* faux_ui_loop_; - bool quit_loop_when_run_; - base::WaitableEvent* was_run_; - DISALLOW_COPY_AND_ASSIGN(BookmarkModelWorkerVisitor); -}; - -// A faux-syncer that only interacts with its model safe worker. -class Syncer { - public: - explicit Syncer(BookmarkModelWorker* worker) : worker_(worker) {} - ~Syncer() {} - - void SyncShare(BookmarkModelWorkerVisitor* visitor) { - worker_->CallDoWorkFromModelSafeThreadAndWait(visitor); - } - private: - BookmarkModelWorker* worker_; - DISALLOW_COPY_AND_ASSIGN(Syncer); -}; - -// A task run from the SyncerThread to "sync share", ie tell the Syncer to -// ask it's ModelSafeWorker to do something. -class FakeSyncShareTask : public Task { - public: - FakeSyncShareTask(Syncer* syncer, BookmarkModelWorkerVisitor* visitor) - : syncer_(syncer), visitor_(visitor) { - } - virtual void Run() { - syncer_->SyncShare(visitor_); - } - private: - Syncer* syncer_; - BookmarkModelWorkerVisitor* visitor_; - DISALLOW_COPY_AND_ASSIGN(FakeSyncShareTask); -}; - -// A task run from the CoreThread to simulate terminating syncapi. -class FakeSyncapiShutdownTask : public Task { - public: - FakeSyncapiShutdownTask(base::Thread* syncer_thread, - BookmarkModelWorker* worker, - base::WaitableEvent** jobs, - size_t job_count) - : syncer_thread_(syncer_thread), worker_(worker), jobs_(jobs), - job_count_(job_count), all_jobs_done_(false, false) { } - virtual void Run() { - // In real life, we would try and close a sync directory, which would - // result in the syncer calling it's own destructor, which results in - // the SyncerThread::HaltSyncer being called, which sets the - // syncer in RequestEarlyExit mode and waits until the Syncer finishes - // SyncShare to remove the syncer from it's watch. Here we just manually - // wait until all outstanding jobs are done to simulate what happens in - // SyncerThread::HaltSyncer. - all_jobs_done_.WaitMany(jobs_, job_count_); - - // These two calls are made from SyncBackendHost::Core::DoShutdown. - syncer_thread_->Stop(); - worker_->OnSyncerShutdownComplete(); - } - private: - base::Thread* syncer_thread_; - BookmarkModelWorker* worker_; - base::WaitableEvent** jobs_; - size_t job_count_; - base::WaitableEvent all_jobs_done_; - DISALLOW_COPY_AND_ASSIGN(FakeSyncapiShutdownTask); -}; - -class BookmarkModelWorkerTest : public testing::Test { - public: - BookmarkModelWorkerTest() : faux_syncer_thread_("FauxSyncerThread"), - faux_core_thread_("FauxCoreThread") { } - - virtual void SetUp() { - faux_syncer_thread_.Start(); - bmw_.reset(new BookmarkModelWorker(&faux_ui_loop_)); - syncer_.reset(new Syncer(bmw_.get())); - } - - Syncer* syncer() { return syncer_.get(); } - BookmarkModelWorker* bmw() { return bmw_.get(); } - base::Thread* core_thread() { return &faux_core_thread_; } - base::Thread* syncer_thread() { return &faux_syncer_thread_; } - MessageLoop* ui_loop() { return &faux_ui_loop_; } - private: - MessageLoop faux_ui_loop_; - base::Thread faux_syncer_thread_; - base::Thread faux_core_thread_; - scoped_ptr bmw_; - scoped_ptr syncer_; -}; - -TEST_F(BookmarkModelWorkerTest, ScheduledWorkRunsOnUILoop) { - base::WaitableEvent v_was_run(false, false); - scoped_ptr v( - new BookmarkModelWorkerVisitor(ui_loop(), &v_was_run, true)); - - syncer_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncShareTask(syncer(), v.get())); - - // We are on the UI thread, so run our loop to process the - // (hopefully) scheduled task from a SyncShare invocation. - MessageLoop::current()->Run(); - - bmw()->OnSyncerShutdownComplete(); - bmw()->Stop(); - syncer_thread()->Stop(); -} - -TEST_F(BookmarkModelWorkerTest, StopWithPendingWork) { - // What we want to set up is the following: - // ("ui_thread" is the thread we are currently executing on) - // 1 - simulate the user shutting down the browser, and the ui thread needing - // to terminate the core thread. - // 2 - the core thread is where the syncapi is accessed from, and so it needs - // to shut down the SyncerThread. - // 3 - the syncer is waiting on the BookmarkModelWorker to - // perform a task for it. - // The BookmarkModelWorker's manual shutdown pump will save the day, as the - // UI thread is not actually trying to join() the core thread, it is merely - // waiting for the SyncerThread to give it work or to finish. After that, it - // will join the core thread which should succeed as the SyncerThread has left - // the building. Unfortunately this test as written is not provably decidable, - // as it will always halt on success, but it may not on failure (namely if - // the task scheduled by the Syncer is _never_ run). - core_thread()->Start(); - base::WaitableEvent v_ran(false, false); - scoped_ptr v(new BookmarkModelWorkerVisitor( - ui_loop(), &v_ran, false)); - base::WaitableEvent* jobs[] = { &v_ran }; - - // The current message loop is not running, so queue a task to cause - // BookmarkModelWorker::Stop() to play a crucial role. See comment below. - syncer_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncShareTask(syncer(), v.get())); - - // This is what gets the core_thread blocked on the syncer_thread. - core_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncapiShutdownTask(syncer_thread(), bmw(), jobs, 1)); - - // This is what gets the UI thread blocked until NotifyExitRequested, - // which is called when FakeSyncapiShutdownTask runs and deletes the syncer. - bmw()->Stop(); - - EXPECT_FALSE(syncer_thread()->IsRunning()); - core_thread()->Stop(); -} - -TEST_F(BookmarkModelWorkerTest, HypotheticalManualPumpFlooding) { - // This situation should not happen in real life because the Syncer should - // never send more than one CallDoWork notification after early_exit_requested - // has been set, but our BookmarkModelWorker is built to handle this case - // nonetheless. It may be needed in the future, and since we support it and - // it is not actually exercised in the wild this test is essential. - // It is identical to above except we schedule more than one visitor. - core_thread()->Start(); - - // Our ammunition. - base::WaitableEvent fox1_ran(false, false); - scoped_ptr fox1(new BookmarkModelWorkerVisitor( - ui_loop(), &fox1_ran, false)); - base::WaitableEvent fox2_ran(false, false); - scoped_ptr fox2(new BookmarkModelWorkerVisitor( - ui_loop(), &fox2_ran, false)); - base::WaitableEvent fox3_ran(false, false); - scoped_ptr fox3(new BookmarkModelWorkerVisitor( - ui_loop(), &fox3_ran, false)); - base::WaitableEvent* jobs[] = { &fox1_ran, &fox2_ran, &fox3_ran }; - - // The current message loop is not running, so queue a task to cause - // BookmarkModelWorker::Stop() to play a crucial role. See comment below. - syncer_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncShareTask(syncer(), fox1.get())); - syncer_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncShareTask(syncer(), fox2.get())); - - // This is what gets the core_thread blocked on the syncer_thread. - core_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncapiShutdownTask(syncer_thread(), bmw(), jobs, 3)); - syncer_thread()->message_loop()->PostTask(FROM_HERE, - new FakeSyncShareTask(syncer(), fox3.get())); - - // This is what gets the UI thread blocked until NotifyExitRequested, - // which is called when FakeSyncapiShutdownTask runs and deletes the syncer. - bmw()->Stop(); - - // Was the thread killed? - EXPECT_FALSE(syncer_thread()->IsRunning()); - core_thread()->Stop(); -} diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 8bff80c..4a6d8d7 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -10,7 +10,6 @@ #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/glue/http_bridge.h" -#include "chrome/browser/sync/glue/bookmark_model_worker.h" #include "webkit/glue/webkit_glue.h" static const int kSaveChangesIntervalSeconds = 10; @@ -28,16 +27,28 @@ SyncBackendHost::SyncBackendHost(SyncFrontend* frontend, std::set processors) : core_thread_("Chrome_SyncCoreThread"), frontend_loop_(MessageLoop::current()), - bookmark_model_worker_(NULL), frontend_(frontend), processors_(processors), sync_data_folder_path_(profile_path.Append(kSyncDataFolderName)), last_auth_error_(AuthError::None()) { + + // Init our registrar state. + for (int i = 0; i < MODEL_SAFE_GROUP_COUNT; i++) + registrar_.workers[i] = NULL; + + for (int i = 0; i < syncable::MODEL_TYPE_COUNT; i++) { + syncable::ModelType t(syncable::ModelTypeFromInt(i)); + registrar_.routing_info[t] = GROUP_PASSIVE; // Init to syncing 0 types. + } + core_ = new Core(this); } SyncBackendHost::~SyncBackendHost() { DCHECK(!core_ && !frontend_) << "Must call Shutdown before destructor."; + + for (int i = 0; i < MODEL_SAFE_GROUP_COUNT; ++i) + DCHECK(registrar_.workers[i] == NULL); } void SyncBackendHost::Initialize( @@ -46,11 +57,19 @@ void SyncBackendHost::Initialize( const std::string& lsid) { if (!core_thread_.Start()) return; - bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_); + + // Create a worker for the UI thread and route bookmark changes to it. + // TODO(tim): Pull this into a method to reuse. For now we don't even + // need to lock because we init before the syncapi exists and we tear down + // after the syncapi is destroyed. Make sure to NULL-check workers_ indices + // when a new type is synced as the worker may already exist and you just + // need to update routing_info_. + registrar_.workers[GROUP_UI] = new UIModelWorker(frontend_loop_); + registrar_.routing_info[syncable::BOOKMARKS] = GROUP_UI; core_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoInitialize, - sync_service_url, bookmark_model_worker_, true, + sync_service_url, true, new HttpBridgeFactory(baseline_context_getter), new HttpBridgeFactory(baseline_context_getter), lsid)); @@ -74,11 +93,12 @@ void SyncBackendHost::Shutdown(bool sync_disabled) { &SyncBackendHost::Core::DoShutdown, sync_disabled)); - // Before joining the core_thread_, we wait for the BookmarkModelWorker to + // Before joining the core_thread_, we wait for the UIModelWorker to // give us the green light that it is not depending on the frontend_loop_ to // process any more tasks. Stop() blocks until this termination condition // is true. - bookmark_model_worker_->Stop(); + if (ui_worker()) + ui_worker()->Stop(); // Stop will return once the thread exits, which will be after DoShutdown // runs. DoShutdown needs to run from core_thread_ because the sync backend @@ -92,7 +112,9 @@ void SyncBackendHost::Shutdown(bool sync_disabled) { // DoShutdown. core_thread_.Stop(); - bookmark_model_worker_ = NULL; + registrar_.routing_info.clear(); + delete registrar_.workers[GROUP_UI]; + registrar_.workers[GROUP_UI] = NULL; frontend_ = NULL; core_ = NULL; // Releases reference to core_. } @@ -133,6 +155,22 @@ const GoogleServiceAuthError& SyncBackendHost::GetAuthError() const { return last_auth_error_; } +void SyncBackendHost::GetWorkers(std::vector* out) { + AutoLock lock(registrar_lock_); + out->clear(); + for (int i = 0; i < MODEL_SAFE_GROUP_COUNT; i++) { + if (registrar_.workers[i] == NULL) + continue; + out->push_back(registrar_.workers[i]); + } +} + +void SyncBackendHost::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) { + AutoLock lock(registrar_lock_); + ModelSafeRoutingInfo copy(registrar_.routing_info); + out->swap(copy); +} + SyncBackendHost::Core::Core(SyncBackendHost* backend) : host_(backend), syncapi_(new sync_api::SyncManager()) { @@ -167,7 +205,6 @@ std::string MakeUserAgentForSyncapi() { void SyncBackendHost::Core::DoInitialize( const GURL& service_url, - BookmarkModelWorker* bookmark_model_worker, bool attempt_last_user_authentication, sync_api::HttpPostProviderFactory* http_provider_factory, sync_api::HttpPostProviderFactory* auth_http_provider_factory, @@ -189,7 +226,7 @@ void SyncBackendHost::Core::DoInitialize( service_url.SchemeIsSecure(), http_provider_factory, auth_http_provider_factory, - bookmark_model_worker, + host_, // ModelSafeWorkerRegistrar. attempt_last_user_authentication, MakeUserAgentForSyncapi().c_str(), lsid.c_str()); @@ -203,13 +240,22 @@ void SyncBackendHost::Core::DoAuthenticate(const std::string& username, syncapi_->Authenticate(username.c_str(), password.c_str(), captcha.c_str()); } +UIModelWorker* SyncBackendHost::ui_worker() { + ModelSafeWorker* w = registrar_.workers[GROUP_UI]; + if (w == NULL) + return NULL; + if (w->GetModelSafeGroup() != GROUP_UI) + NOTREACHED(); + return static_cast(w); +} + void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { DCHECK(MessageLoop::current() == host_->core_thread_.message_loop()); save_changes_timer_.Stop(); syncapi_->Shutdown(); // Stops the SyncerThread. syncapi_->RemoveObserver(); - host_->bookmark_model_worker_->OnSyncerShutdownComplete(); + host_->ui_worker()->OnSyncerShutdownComplete(); if (sync_disabled && file_util::DirectoryExists(host_->sync_data_folder_path())) { @@ -231,7 +277,7 @@ void SyncBackendHost::Core::OnChangesApplied( } // ChangesApplied is the one exception that should come over from the sync - // backend already on the service_loop_ thanks to our BookmarkModelWorker. + // backend already on the service_loop_ thanks to our UIModelWorker. // SyncFrontend changes exclusively on the UI loop, because it updates // the bookmark model. As such, we don't need to worry about changes that // have been made to the bookmark model but not yet applied to the sync diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 59ce96c..5150d4d 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -7,6 +7,7 @@ #include #include +#include #include "base/file_path.h" #include "base/lock.h" @@ -17,7 +18,9 @@ #include "chrome/browser/google_service_auth_error.h" #include "chrome/browser/net/url_request_context_getter.h" #include "chrome/browser/sync/engine/syncapi.h" -#include "chrome/browser/sync/glue/bookmark_model_worker.h" +#include "chrome/browser/sync/engine/model_safe_worker.h" +#include "chrome/browser/sync/glue/ui_model_worker.h" +#include "chrome/browser/sync/syncable/model_type.h" #include "googleurl/src/gurl.h" namespace browser_sync { @@ -56,7 +59,7 @@ class SyncFrontend { // syncapi element, the SyncManager, on its own thread. This class handles // dispatch of potentially blocking calls to appropriate threads and ensures // that the SyncFrontend is only accessed on the UI loop. -class SyncBackendHost { +class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { public: typedef sync_api::UserShare* UserShareHandle; typedef sync_api::SyncManager::Status::Summary StatusSummary; @@ -105,6 +108,10 @@ class SyncBackendHost { // GAIA) has confirmed the username is authentic. string16 GetAuthenticatedUsername() const; + // ModelSafeWorkerRegistrar implementation. + virtual void GetWorkers(std::vector* out); + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out); + #if defined(UNIT_TEST) // Called from unit test to bypass authentication and initialize the syncapi // to a state suitable for testing but not production. @@ -113,12 +120,12 @@ class SyncBackendHost { sync_api::HttpPostProviderFactory* auth_factory) { if (!core_thread_.Start()) return; - bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_); + registrar_.workers[GROUP_UI] = new UIModelWorker(frontend_loop_); + registrar_.routing_info[syncable::BOOKMARKS] = GROUP_UI; core_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoInitializeForTest, - bookmark_model_worker_, test_user, factory, auth_factory)); @@ -152,7 +159,6 @@ class SyncBackendHost { // Called on the SyncBackendHost core_thread_ to perform initialization // of the syncapi on behalf of SyncBackendHost::Initialize. void DoInitialize(const GURL& service_url, - BookmarkModelWorker* bookmark_model_worker, bool attempt_last_user_authentication, sync_api::HttpPostProviderFactory* http_bridge_factory, sync_api::HttpPostProviderFactory* auth_http_bridge_factory, @@ -185,12 +191,10 @@ class SyncBackendHost { // Special form of initialization that does not try and authenticate the // last known user (since it will fail in test mode) and does some extra // setup to nudge the syncapi into a useable state. - void DoInitializeForTest(BookmarkModelWorker* bookmark_model_worker, - const std::wstring& test_user, + void DoInitializeForTest(const std::wstring& test_user, sync_api::HttpPostProviderFactory* factory, sync_api::HttpPostProviderFactory* auth_factory) { - DoInitialize(GURL(), bookmark_model_worker, false, factory, - auth_factory, std::string()); + DoInitialize(GURL(), false, factory, auth_factory, std::string()); syncapi_->SetupForTestMode(test_user); } #endif @@ -248,6 +252,8 @@ class SyncBackendHost { DISALLOW_COPY_AND_ASSIGN(Core); }; + UIModelWorker* ui_worker(); + // A thread we dedicate for use by our Core to perform initialization, // authentication, handle messages from the syncapi, and periodically tell // the syncapi to persist itself. @@ -260,12 +266,32 @@ class SyncBackendHost { // to safely talk back to the SyncFrontend. MessageLoop* const frontend_loop_; - // We hold on to the BookmarkModelWorker created for the syncapi to ensure - // shutdown occurs in the sequence we expect by calling Stop() at the - // appropriate time. It is guaranteed to be valid because the worker is - // only destroyed when the SyncManager is destroyed, which happens when - // our Core is destroyed, which happens in Shutdown(). - BookmarkModelWorker* bookmark_model_worker_; + // This is state required to implement ModelSafeWorkerRegistrar. + struct { + // We maintain ownership of all workers. In some cases, we need to ensure + // shutdown occurs in an expected sequence by Stop()ing certain workers. + // They are guaranteed to be valid because we only destroy elements of + // |workers_| after the syncapi has been destroyed. Unless a worker is no + // longer needed because all types that get routed to it have been disabled + // (from syncing). In that case, we'll destroy on demand *after* routing + // any dependent types to GROUP_PASSIVE, so that the syncapi doesn't call + // into garbage. If an index is non-NULL, it means at least one ModelType + // that routes to that model safe group is being synced. + browser_sync::ModelSafeWorker* + workers[browser_sync::MODEL_SAFE_GROUP_COUNT]; + browser_sync::ModelSafeRoutingInfo routing_info; + } registrar_; + + // The user can incur changes to registrar_ at any time from the UI thread. + // The syncapi needs to periodically get a consistent snapshot of the state, + // and it does so from a different thread. Therefore, we protect creation, + // destruction, and re-routing events by acquiring this lock. Note that the + // SyncBackendHost may read (on the UI thread or core thread) from registrar_ + // without acquiring the lock (which is typically "read ModelSafeWorker + // pointer value", and then invoke methods), because lifetimes are managed on + // the UI thread. Of course, this comment only applies to ModelSafeWorker + // impls that are themselves thread-safe, such as UIModelWorker. + Lock registrar_lock_; // The frontend which we serve (and are owned by). SyncFrontend* frontend_; diff --git a/chrome/browser/sync/glue/ui_model_worker.cc b/chrome/browser/sync/glue/ui_model_worker.cc new file mode 100644 index 0000000..d11ad5b --- /dev/null +++ b/chrome/browser/sync/glue/ui_model_worker.cc @@ -0,0 +1,106 @@ +// Copyright (c) 2006-2008 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/glue/ui_model_worker.h" + +#include "base/message_loop.h" +#include "base/waitable_event.h" + +namespace browser_sync { + +void UIModelWorker::DoWorkAndWaitUntilDone(Closure* work) { + // It is possible this gets called when we are in the STOPPING state, because + // the UI loop has initiated shutdown but the syncer hasn't got the memo yet. + // This is fine, the work will get scheduled and run normally or run by our + // code handling this case in Stop(). + DCHECK_NE(state_, STOPPED); + if (state_ == STOPPED) + return; + if (MessageLoop::current() == ui_loop_) { + DLOG(WARNING) << "DoWorkAndWaitUntilDone called from " + << "ui_loop_. Probably a nested invocation?"; + work->Run(); + return; + } + + // Create an unsignaled event to wait on. + base::WaitableEvent work_done(false, false); + { + // We lock only to avoid PostTask'ing a NULL pending_work_ (because it + // could get Run() in Stop() and call OnTaskCompleted before we post). + // The task is owned by the message loop as per usual. + AutoLock lock(lock_); + DCHECK(!pending_work_); + pending_work_ = new CallDoWorkAndSignalTask(work, &work_done, this); + ui_loop_->PostTask(FROM_HERE, pending_work_); + } + syncapi_event_.Signal(); // Notify that the syncapi produced work for us. + work_done.Wait(); +} + +UIModelWorker::~UIModelWorker() { + DCHECK_EQ(state_, STOPPED); +} + +void UIModelWorker::OnSyncerShutdownComplete() { + AutoLock lock(lock_); + // The SyncerThread has terminated and we are no longer needed by syncapi. + // The UI loop initiated shutdown and is (or will be) waiting in Stop(). + // We could either be WORKING or RUNNING_MANUAL_SHUTDOWN_PUMP, depending + // on where we timeslice the UI thread in Stop; but we can't be STOPPED, + // because that would imply OnSyncerShutdownComplete already signaled. + DCHECK_NE(state_, STOPPED); + + syncapi_has_shutdown_ = true; + syncapi_event_.Signal(); +} + +void UIModelWorker::Stop() { + DCHECK_EQ(MessageLoop::current(), ui_loop_); + + AutoLock lock(lock_); + DCHECK_EQ(state_, WORKING); + + // We're on our own now, the beloved UI MessageLoop is no longer running. + // Any tasks scheduled or to be scheduled on the UI MessageLoop will not run. + state_ = RUNNING_MANUAL_SHUTDOWN_PUMP; + + // Drain any final tasks manually until the SyncerThread tells us it has + // totally finished. There should only ever be 0 or 1 tasks Run() here. + while (!syncapi_has_shutdown_) { + if (pending_work_) + pending_work_->Run(); // OnTaskCompleted will set pending_work_ to NULL. + + // Wait for either a new task or SyncerThread termination. + syncapi_event_.Wait(); + } + + state_ = STOPPED; +} + +void UIModelWorker::CallDoWorkAndSignalTask::Run() { + if (!work_) { + // This can happen during tests or cases where there are more than just the + // default UIModelWorker in existence and it gets destroyed before + // the main UI loop has terminated. There is no easy way to assert the + // loop is running / not running at the moment, so we just provide cancel + // semantics here and short-circuit. + // TODO(timsteele): Maybe we should have the message loop destruction + // observer fire when the loop has ended, just a bit before it + // actually gets destroyed. + return; + } + work_->Run(); + + // Sever ties with work_ to allow the sanity-checking above that we don't + // get run twice. + work_ = NULL; + + // Notify the UIModelWorker that scheduled us that we have run + // successfully. + scheduler_->OnTaskCompleted(); + work_done_->Signal(); // Unblock the syncer thread that scheduled us. +} + +} // namespace browser_sync diff --git a/chrome/browser/sync/glue/ui_model_worker.h b/chrome/browser/sync/glue/ui_model_worker.h new file mode 100644 index 0000000..de87795 --- /dev/null +++ b/chrome/browser/sync/glue/ui_model_worker.h @@ -0,0 +1,139 @@ +// Copyright (c) 2006-2008 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. + +#ifndef CHROME_BROWSER_SYNC_GLUE_UI_MODEL_WORKER_H_ +#define CHROME_BROWSER_SYNC_GLUE_UI_MODEL_WORKER_H_ + +#include "base/condition_variable.h" +#include "base/lock.h" +#include "base/task.h" +#include "base/waitable_event.h" +#include "chrome/browser/sync/engine/syncapi.h" +#include "chrome/browser/sync/engine/model_safe_worker.h" +#include "chrome/browser/sync/util/closure.h" + +class MessageLoop; + +namespace browser_sync { + +// A ModelSafeWorker for UI models (e.g. bookmarks) that accepts work requests +// from the syncapi that need to be fulfilled from the MessageLoop home to the +// native model. +// +// Lifetime note: Instances of this class will generally be owned by the +// SyncerThread. When the SyncerThread _object_ is destroyed, the +// UIModelWorker will be destroyed. The SyncerThread object is destroyed +// after the actual syncer pthread has exited. +class UIModelWorker : public browser_sync::ModelSafeWorker { + public: + explicit UIModelWorker(MessageLoop* ui_loop) + : state_(WORKING), + pending_work_(NULL), + syncapi_has_shutdown_(false), + ui_loop_(ui_loop), + syncapi_event_(&lock_) { + } + virtual ~UIModelWorker(); + + // A simple task to signal a waitable event after Run()ning a Closure. + class CallDoWorkAndSignalTask : public Task { + public: + CallDoWorkAndSignalTask(Closure* work, + base::WaitableEvent* work_done, + UIModelWorker* scheduler) + : work_(work), work_done_(work_done), scheduler_(scheduler) { + } + virtual ~CallDoWorkAndSignalTask() { } + + // Task implementation. + virtual void Run(); + + private: + // Task data - a closure and a waitable event to signal after the work has + // been done. + Closure* work_; + base::WaitableEvent* work_done_; + + // The UIModelWorker responsible for scheduling us. + UIModelWorker* const scheduler_; + + DISALLOW_COPY_AND_ASSIGN(CallDoWorkAndSignalTask); + }; + + // Called by the UI thread on shutdown of the sync service. Blocks until + // the UIModelWorker has safely met termination conditions, namely that + // no task scheduled by CallDoWorkFromModelSafeThreadAndWait remains un- + // processed and that syncapi will not schedule any further work for us to do. + void Stop(); + + // ModelSafeWorker implementation. Called on syncapi SyncerThread. + virtual void DoWorkAndWaitUntilDone(Closure* work); + virtual ModelSafeGroup GetModelSafeGroup() { return GROUP_UI; } + + // Upon receiving this idempotent call, the ModelSafeWorker can + // assume no work will ever be scheduled again from now on. If it has any work + // that it has not yet completed, it must make sure to run it as soon as + // possible as the Syncer is trying to shut down. Called from the CoreThread. + void OnSyncerShutdownComplete(); + + // Callback from |pending_work_| to notify us that it has been run. + // Called on |ui_loop_|. + void OnTaskCompleted() { pending_work_ = NULL; } + + private: + // The life-cycle of a UIModelWorker in three states. + enum State { + // We hit the ground running in this state and remain until + // the UI loop calls Stop(). + WORKING, + // Stop() sequence has been initiated, but we have not received word that + // the SyncerThread has terminated and doesn't need us anymore. Since the + // UI MessageLoop is not running at this point, we manually process any + // last pending_task_ that the Syncer throws at us, effectively dedicating + // the UI thread to terminating the Syncer. + RUNNING_MANUAL_SHUTDOWN_PUMP, + // We have come to a complete stop, no scheduled work remains, and no work + // will be scheduled from now until our destruction. + STOPPED, + }; + + // This is set by the UI thread, but is not explicitly thread safe, so only + // read this value from other threads when you know it is absolutely safe (e.g + // there is _no_ way we can be in CallDoWork with state_ = STOPPED, so it is + // safe to read / compare in this case). + State state_; + + // We keep a reference to any task we have scheduled so we can gracefully + // force them to run if the syncer is trying to shutdown. + Task* pending_work_; + + // Set by the SyncCoreThread when Syncapi shutdown has completed and the + // SyncerThread has terminated, so no more work will be scheduled. Read by + // the UI thread in Stop(). + bool syncapi_has_shutdown_; + + // The UI model home-sweet-home MessageLoop. + MessageLoop* const ui_loop_; + + // We use a Lock for all data members and a ConditionVariable to synchronize. + // We do this instead of using a WaitableEvent and a bool condition in order + // to guard against races that could arise due to the fact that the lack of a + // barrier permits instructions to be reordered by compiler optimizations. + // Possible or not, that route makes for very fragile code due to existence + // of theoretical races. + Lock lock_; + + // Used as a barrier at shutdown to ensure the SyncerThread terminates before + // we allow the UI thread to return from Stop(). This gets signalled whenever + // one of two events occur: a new pending_work_ task was scheduled, or the + // SyncerThread has terminated. We only care about (1) when we are in Stop(), + // because we have to manually Run() the task. + ConditionVariable syncapi_event_; + + DISALLOW_COPY_AND_ASSIGN(UIModelWorker); +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_UI_MODEL_WORKER_H_ diff --git a/chrome/browser/sync/glue/ui_model_worker_unittest.cc b/chrome/browser/sync/glue/ui_model_worker_unittest.cc new file mode 100644 index 0000000..a4fc391 --- /dev/null +++ b/chrome/browser/sync/glue/ui_model_worker_unittest.cc @@ -0,0 +1,222 @@ +// Copyright (c) 2006-2009 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 "base/thread.h" +#include "chrome/browser/sync/engine/syncapi.h" +#include "chrome/browser/sync/glue/ui_model_worker.h" +#include "testing/gtest/include/gtest/gtest.h" + +using browser_sync::UIModelWorker; +using namespace sync_api; + +// Various boilerplate, primarily for the StopWithPendingWork test. + +class UIModelWorkerVisitor { + public: + UIModelWorkerVisitor(MessageLoop* faux_ui_loop, + base::WaitableEvent* was_run, + bool quit_loop) + : faux_ui_loop_(faux_ui_loop), quit_loop_when_run_(quit_loop), + was_run_(was_run) { } + virtual ~UIModelWorkerVisitor() { } + + virtual void DoWork() { + EXPECT_EQ(MessageLoop::current(), faux_ui_loop_); + was_run_->Signal(); + if (quit_loop_when_run_) + MessageLoop::current()->Quit(); + } + + private: + MessageLoop* faux_ui_loop_; + bool quit_loop_when_run_; + base::WaitableEvent* was_run_; + DISALLOW_COPY_AND_ASSIGN(UIModelWorkerVisitor); +}; + +// A faux-syncer that only interacts with its model safe worker. +class Syncer { + public: + explicit Syncer(UIModelWorker* worker) : worker_(worker) {} + ~Syncer() {} + + void SyncShare(UIModelWorkerVisitor* visitor) { + worker_->DoWorkAndWaitUntilDone(NewCallback(visitor, + &UIModelWorkerVisitor::DoWork)); + } + private: + UIModelWorker* worker_; + DISALLOW_COPY_AND_ASSIGN(Syncer); +}; + +// A task run from the SyncerThread to "sync share", ie tell the Syncer to +// ask it's ModelSafeWorker to do something. +class FakeSyncShareTask : public Task { + public: + FakeSyncShareTask(Syncer* syncer, UIModelWorkerVisitor* visitor) + : syncer_(syncer), visitor_(visitor) { + } + virtual void Run() { + syncer_->SyncShare(visitor_); + } + private: + Syncer* syncer_; + UIModelWorkerVisitor* visitor_; + DISALLOW_COPY_AND_ASSIGN(FakeSyncShareTask); +}; + +// A task run from the CoreThread to simulate terminating syncapi. +class FakeSyncapiShutdownTask : public Task { + public: + FakeSyncapiShutdownTask(base::Thread* syncer_thread, + UIModelWorker* worker, + base::WaitableEvent** jobs, + size_t job_count) + : syncer_thread_(syncer_thread), worker_(worker), jobs_(jobs), + job_count_(job_count), all_jobs_done_(false, false) { } + virtual void Run() { + // In real life, we would try and close a sync directory, which would + // result in the syncer calling it's own destructor, which results in + // the SyncerThread::HaltSyncer being called, which sets the + // syncer in RequestEarlyExit mode and waits until the Syncer finishes + // SyncShare to remove the syncer from it's watch. Here we just manually + // wait until all outstanding jobs are done to simulate what happens in + // SyncerThread::HaltSyncer. + all_jobs_done_.WaitMany(jobs_, job_count_); + + // These two calls are made from SyncBackendHost::Core::DoShutdown. + syncer_thread_->Stop(); + worker_->OnSyncerShutdownComplete(); + } + private: + base::Thread* syncer_thread_; + UIModelWorker* worker_; + base::WaitableEvent** jobs_; + size_t job_count_; + base::WaitableEvent all_jobs_done_; + DISALLOW_COPY_AND_ASSIGN(FakeSyncapiShutdownTask); +}; + +class UIModelWorkerTest : public testing::Test { + public: + UIModelWorkerTest() : faux_syncer_thread_("FauxSyncerThread"), + faux_core_thread_("FauxCoreThread") { } + + virtual void SetUp() { + faux_syncer_thread_.Start(); + bmw_.reset(new UIModelWorker(&faux_ui_loop_)); + syncer_.reset(new Syncer(bmw_.get())); + } + + Syncer* syncer() { return syncer_.get(); } + UIModelWorker* bmw() { return bmw_.get(); } + base::Thread* core_thread() { return &faux_core_thread_; } + base::Thread* syncer_thread() { return &faux_syncer_thread_; } + MessageLoop* ui_loop() { return &faux_ui_loop_; } + private: + MessageLoop faux_ui_loop_; + base::Thread faux_syncer_thread_; + base::Thread faux_core_thread_; + scoped_ptr bmw_; + scoped_ptr syncer_; +}; + +TEST_F(UIModelWorkerTest, ScheduledWorkRunsOnUILoop) { + base::WaitableEvent v_was_run(false, false); + scoped_ptr v( + new UIModelWorkerVisitor(ui_loop(), &v_was_run, true)); + + syncer_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncShareTask(syncer(), v.get())); + + // We are on the UI thread, so run our loop to process the + // (hopefully) scheduled task from a SyncShare invocation. + MessageLoop::current()->Run(); + + bmw()->OnSyncerShutdownComplete(); + bmw()->Stop(); + syncer_thread()->Stop(); +} + +TEST_F(UIModelWorkerTest, StopWithPendingWork) { + // What we want to set up is the following: + // ("ui_thread" is the thread we are currently executing on) + // 1 - simulate the user shutting down the browser, and the ui thread needing + // to terminate the core thread. + // 2 - the core thread is where the syncapi is accessed from, and so it needs + // to shut down the SyncerThread. + // 3 - the syncer is waiting on the UIModelWorker to + // perform a task for it. + // The UIModelWorker's manual shutdown pump will save the day, as the + // UI thread is not actually trying to join() the core thread, it is merely + // waiting for the SyncerThread to give it work or to finish. After that, it + // will join the core thread which should succeed as the SyncerThread has left + // the building. Unfortunately this test as written is not provably decidable, + // as it will always halt on success, but it may not on failure (namely if + // the task scheduled by the Syncer is _never_ run). + core_thread()->Start(); + base::WaitableEvent v_ran(false, false); + scoped_ptr v(new UIModelWorkerVisitor( + ui_loop(), &v_ran, false)); + base::WaitableEvent* jobs[] = { &v_ran }; + + // The current message loop is not running, so queue a task to cause + // UIModelWorker::Stop() to play a crucial role. See comment below. + syncer_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncShareTask(syncer(), v.get())); + + // This is what gets the core_thread blocked on the syncer_thread. + core_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncapiShutdownTask(syncer_thread(), bmw(), jobs, 1)); + + // This is what gets the UI thread blocked until NotifyExitRequested, + // which is called when FakeSyncapiShutdownTask runs and deletes the syncer. + bmw()->Stop(); + + EXPECT_FALSE(syncer_thread()->IsRunning()); + core_thread()->Stop(); +} + +TEST_F(UIModelWorkerTest, HypotheticalManualPumpFlooding) { + // This situation should not happen in real life because the Syncer should + // never send more than one CallDoWork notification after early_exit_requested + // has been set, but our UIModelWorker is built to handle this case + // nonetheless. It may be needed in the future, and since we support it and + // it is not actually exercised in the wild this test is essential. + // It is identical to above except we schedule more than one visitor. + core_thread()->Start(); + + // Our ammunition. + base::WaitableEvent fox1_ran(false, false); + scoped_ptr fox1(new UIModelWorkerVisitor( + ui_loop(), &fox1_ran, false)); + base::WaitableEvent fox2_ran(false, false); + scoped_ptr fox2(new UIModelWorkerVisitor( + ui_loop(), &fox2_ran, false)); + base::WaitableEvent fox3_ran(false, false); + scoped_ptr fox3(new UIModelWorkerVisitor( + ui_loop(), &fox3_ran, false)); + base::WaitableEvent* jobs[] = { &fox1_ran, &fox2_ran, &fox3_ran }; + + // The current message loop is not running, so queue a task to cause + // UIModelWorker::Stop() to play a crucial role. See comment below. + syncer_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncShareTask(syncer(), fox1.get())); + syncer_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncShareTask(syncer(), fox2.get())); + + // This is what gets the core_thread blocked on the syncer_thread. + core_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncapiShutdownTask(syncer_thread(), bmw(), jobs, 3)); + syncer_thread()->message_loop()->PostTask(FROM_HERE, + new FakeSyncShareTask(syncer(), fox3.get())); + + // This is what gets the UI thread blocked until NotifyExitRequested, + // which is called when FakeSyncapiShutdownTask runs and deletes the syncer. + bmw()->Stop(); + + // Was the thread killed? + EXPECT_FALSE(syncer_thread()->IsRunning()); + core_thread()->Stop(); +} diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc index 296c532..5a0a3e8 100644 --- a/chrome/browser/sync/sessions/sync_session.cc +++ b/chrome/browser/sync/sessions/sync_session.cc @@ -14,6 +14,23 @@ SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate) write_transaction_(NULL), delegate_(delegate), auth_failure_occurred_(false) { + + std::vector* s = + const_cast* >(&workers_); + context_->registrar()->GetWorkers(s); + + // TODO(tim): Use ModelSafeRoutingInfo to silo parts of the session status by + // ModelSafeGroup; + // e.g. have a map, map etc. + // The routing will be used to map multiple model types into the right silo. + // The routing info can't change throughout a session, so we're assured that + // (for example) commit_ids for syncable::AUTOFILL items that were being + // processed as part of the GROUP_PASSIVE run (because they weren't being + // synced) *continue* to be for this whole session, even though the + // ModelSafeWorkerRegistrar may be configured to route syncable::AUTOFILL to + // GROUP_DB now. + group_restriction_in_effect_ = false; + group_restriction_ = GROUP_PASSIVE; } SyncSessionSnapshot SyncSession::TakeSnapshot() const { diff --git a/chrome/browser/sync/sessions/sync_session.h b/chrome/browser/sync/sessions/sync_session.h index 4f44596..3d990a7 100644 --- a/chrome/browser/sync/sessions/sync_session.h +++ b/chrome/browser/sync/sessions/sync_session.h @@ -15,6 +15,7 @@ #define CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSION_H_ #include +#include #include "base/basictypes.h" #include "base/scoped_ptr.h" @@ -29,6 +30,8 @@ class WriteTransaction; } namespace browser_sync { +class ModelSafeWorker; + namespace sessions { class SyncSession { @@ -99,11 +102,14 @@ class SyncSession { source_ = source; } + const std::vector& workers() const { return workers_; } + private: // Extend the encapsulation boundary to utilities for internal member // assignments. This way, the scope of these actions is explicit, they can't // be overridden, and assigning is always accompanied by unassigning. friend class ScopedSetSessionWriteTransaction; + friend class ScopedModelSafeGroupRestriction; // The context for this session, guaranteed to outlive |this|. SyncSessionContext* const context_; @@ -126,6 +132,14 @@ class SyncSession { // Used to determine if an auth error notification should be sent out. bool auth_failure_occurred_; + // The set of active ModelSafeWorkers for the duration of this session. + const std::vector workers_; + + // Used to fail read/write operations on this SyncSession that don't obey the + // currently active ModelSafeWorker contract. + bool group_restriction_in_effect_; + ModelSafeGroup group_restriction_; + DISALLOW_COPY_AND_ASSIGN(SyncSession); }; @@ -147,6 +161,26 @@ class ScopedSetSessionWriteTransaction { DISALLOW_COPY_AND_ASSIGN(ScopedSetSessionWriteTransaction); }; +// A utility to restrict access to only those parts of the given SyncSession +// that pertain to the specified ModelSafeGroup. See +// SyncSession::ModelSafetyRestriction. +class ScopedModelSafeGroupRestriction { + public: + ScopedModelSafeGroupRestriction(SyncSession* to_restrict, + ModelSafeGroup restriction) + : session_(to_restrict) { + DCHECK(!session_->group_restriction_in_effect_); + session_->group_restriction_in_effect_ = true; + session_->group_restriction_ = restriction; + } + ~ScopedModelSafeGroupRestriction() { + session_->group_restriction_in_effect_ = true; + } + private: + SyncSession* session_; + DISALLOW_COPY_AND_ASSIGN(ScopedModelSafeGroupRestriction); +}; + } // namespace sessions } // namespace browser_sync diff --git a/chrome/browser/sync/sessions/sync_session_context.h b/chrome/browser/sync/sessions/sync_session_context.h index 40deab1..3565ec6 100644 --- a/chrome/browser/sync/sessions/sync_session_context.h +++ b/chrome/browser/sync/sessions/sync_session_context.h @@ -31,6 +31,7 @@ class DirectoryManager; namespace browser_sync { class ConflictResolver; +class ModelSafeWorkerRegistrar; class ServerConnectionManager; namespace sessions { @@ -41,12 +42,12 @@ class SyncSessionContext { public: SyncSessionContext(ServerConnectionManager* connection_manager, syncable::DirectoryManager* directory_manager, - ModelSafeWorker* model_safe_worker) + ModelSafeWorkerRegistrar* model_safe_worker_registrar) : resolver_(NULL), syncer_event_channel_(NULL), connection_manager_(connection_manager), directory_manager_(directory_manager), - model_safe_worker_(model_safe_worker), + registrar_(model_safe_worker_registrar), extensions_activity_monitor_(new ExtensionsActivityMonitor()), notifications_enabled_(false) { } @@ -69,8 +70,8 @@ class SyncSessionContext { SyncerEventChannel* syncer_event_channel() { return syncer_event_channel_; } - ModelSafeWorker* model_safe_worker() { - return model_safe_worker_.get(); + ModelSafeWorkerRegistrar* registrar() { + return registrar_; } ExtensionsActivityMonitor* extensions_monitor() { return extensions_activity_monitor_; @@ -103,9 +104,9 @@ class SyncSessionContext { ServerConnectionManager* const connection_manager_; syncable::DirectoryManager* const directory_manager_; - // A worker capable of processing work closures on a thread that is - // guaranteed to be safe for model modifications. - scoped_ptr model_safe_worker_; + // A registrar of workers capable of processing work closures on a thread + // that is guaranteed to be safe for model modifications. + ModelSafeWorkerRegistrar* registrar_; // We use this to stuff extensions activity into CommitMessages so the server // can correlate commit traffic with extension-related bookmark mutations. diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc index e2a3c12..5ac602d 100644 --- a/chrome/browser/sync/sessions/sync_session_unittest.cc +++ b/chrome/browser/sync/sessions/sync_session_unittest.cc @@ -19,11 +19,12 @@ namespace sessions { namespace { class SyncSessionTest : public testing::Test, - public SyncSession::Delegate { + public SyncSession::Delegate, + public ModelSafeWorkerRegistrar { public: SyncSessionTest() : controller_invocations_allowed_(false) {} virtual void SetUp() { - context_.reset(new SyncSessionContext(NULL, NULL, NULL)); + context_.reset(new SyncSessionContext(NULL, NULL, this)); session_.reset(new SyncSession(context_.get(), this)); } virtual void TearDown() { @@ -47,6 +48,10 @@ class SyncSessionTest : public testing::Test, FailControllerInvocationIfDisabled("OnReceivedShortPollIntervalUpdate"); } + // ModelSafeWorkerRegistrar implementation. + virtual void GetWorkers(std::vector* out) {} + virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {} + StatusController* status() { return session_->status_controller(); } protected: void FailControllerInvocationIfDisabled(const std::string& msg) { @@ -78,7 +83,7 @@ TEST_F(SyncSessionTest, SetWriteTransaction) { TestDirectorySetterUpper db; db.SetUp(); session_.reset(NULL); - context_.reset(new SyncSessionContext(NULL, db.manager(), NULL)); + context_.reset(new SyncSessionContext(NULL, db.manager(), this)); session_.reset(new SyncSession(context_.get(), this)); context_->set_account_name(db.name()); syncable::ScopedDirLookup dir(context_->directory_manager(), diff --git a/chrome/browser/sync/syncable/model_type.h b/chrome/browser/sync/syncable/model_type.h new file mode 100644 index 0000000..7bd9826 --- /dev/null +++ b/chrome/browser/sync/syncable/model_type.h @@ -0,0 +1,27 @@ +// 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. + +#ifndef CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ +#define CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ + +#include "base/logging.h" + +namespace syncable { + +enum ModelType { + UNSPECIFIED = 0, + BOOKMARKS, + MODEL_TYPE_COUNT +}; + +inline ModelType ModelTypeFromInt(int i) { + DCHECK_GE(i, 0); + DCHECK_LT(i, MODEL_TYPE_COUNT); + return static_cast(i); +} + +} // namespace syncable + +#endif // CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ + diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index d0dc3a6..3d0b066 100755 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -835,6 +835,7 @@ 'browser/sync/syncable/directory_event.h', 'browser/sync/syncable/directory_manager.cc', 'browser/sync/syncable/directory_manager.h', + 'browser/sync/syncable/model_type.h', 'browser/sync/syncable/path_name_cmp.h', 'browser/sync/syncable/syncable-inl.h', 'browser/sync/syncable/syncable.cc', diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 79b73c0..de11a5d 100755 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1530,8 +1530,6 @@ 'browser/transport_security_persister.cc', 'browser/transport_security_persister.h', 'browser/sync/engine/syncapi.h', - 'browser/sync/glue/bookmark_model_worker.cc', - 'browser/sync/glue/bookmark_model_worker.h', 'browser/sync/glue/bookmark_change_processor.h', 'browser/sync/glue/bookmark_change_processor.cc', 'browser/sync/glue/bookmark_model_associator.h', @@ -1546,7 +1544,9 @@ 'browser/sync/glue/preference_model_associator.cc', 'browser/sync/glue/preference_model_associator.h', 'browser/sync/glue/sync_backend_host.cc', - 'browser/sync/glue/sync_backend_host.h', + 'browser/sync/glue/sync_backend_host.h', + 'browser/sync/glue/ui_model_worker.cc', + 'browser/sync/glue/ui_model_worker.h', 'browser/sync/profile_sync_service.cc', 'browser/sync/profile_sync_service.h', 'browser/sync/sync_setup_flow.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 96726e4..b989f3a 100755 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -784,8 +784,8 @@ 'browser/shell_integration_unittest.cc', 'browser/spellchecker_platform_engine_unittest.cc', 'browser/ssl/ssl_host_state_unittest.cc', - 'browser/sync/glue/bookmark_model_worker_unittest.cc', 'browser/sync/glue/http_bridge_unittest.cc', + 'browser/sync/glue/ui_model_worker_unittest.cc', 'browser/sync/profile_sync_service_unittest.cc', 'browser/sync/sync_setup_wizard_unittest.cc', 'browser/sync/sync_ui_util_mac_unittest.mm', -- cgit v1.1