diff options
19 files changed, 426 insertions, 163 deletions
diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc index 1580dc4..77d2f5f 100644 --- a/chrome/browser/sync/engine/all_status.cc +++ b/chrome/browser/sync/engine/all_status.cc @@ -127,9 +127,6 @@ void AllStatus::HandleServerConnectionEvent( status_.server_reachable = event.server_reachable; if (event.connection_code == HttpResponse::SERVER_CONNECTION_OK) { - if (!status_.authenticated) { - status_ = CreateBlankStatus(); - } status_.authenticated = true; } else { status_.authenticated = false; diff --git a/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc b/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc index 9a3e6ab..02ec54d 100644 --- a/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc +++ b/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc @@ -82,17 +82,5 @@ TEST_F(CleanupDisabledTypesCommandTest, TypeDisabled) { command.ExecuteImpl(session()); } -TEST_F(CleanupDisabledTypesCommandTest, - SyncerEndCommandSetsPreviousRoutingInfo) { - SyncerEndCommand command; - - ModelSafeRoutingInfo info; - EXPECT_TRUE(info == session()->context()->previous_session_routing_info()); - command.ExecuteImpl(session()); - ASSERT_FALSE(routing_info().empty()); - EXPECT_TRUE(routing_info() == - session()->context()->previous_session_routing_info()); -} - } // namespace browser_sync diff --git a/chrome/browser/sync/engine/model_safe_worker.cc b/chrome/browser/sync/engine/model_safe_worker.cc index c488e3b..d9f44bf 100644 --- a/chrome/browser/sync/engine/model_safe_worker.cc +++ b/chrome/browser/sync/engine/model_safe_worker.cc @@ -14,7 +14,7 @@ ModelSafeGroup GetGroupForModelType(const syncable::ModelType type, // with the server's PermanentItemPopulator is causing TLF updates in // some cases. See bug 36735. if (type != syncable::UNSPECIFIED && type != syncable::TOP_LEVEL_FOLDER) - NOTREACHED() << "Entry does not belong to active ModelSafeGroup!"; + LOG(WARNING) << "Entry does not belong to active ModelSafeGroup!"; return GROUP_PASSIVE; } return it->second; diff --git a/chrome/browser/sync/engine/net/server_connection_manager.cc b/chrome/browser/sync/engine/net/server_connection_manager.cc index 7f53a29..4598588 100644 --- a/chrome/browser/sync/engine/net/server_connection_manager.cc +++ b/chrome/browser/sync/engine/net/server_connection_manager.cc @@ -10,6 +10,7 @@ #include <string> #include <vector> +#include "base/command_line.h" #include "build/build_config.h" #include "chrome/browser/sync/engine/net/url_translator.h" #include "chrome/browser/sync/engine/syncapi.h" @@ -17,6 +18,7 @@ #include "chrome/browser/sync/engine/syncproto.h" #include "chrome/browser/sync/protocol/sync.pb.h" #include "chrome/browser/sync/syncable/directory_manager.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/net/http_return.h" #include "googleurl/src/gurl.h" @@ -144,6 +146,7 @@ ServerConnectionManager::ServerConnectionManager( get_time_path_(kSyncServerGetTimePath), error_count_(0), channel_(new Channel(shutdown_event)), + listeners_(new ObserverListThreadSafe<ServerConnectionEventListener>()), server_status_(HttpResponse::NONE), server_reachable_(false), reset_count_(0), @@ -155,10 +158,16 @@ ServerConnectionManager::~ServerConnectionManager() { } void ServerConnectionManager::NotifyStatusChanged() { - ServerConnectionEvent event = { ServerConnectionEvent::STATUS_CHANGED, - server_status_, - server_reachable_ }; - channel_->NotifyListeners(event); + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kNewSyncerThread)) { + listeners_->Notify(&ServerConnectionEventListener::OnServerConnectionEvent, + ServerConnectionEvent2(server_status_, server_reachable_)); + } else { + ServerConnectionEvent event = { ServerConnectionEvent::STATUS_CHANGED, + server_status_, + server_reachable_ }; + channel_->NotifyListeners(event); + } } bool ServerConnectionManager::PostBufferWithCachedAuth( @@ -329,6 +338,16 @@ std::string ServerConnectionManager::GetServerHost() const { return gurl.host(); } +void ServerConnectionManager::AddListener( + ServerConnectionEventListener* listener) { + listeners_->AddObserver(listener); +} + +void ServerConnectionManager::RemoveListener( + ServerConnectionEventListener* listener) { + listeners_->RemoveObserver(listener); +} + ServerConnectionManager::Post* ServerConnectionManager::MakePost() { return NULL; // For testing. } diff --git a/chrome/browser/sync/engine/net/server_connection_manager.h b/chrome/browser/sync/engine/net/server_connection_manager.h index 214c622..9f49d4a 100644 --- a/chrome/browser/sync/engine/net/server_connection_manager.h +++ b/chrome/browser/sync/engine/net/server_connection_manager.h @@ -10,6 +10,7 @@ #include <string> #include "base/atomicops.h" +#include "base/observer_list_threadsafe.h" #include "base/string_util.h" #include "base/synchronization/lock.h" #include "chrome/browser/sync/syncable/syncable_id.h" @@ -107,6 +108,7 @@ inline bool IsGoodReplyFromServer(HttpResponse::ServerConnectionCode code) { return code >= HttpResponse::SERVER_CONNECTION_OK; } +// TODO(tim): Deprecated. struct ServerConnectionEvent { // Traits. typedef ServerConnectionEvent EventType; @@ -124,6 +126,21 @@ struct ServerConnectionEvent { bool server_reachable; }; +struct ServerConnectionEvent2 { + HttpResponse::ServerConnectionCode connection_code; + bool server_reachable; + ServerConnectionEvent2(HttpResponse::ServerConnectionCode code, + bool server_reachable) : + connection_code(code), server_reachable(server_reachable) {} +}; + +class ServerConnectionEventListener { + public: + virtual void OnServerConnectionEvent(const ServerConnectionEvent2& event) = 0; + protected: + virtual ~ServerConnectionEventListener() {} +}; + class ServerConnectionManager; // A helper class that automatically notifies when the status changes. // TODO(tim): This class shouldn't be exposed outside of the implementation, @@ -240,6 +257,9 @@ class ServerConnectionManager { inline Channel* channel() const { return channel_; } + void AddListener(ServerConnectionEventListener* listener); + void RemoveListener(ServerConnectionEventListener* listener); + inline std::string user_agent() const { return user_agent_; } inline HttpResponse::ServerConnectionCode server_status() const { @@ -344,8 +364,12 @@ class ServerConnectionManager { base::Lock error_count_mutex_; // Protects error_count_ int error_count_; // Tracks the number of connection errors. + // TODO(tim): Deprecated. Channel* const channel_; + scoped_refptr<ObserverListThreadSafe<ServerConnectionEventListener> > + listeners_; + // Volatile so various threads can call server_status() without // synchronization. volatile HttpResponse::ServerConnectionCode server_status_; diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index 354cbcf1..8159316 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -72,6 +72,8 @@ using browser_sync::ModelSafeRoutingInfo; using browser_sync::ModelSafeWorker; using browser_sync::ModelSafeWorkerRegistrar; using browser_sync::ServerConnectionEvent; +using browser_sync::ServerConnectionEvent2; +using browser_sync::ServerConnectionEventListener; using browser_sync::SyncEngineEvent; using browser_sync::SyncEngineEventListener; using browser_sync::Syncer; @@ -1099,7 +1101,8 @@ class SyncManager::SyncInternal public sync_notifier::SyncNotifierObserver, public browser_sync::ChannelEventHandler<syncable::DirectoryChangeEvent>, public browser_sync::JsBackend, - public SyncEngineEventListener { + public SyncEngineEventListener, + public ServerConnectionEventListener { static const int kDefaultNudgeDelayMilliseconds; static const int kPreferencesNudgeDelayMilliseconds; public: @@ -1296,6 +1299,9 @@ class SyncManager::SyncInternal // SyncEngineEventListener implementation. virtual void OnSyncEngineEvent(const SyncEngineEvent& event); + // ServerConnectionEventListener implementation. + virtual void OnServerConnectionEvent(const ServerConnectionEvent2& event); + // browser_sync::JsBackend implementation. virtual void SetParentJsEventRouter(browser_sync::JsEventRouter* router); virtual void RemoveParentJsEventRouter(); @@ -1597,11 +1603,18 @@ void SyncManager::RequestConfig(const syncable::ModelTypeBitSet& types) { if (!data_->syncer_thread()) return; // It is an error for this to be called if new_impl is null. - data_->syncer_thread()->new_impl()->Start( - browser_sync::s3::SyncerThread::CONFIGURATION_MODE); + StartConfigurationMode(NULL); data_->syncer_thread()->new_impl()->ScheduleConfig(types); } +void SyncManager::StartConfigurationMode(ModeChangeCallback* callback) { + if (!data_->syncer_thread()) + return; + // It is an error for this to be called if new_impl is null. + data_->syncer_thread()->new_impl()->Start( + browser_sync::s3::SyncerThread::CONFIGURATION_MODE, callback); +} + const std::string& SyncManager::GetAuthenticatedUsername() { DCHECK(data_); return data_->username_for_share(); @@ -1635,11 +1648,19 @@ bool SyncManager::SyncInternal::Init( connection_manager_.reset(new SyncAPIServerConnectionManager( sync_server_and_path, port, use_ssl, user_agent, post_factory)); - connection_manager_hookup_.reset( - NewEventListenerHookup(connection_manager()->channel(), this, - &SyncManager::SyncInternal::HandleServerConnectionEvent)); - net::NetworkChangeNotifier::AddIPAddressObserver(this); + + bool new_syncer_thread = CommandLine::ForCurrentProcess()->HasSwitch( + switches::kNewSyncerThread); + + if (new_syncer_thread) { + connection_manager()->AddListener(this); + } else { + connection_manager_hookup_.reset( + NewEventListenerHookup(connection_manager()->channel(), this, + &SyncManager::SyncInternal::HandleServerConnectionEvent)); + } + // TODO(akalin): CheckServerReachable() can block, which may cause jank if we // try to shut down sync. Fix this. core_message_loop_->PostTask(FROM_HERE, @@ -1660,8 +1681,7 @@ bool SyncManager::SyncInternal::Init( context->set_account_name(credentials.email); // The SyncerThread takes ownership of |context|. syncer_thread_.reset(new SyncerThreadAdapter(context, - CommandLine::ForCurrentProcess()->HasSwitch( - switches::kNewSyncerThread))); + new_syncer_thread)); } bool signed_in = SignIn(credentials); @@ -2161,6 +2181,15 @@ void SyncManager::SyncInternal::HandleTransactionCompleteChangeEvent( } } +void SyncManager::SyncInternal::OnServerConnectionEvent( + const ServerConnectionEvent2& event) { + ServerConnectionEvent legacy; + legacy.what_happened = ServerConnectionEvent::STATUS_CHANGED; + legacy.connection_code = event.connection_code; + legacy.server_reachable = event.server_reachable; + HandleServerConnectionEvent(legacy); +} + void SyncManager::SyncInternal::HandleServerConnectionEvent( const ServerConnectionEvent& event) { allstatus_.HandleServerConnectionEvent(event); diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h index badc84b..14a6749 100644 --- a/chrome/browser/sync/engine/syncapi.h +++ b/chrome/browser/sync/engine/syncapi.h @@ -43,6 +43,7 @@ #include <vector> #include "base/basictypes.h" +#include "base/callback.h" #include "base/gtest_prod_util.h" #include "base/memory/scoped_ptr.h" #include "build/build_config.h" @@ -828,6 +829,8 @@ class SyncManager { virtual ~Observer(); }; + typedef Callback0::Type ModeChangeCallback; + // Create an uninitialized SyncManager. Callers must Init() before using. SyncManager(); virtual ~SyncManager(); @@ -925,6 +928,12 @@ class SyncManager { // TODO(tim): Deprecated. bool RequestResume(); + // Puts the SyncerThread into a mode where no normal nudge or poll traffic + // will occur, but calls to RequestConfig will be supported. If |callback| + // is provided, it will be invoked (from the internal SyncerThread) when + // the thread has changed to configuration mode. + void StartConfigurationMode(ModeChangeCallback* callback); + // For the new SyncerThread impl, this switches the mode of operation to // CONFIGURATION_MODE and schedules a config task to fetch updates for // |types|. It is an error to call this with legacy SyncerThread in use. diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc index ac20600..2f24ee1 100644 --- a/chrome/browser/sync/engine/syncer.cc +++ b/chrome/browser/sync/engine/syncer.cc @@ -271,9 +271,6 @@ void Syncer::SyncShare(sessions::SyncSession* session, break; } case SYNCER_END: { - VLOG(1) << "Syncer End"; - SyncerEndCommand syncer_end_command; - syncer_end_command.Execute(session); break; } default: @@ -284,11 +281,9 @@ void Syncer::SyncShare(sessions::SyncSession* session, current_step = next_step; } - // Always send out a cycle ended notification, regardless of end-state. - SyncEngineEvent event(SyncEngineEvent::SYNC_CYCLE_ENDED); - sessions::SyncSessionSnapshot snapshot(session->TakeSnapshot()); - event.snapshot = &snapshot; - session->context()->NotifyListeners(event); + VLOG(1) << "Syncer End"; + SyncerEndCommand syncer_end_command; + syncer_end_command.Execute(session); return; } diff --git a/chrome/browser/sync/engine/syncer_end_command.cc b/chrome/browser/sync/engine/syncer_end_command.cc index dcdfcc1..8f67841 100644 --- a/chrome/browser/sync/engine/syncer_end_command.cc +++ b/chrome/browser/sync/engine/syncer_end_command.cc @@ -15,10 +15,12 @@ SyncerEndCommand::SyncerEndCommand() {} SyncerEndCommand::~SyncerEndCommand() {} void SyncerEndCommand::ExecuteImpl(sessions::SyncSession* session) { - sessions::StatusController* status(session->status_controller()); - status->set_syncing(false); - session->context()->set_previous_session_routing_info( - session->routing_info()); + // Always send out a cycle ended notification, regardless of end-state. + session->status_controller()->set_syncing(false); + SyncEngineEvent event(SyncEngineEvent::SYNC_CYCLE_ENDED); + sessions::SyncSessionSnapshot snapshot(session->TakeSnapshot()); + event.snapshot = &snapshot; + session->context()->NotifyListeners(event); session->context()->set_last_snapshot(session->TakeSnapshot()); } diff --git a/chrome/browser/sync/engine/syncer_thread2.cc b/chrome/browser/sync/engine/syncer_thread2.cc index 220f5d4..639f518 100644 --- a/chrome/browser/sync/engine/syncer_thread2.cc +++ b/chrome/browser/sync/engine/syncer_thread2.cc @@ -78,22 +78,63 @@ SyncerThread::~SyncerThread() { DCHECK(!thread_.IsRunning()); } -void SyncerThread::Start(Mode mode) { - if (!thread_.IsRunning() && !thread_.Start()) { - NOTREACHED() << "Unable to start SyncerThread."; - return; +void SyncerThread::CheckServerConnectionManagerStatus( + HttpResponse::ServerConnectionCode code) { + // Note, be careful when adding cases here because if the SyncerThread + // thinks there is no valid connection as determined by this method, it + // will drop out of *all* forward progress sync loops (it won't poll and it + // will queue up Talk notifications but not actually call SyncShare) until + // some external action causes a ServerConnectionManager to broadcast that + // a valid connection has been re-established. + if (HttpResponse::CONNECTION_UNAVAILABLE == code || + HttpResponse::SYNC_AUTH_ERROR == code) { + server_connection_ok_ = false; + } else if (HttpResponse::SERVER_CONNECTION_OK == code) { + server_connection_ok_ = true; + } +} + +void SyncerThread::Start(Mode mode, ModeChangeCallback* callback) { + if (!thread_.IsRunning()) { + if (!thread_.Start()) { + NOTREACHED() << "Unable to start SyncerThread."; + return; + } + WatchConnectionManager(); + thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod( + this, &SyncerThread::SendInitialSnapshot)); } thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod( - this, &SyncerThread::StartImpl, mode)); + this, &SyncerThread::StartImpl, mode, make_linked_ptr(callback))); +} + +void SyncerThread::SendInitialSnapshot() { + DCHECK_EQ(MessageLoop::current(), thread_.message_loop()); + scoped_ptr<SyncSession> dummy(new SyncSession(session_context_.get(), this, + SyncSourceInfo(), ModelSafeRoutingInfo(), + std::vector<ModelSafeWorker*>())); + SyncEngineEvent event(SyncEngineEvent::STATUS_CHANGED); + sessions::SyncSessionSnapshot snapshot(dummy->TakeSnapshot()); + event.snapshot = &snapshot; + session_context_->NotifyListeners(event); +} + +void SyncerThread::WatchConnectionManager() { + ServerConnectionManager* scm = session_context_->connection_manager(); + CheckServerConnectionManagerStatus(scm->server_status()); + scm->AddListener(this); } -void SyncerThread::StartImpl(Mode mode) { +void SyncerThread::StartImpl(Mode mode, + linked_ptr<ModeChangeCallback> callback) { DCHECK_EQ(MessageLoop::current(), thread_.message_loop()); DCHECK(!session_context_->account_name().empty()); DCHECK(syncer_.get()); mode_ = mode; AdjustPolling(NULL); // Will kick start poll timer if needed. + if (callback.get()) + callback->Run(); } bool SyncerThread::ShouldRunJob(SyncSessionJobPurpose purpose, @@ -221,6 +262,11 @@ void SyncerThread::ScheduleNudgeImpl(const TimeDelta& delay, NudgeSource source, const ModelTypePayloadMap& types_with_payloads) { DCHECK_EQ(MessageLoop::current(), thread_.message_loop()); TimeTicks rough_start = TimeTicks::Now() + delay; + if (!ShouldRunJob(NUDGE, rough_start)) { + LOG(WARNING) << "Dropping nudge at scheduling time, source = " + << source; + return; + } // Note we currently nudge for all types regardless of the ones incurring // the nudge. Doing different would throw off some syncer commands like @@ -349,6 +395,11 @@ void SyncerThread::SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose, void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) { DCHECK_EQ(MessageLoop::current(), thread_.message_loop()); + if (!ShouldRunJob(job.purpose, job.scheduled_start)) { + LOG(WARNING) << "Dropping nudge at DoSyncSessionJob, source = " + << job.session->source().updates_source; + return; + } if (job.purpose == NUDGE) { DCHECK(pending_nudge_.get()); @@ -362,10 +413,8 @@ void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) { SetSyncerStepsForPurpose(job.purpose, &begin, &end); bool has_more_to_sync = true; - bool did_job = false; while (ShouldRunJob(job.purpose, job.scheduled_start) && has_more_to_sync) { VLOG(1) << "SyncerThread: Calling SyncShare."; - did_job = true; // Synchronously perform the sync session from this thread. syncer_->SyncShare(job.session.get(), begin, end); has_more_to_sync = job.session->HasMoreToSync(); @@ -373,8 +422,26 @@ void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) { job.session->ResetTransientState(); } VLOG(1) << "SyncerThread: Done SyncShare looping."; - if (did_job) - FinishSyncSessionJob(job); + FinishSyncSessionJob(job); +} + +void SyncerThread::UpdateCarryoverSessionState(const SyncSessionJob& old_job) { + if (old_job.purpose == CONFIGURATION) { + // Whatever types were part of a configuration task will have had updates + // downloaded. For that reason, we make sure they get recorded in the + // event that they get disabled at a later time. + ModelSafeRoutingInfo r(session_context_->previous_session_routing_info()); + if (!r.empty()) { + ModelSafeRoutingInfo temp_r; + ModelSafeRoutingInfo old_info(old_job.session->routing_info()); + std::set_union(r.begin(), r.end(), old_info.begin(), old_info.end(), + std::insert_iterator<ModelSafeRoutingInfo>(temp_r, temp_r.begin())); + session_context_->set_previous_session_routing_info(temp_r); + } + } else { + session_context_->set_previous_session_routing_info( + old_job.session->routing_info()); + } } void SyncerThread::FinishSyncSessionJob(const SyncSessionJob& job) { @@ -391,6 +458,7 @@ void SyncerThread::FinishSyncSessionJob(const SyncSessionJob& job) { } } last_sync_session_end_time_ = now; + UpdateCarryoverSessionState(job); if (IsSyncingCurrentlySilenced()) return; // Nothing to do. @@ -506,8 +574,8 @@ TimeDelta SyncerThread::GetRecommendedDelay(const TimeDelta& last_delay) { void SyncerThread::Stop() { syncer_->RequestEarlyExit(); // Safe to call from any thread. + session_context_->connection_manager()->RemoveListener(this); thread_.Stop(); - Notify(SyncEngineEvent::SYNCER_THREAD_EXITING); } void SyncerThread::DoCanaryJob() { @@ -577,8 +645,10 @@ void SyncerThread::OnShouldStopSyncingPermanently() { } void SyncerThread::OnServerConnectionEvent( - const ServerConnectionEvent& event) { - NOTIMPLEMENTED(); + const ServerConnectionEvent2& event) { + thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this, + &SyncerThread::CheckServerConnectionManagerStatus, + event.connection_code)); } void SyncerThread::set_notifications_enabled(bool notifications_enabled) { diff --git a/chrome/browser/sync/engine/syncer_thread2.h b/chrome/browser/sync/engine/syncer_thread2.h index 363a1c9..3768456 100644 --- a/chrome/browser/sync/engine/syncer_thread2.h +++ b/chrome/browser/sync/engine/syncer_thread2.h @@ -7,6 +7,7 @@ #define CHROME_BROWSER_SYNC_ENGINE_SYNCER_THREAD2_H_ #pragma once +#include "base/callback.h" #include "base/memory/linked_ptr.h" #include "base/memory/scoped_ptr.h" #include "base/observer_list.h" @@ -18,6 +19,7 @@ #include "chrome/browser/sync/engine/polling_constants.h" #include "chrome/browser/sync/engine/syncer.h" #include "chrome/browser/sync/syncable/model_type_payload_map.h" +#include "chrome/browser/sync/engine/net/server_connection_manager.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/sessions/sync_session_context.h" @@ -27,7 +29,8 @@ struct ServerConnectionEvent; namespace s3 { -class SyncerThread : public sessions::SyncSession::Delegate { +class SyncerThread : public sessions::SyncSession::Delegate, + public ServerConnectionEventListener { public: enum Mode { // In this mode, the thread only performs configuration tasks. This is @@ -44,6 +47,8 @@ class SyncerThread : public sessions::SyncSession::Delegate { SyncerThread(sessions::SyncSessionContext* context, Syncer* syncer); virtual ~SyncerThread(); + typedef Callback0::Type ModeChangeCallback; + // Change the mode of operation. // We don't use a lock when changing modes, so we won't cause currently // scheduled jobs to adhere to the new mode. We could protect it, but it @@ -52,7 +57,10 @@ class SyncerThread : public sessions::SyncSession::Delegate { // all their required state and won't be affected by potential change at // higher levels (i.e. the registrar), and c) we service tasks FIFO, so once // the mode changes all future jobs will be run against the updated mode. - void Start(Mode mode); + // If supplied, |callback| will be invoked when the mode has been + // changed to |mode| *from the SyncerThread*, and not from the caller + // thread. + void Start(Mode mode, ModeChangeCallback* callback); // Joins on the thread as soon as possible (currently running session // completes). @@ -84,6 +92,10 @@ class SyncerThread : public sessions::SyncSession::Delegate { const base::TimeDelta& new_interval); virtual void OnShouldStopSyncingPermanently(); + // ServerConnectionEventListener implementation. + // TODO(tim): schedule a nudge when valid connection detected? in 1 minute? + virtual void OnServerConnectionEvent(const ServerConnectionEvent2& event); + private: friend class SyncerThread2Test; @@ -133,6 +145,10 @@ class SyncerThread : public sessions::SyncSession::Delegate { // reset our state. void FinishSyncSessionJob(const SyncSessionJob& job); + // Record important state that might be needed in future syncs, such as which + // data types may require cleanup. + void UpdateCarryoverSessionState(const SyncSessionJob& old_job); + // Helper to FinishSyncSessionJob to schedule the next sync operation. void ScheduleNextSync(const SyncSessionJob& old_job); @@ -150,7 +166,7 @@ class SyncerThread : public sessions::SyncSession::Delegate { // 'Impl' here refers to real implementation of public functions, running on // |thread_|. - void StartImpl(Mode mode); + void StartImpl(Mode mode, linked_ptr<ModeChangeCallback> callback); void ScheduleNudgeImpl( const base::TimeDelta& delay, NudgeSource source, @@ -165,10 +181,6 @@ class SyncerThread : public sessions::SyncSession::Delegate { // Helper to signal all listeners registered with |session_context_|. void Notify(SyncEngineEvent::EventCause cause); - // ServerConnectionEventListener implementation. - // TODO(tim): schedule a nudge when valid connection detected? in 1 minute? - virtual void OnServerConnectionEvent(const ServerConnectionEvent& event); - // Callback to change backoff state. void DoCanaryJob(); void Unthrottle(); @@ -182,6 +194,18 @@ class SyncerThread : public sessions::SyncSession::Delegate { SyncerStep* start, SyncerStep* end); + // Initializes the hookup between the ServerConnectionManager and us. + void WatchConnectionManager(); + + // Used to update |server_connection_ok_|, see below. + void CheckServerConnectionManagerStatus( + HttpResponse::ServerConnectionCode code); + + // Called once the first time thread_ is started to broadcast an initial + // session snapshot containing data like initial_sync_ended. Important when + // the client starts up and does not need to perform an initial sync. + void SendInitialSnapshot(); + base::Thread thread_; // Modifiable versions of kDefaultLongPollIntervalSeconds which can be diff --git a/chrome/browser/sync/engine/syncer_thread2_unittest.cc b/chrome/browser/sync/engine/syncer_thread2_unittest.cc index 70d2f73..ac03cfb 100644 --- a/chrome/browser/sync/engine/syncer_thread2_unittest.cc +++ b/chrome/browser/sync/engine/syncer_thread2_unittest.cc @@ -8,6 +8,7 @@ #include "chrome/browser/sync/engine/syncer.h" #include "chrome/browser/sync/engine/syncer_thread2.h" #include "chrome/browser/sync/sessions/test_util.h" +#include "chrome/test/sync/engine/mock_connection_manager.h" #include "chrome/test/sync/engine/test_directory_setter_upper.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/gmock/include/gmock/gmock.h" @@ -59,18 +60,19 @@ class SyncerThread2Test : public testing::Test { syncer_ = new MockSyncer(); delay_ = NULL; registrar_.reset(MockModelSafeWorkerRegistrar::PassiveBookmarks()); - context_ = new SyncSessionContext(NULL, syncdb_.manager(), + connection_.reset(new MockConnectionManager(syncdb_.manager(), "Test")); + connection_->SetServerReachable(); + context_ = new SyncSessionContext(connection_.get(), syncdb_.manager(), registrar_.get(), std::vector<SyncEngineEventListener*>()); context_->set_notifications_enabled(true); context_->set_account_name("Test"); syncer_thread_.reset(new SyncerThread(context_, syncer_)); - // TODO(tim): Once the SCM is hooked up, remove this. - syncer_thread_->server_connection_ok_ = true; } SyncerThread* syncer_thread() { return syncer_thread_.get(); } MockSyncer* syncer() { return syncer_; } MockDelayProvider* delay() { return delay_; } + MockConnectionManager* connection() { return connection_.get(); } TimeDelta zero() { return TimeDelta::FromSeconds(0); } TimeDelta timeout() { return TimeDelta::FromMilliseconds(TestTimeouts::action_timeout_ms()); @@ -96,7 +98,7 @@ class SyncerThread2Test : public testing::Test { bool GetBackoffAndResetTest(base::WaitableEvent* done) { syncable::ModelTypeBitSet nudge_types; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, nudge_types); done->TimedWait(timeout()); TearDown(); @@ -130,6 +132,10 @@ class SyncerThread2Test : public testing::Test { event->Signal(); } + static void QuitMessageLoop() { + MessageLoop::current()->Quit(); + } + // Compare a ModelTypeBitSet to a ModelTypePayloadMap, ignoring // payload values. bool CompareModelTypeBitSetToModelTypePayloadMap( @@ -146,8 +152,11 @@ class SyncerThread2Test : public testing::Test { return true; } + SyncSessionContext* context() { return context_; } + private: scoped_ptr<SyncerThread> syncer_thread_; + scoped_ptr<MockConnectionManager> connection_; SyncSessionContext* context_; MockSyncer* syncer_; MockDelayProvider* delay_; @@ -179,7 +188,7 @@ ACTION_P(SignalEvent, event) { // Test nudge scheduling. TEST_F(SyncerThread2Test, Nudge) { - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); base::WaitableEvent done(false, false); SyncShareRecords records; syncable::ModelTypeBitSet model_types; @@ -217,7 +226,7 @@ TEST_F(SyncerThread2Test, Nudge) { // Test that nudges are coalesced. TEST_F(SyncerThread2Test, NudgeCoalescing) { - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); base::WaitableEvent done(false, false); SyncShareRecords r; EXPECT_CALL(*syncer(), SyncShare(_,_,_)) @@ -257,7 +266,7 @@ TEST_F(SyncerThread2Test, NudgeCoalescing) { // Test nudge scheduling. TEST_F(SyncerThread2Test, NudgeWithPayloads) { - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); base::WaitableEvent done(false, false); SyncShareRecords records; syncable::ModelTypePayloadMap model_types_with_payloads; @@ -295,7 +304,7 @@ TEST_F(SyncerThread2Test, NudgeWithPayloads) { // Test that nudges are coalesced. TEST_F(SyncerThread2Test, NudgeWithPayloadsCoalescing) { - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); base::WaitableEvent done(false, false); SyncShareRecords r; EXPECT_CALL(*syncer(), SyncShare(_,_,_)) @@ -349,7 +358,7 @@ TEST_F(SyncerThread2Test, Polling) { WithArg<0>(RecordSyncShare(&records, kMinNumSamples, &done)))); TimeTicks optimal_start = TimeTicks::Now() + poll_interval; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); @@ -368,7 +377,7 @@ TEST_F(SyncerThread2Test, PollNotificationsDisabled) { WithArg<0>(RecordSyncShare(&records, kMinNumSamples, &done)))); TimeTicks optimal_start = TimeTicks::Now() + poll_interval; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); @@ -389,7 +398,7 @@ TEST_F(SyncerThread2Test, PollIntervalUpdate) { WithArg<0>(RecordSyncShare(&records, kMinNumSamples, &done)))); TimeTicks optimal_start = TimeTicks::Now() + poll1 + poll2; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); @@ -398,7 +407,7 @@ TEST_F(SyncerThread2Test, PollIntervalUpdate) { // Test that a sync session is run through to completion. TEST_F(SyncerThread2Test, HasMoreToSync) { - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); base::WaitableEvent done(false, false); EXPECT_CALL(*syncer(), SyncShare(_,_,_)) .WillOnce(Invoke(sessions::test_util::SimulateHasMoreToSync)) @@ -421,11 +430,11 @@ TEST_F(SyncerThread2Test, ThrottlingDoesThrottle) { EXPECT_CALL(*syncer(), SyncShare(_,_,_)) .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle))); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, types); FlushLastTask(&done); - syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE); + syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL); syncer_thread()->ScheduleConfig(types); FlushLastTask(&done); } @@ -447,7 +456,7 @@ TEST_F(SyncerThread2Test, ThrottlingExpires) { WithArg<0>(RecordSyncShare(&records, kMinNumSamples, &done)))); TimeTicks optimal_start = TimeTicks::Now() + poll + throttle1; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); @@ -464,7 +473,7 @@ TEST_F(SyncerThread2Test, ConfigurationMode) { EXPECT_CALL(*syncer(), SyncShare(_,_,_)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), WithArg<0>(RecordSyncShare(&records, 1U, dummy)))); - syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE); + syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL); syncable::ModelTypeBitSet nudge_types; nudge_types[syncable::AUTOFILL] = true; syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, nudge_types); @@ -540,7 +549,7 @@ TEST_F(SyncerThread2Test, BackoffDropsJobs) { EXPECT_CALL(*delay(), GetDelay(_)) .WillRepeatedly(Return(TimeDelta::FromDays(1))); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); ASSERT_TRUE(done.TimedWait(timeout())); done.Reset(); @@ -570,11 +579,11 @@ TEST_F(SyncerThread2Test, BackoffDropsJobs) { EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(0); EXPECT_CALL(*delay(), GetDelay(_)).Times(0); - syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE); + syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL); syncer_thread()->ScheduleConfig(types); FlushLastTask(&done); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, types); syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, types); FlushLastTask(&done); @@ -606,7 +615,7 @@ TEST_F(SyncerThread2Test, BackoffElevation) { .RetiresOnSaturation(); EXPECT_CALL(*delay(), GetDelay(Eq(fourth))).WillOnce(Return(fifth)); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); ASSERT_TRUE(done.TimedWait(timeout())); EXPECT_GE(r.times[2] - r.times[1], second); @@ -634,7 +643,7 @@ TEST_F(SyncerThread2Test, BackoffRelief) { // Optimal start for the post-backoff poll party. TimeTicks optimal_start = TimeTicks::Now() + poll + backoff; - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); @@ -673,7 +682,7 @@ TEST_F(SyncerThread2Test, SyncerSteps) { base::WaitableEvent done(false, false); EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END)) .Times(1); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet()); FlushLastTask(&done); syncer_thread()->Stop(); @@ -682,15 +691,14 @@ TEST_F(SyncerThread2Test, SyncerSteps) { // ClearUserData. EXPECT_CALL(*syncer(), SyncShare(_, CLEAR_PRIVATE_DATA, SYNCER_END)) .Times(1); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); syncer_thread()->ScheduleClearUserData(); FlushLastTask(&done); syncer_thread()->Stop(); Mock::VerifyAndClearExpectations(syncer()); - // Configuration. EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES)); - syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE); + syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL); syncer_thread()->ScheduleConfig(ModelTypeBitSet()); FlushLastTask(&done); syncer_thread()->Stop(); @@ -702,7 +710,7 @@ TEST_F(SyncerThread2Test, SyncerSteps) { .WillRepeatedly(SignalEvent(&done)); const TimeDelta poll(TimeDelta::FromMilliseconds(10)); syncer_thread()->OnReceivedLongPollIntervalUpdate(poll); - syncer_thread()->Start(SyncerThread::NORMAL_MODE); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); done.TimedWait(timeout()); syncer_thread()->Stop(); Mock::VerifyAndClearExpectations(syncer()); @@ -716,10 +724,42 @@ TEST_F(SyncerThread2Test, DISABLED_NoConfigDuringNormal) { // Test that starting the syncer thread without a valid connection doesn't // break things when a connection is detected. -// Test config tasks don't run during normal mode. -// TODO(tim): Implement this test and then the functionality! -TEST_F(SyncerThread2Test, DISABLED_StartWhenNotConnected) { +TEST_F(SyncerThread2Test, StartWhenNotConnected) { + base::WaitableEvent done(false, false); + MessageLoop cur; + connection()->SetServerNotReachable(); + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); + syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet()); + FlushLastTask(&done); + connection()->SetServerReachable(); + cur.PostTask(FROM_HERE, NewRunnableFunction( + &SyncerThread2Test::QuitMessageLoop)); + cur.Run(); + + // By now, the server connection event should have been posted to the + // SyncerThread. + FlushLastTask(&done); + EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done)); + syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet()); + done.TimedWait(timeout()); +} + +TEST_F(SyncerThread2Test, SetsPreviousRoutingInfo) { + base::WaitableEvent done(false, false); + ModelSafeRoutingInfo info; + EXPECT_TRUE(info == context()->previous_session_routing_info()); + ModelSafeRoutingInfo expected; + context()->registrar()->GetModelSafeRoutingInfo(&expected); + ASSERT_FALSE(expected.empty()); + EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1); + + syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); + syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet()); + FlushLastTask(&done); + syncer_thread()->Stop(); + + EXPECT_TRUE(expected == context()->previous_session_routing_info()); } } // namespace s3 @@ -727,4 +767,3 @@ TEST_F(SyncerThread2Test, DISABLED_StartWhenNotConnected) { // SyncerThread won't outlive the test! DISABLE_RUNNABLE_METHOD_REFCOUNT(browser_sync::s3::SyncerThread2Test); - diff --git a/chrome/browser/sync/engine/syncer_thread_adapter.cc b/chrome/browser/sync/engine/syncer_thread_adapter.cc index 174e49a..04d3862 100644 --- a/chrome/browser/sync/engine/syncer_thread_adapter.cc +++ b/chrome/browser/sync/engine/syncer_thread_adapter.cc @@ -16,7 +16,7 @@ SyncerThreadAdapter::SyncerThreadAdapter(sessions::SyncSessionContext* context, : legacy_(NULL), new_impl_(NULL), using_new_impl_(using_new_impl) { if (using_new_impl_) { new_impl_.reset(new s3::SyncerThread(context, new Syncer())); - new_impl_->Start(s3::SyncerThread::CONFIGURATION_MODE); + new_impl_->Start(s3::SyncerThread::CONFIGURATION_MODE, NULL); } else { legacy_ = new SyncerThread(context); } @@ -35,7 +35,7 @@ void SyncerThreadAdapter::WatchConnectionManager( bool SyncerThreadAdapter::Start() { if (using_new_impl_) { - new_impl_->Start(s3::SyncerThread::NORMAL_MODE); + new_impl_->Start(s3::SyncerThread::NORMAL_MODE, NULL); return true; } else { return legacy_->Start(); diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h index 0e05e0e..50f079f 100644 --- a/chrome/browser/sync/engine/syncer_types.h +++ b/chrome/browser/sync/engine/syncer_types.h @@ -97,13 +97,16 @@ struct SyncEngineEvent { // This event is sent when the thread is waiting for a connection // to be established. + // TODO(tim): Deprecated. SYNCER_THREAD_WAITING_FOR_CONNECTION, // This event is sent when a connection has been established and // the thread continues. + // TODO(tim): Deprecated. SYNCER_THREAD_CONNECTED, // Sent when the main syncer loop finishes. + // TODO(tim): Deprecated. SYNCER_THREAD_EXITING, //////////////////////////////////////////////////////////////// diff --git a/chrome/browser/sync/engine/update_applicator.cc b/chrome/browser/sync/engine/update_applicator.cc index 6922c0f..04eca43 100644 --- a/chrome/browser/sync/engine/update_applicator.cc +++ b/chrome/browser/sync/engine/update_applicator.cc @@ -91,10 +91,21 @@ void UpdateApplicator::Advance() { } bool UpdateApplicator::SkipUpdate(const syncable::Entry& entry) { - ModelSafeGroup g = - GetGroupForModelType(entry.GetServerModelType(), routing_info_); + syncable::ModelType type = entry.GetServerModelType(); + ModelSafeGroup g = GetGroupForModelType(type, routing_info_); + // The extra routing_info count check here is to support GetUpdateses for + // a subset of the globally enabled types, and not attempt to update items + // if their type isn't permitted in the current run. These would typically + // be unapplied items from a previous sync. if (g != group_filter_) return true; + if (g == GROUP_PASSIVE && + !routing_info_.count(type) && + type != syncable::UNSPECIFIED && + type != syncable::TOP_LEVEL_FOLDER) { + VLOG(1) << "Skipping update application, type not permitted."; + return true; + } return false; } diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 9f5ad2d..08a771e 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -345,20 +345,24 @@ void SyncBackendHost::ConfigureAutofillMigration() { } } +SyncBackendHost::PendingConfigureDataTypesState:: +PendingConfigureDataTypesState() : deleted_type(false) {} + void SyncBackendHost::ConfigureDataTypes( const DataTypeController::TypeMap& data_type_controllers, const syncable::ModelTypeSet& types, CancelableTask* ready_task) { // Only one configure is allowed at a time. - DCHECK(!configure_ready_task_.get()); + DCHECK(!pending_config_mode_state_.get()); + DCHECK(!pending_download_state_.get()); DCHECK(syncapi_initialized_); if (types.count(syncable::AUTOFILL_PROFILE) != 0) { ConfigureAutofillMigration(); } - bool deleted_type = false; - syncable::ModelTypeBitSet added_types; + scoped_ptr<PendingConfigureDataTypesState> state(new + PendingConfigureDataTypesState()); { base::AutoLock lock(registrar_lock_); @@ -370,32 +374,34 @@ void SyncBackendHost::ConfigureDataTypes( // If a type is not specified, remove it from the routing_info. if (types.count(type) == 0) { registrar_.routing_info.erase(type); - deleted_type = true; + state->deleted_type = true; } else { // Add a newly specified data type as GROUP_PASSIVE into the // routing_info, if it does not already exist. if (registrar_.routing_info.count(type) == 0) { registrar_.routing_info[type] = GROUP_PASSIVE; - added_types.set(type); + state->added_types.set(type); } } } } - // If no new data types were added to the passive group, no need to - // wait for the syncer. - if (core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) { - ready_task->Run(); - delete ready_task; + state->ready_task.reset(ready_task); + state->initial_types = types; + pending_config_mode_state_.reset(state.release()); + + // If we're doing the first configure (at startup) this is redundant as the + // syncer thread always must start in config mode. + if (using_new_syncer_thread_) { + core_->syncapi()->StartConfigurationMode(NewCallback(core_.get(), + &SyncBackendHost::Core::FinishConfigureDataTypes)); } else { - // Save the task here so we can run it when the syncer finishes - // initializing the new data types. It will be run only when the - // set of initially synced data types matches the types requested in - // this configure. - configure_ready_task_.reset(ready_task); - configure_initial_sync_types_ = types; + FinishConfigureDataTypesOnFrontendLoop(); } +} +void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() { + DCHECK_EQ(MessageLoop::current(), frontend_loop_); // Nudge the syncer. This is necessary for both datatype addition/deletion. // // Deletions need a nudge in order to ensure the deletion occurs in a timely @@ -404,9 +410,45 @@ void SyncBackendHost::ConfigureDataTypes( // In the case of additions, on the next sync cycle, the syncer should // notice that the routing info has changed and start the process of // downloading updates for newly added data types. Once this is - // complete, the configure_ready_task_ is run via an + // complete, the configure_state_.ready_task_ is run via an // OnInitializationComplete notification. - ScheduleSyncEventForConfigChange(deleted_type, added_types); + bool request_nudge = false; + if (pending_config_mode_state_->deleted_type) { + if (using_new_syncer_thread_) { + core_thread_.message_loop()->PostTask(FROM_HERE, + NewRunnableMethod(core_.get(), + &SyncBackendHost::Core::DeferNudgeForCleanup)); + } else { + request_nudge = true; + } + } + + if (core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) { + pending_config_mode_state_->ready_task->Run(); + } else { + if (!pending_config_mode_state_->added_types.any()) { + LOG(WARNING) << "No new types, but initial sync not finished." + << "Possible sync db corruption / removal."; + // TODO(tim): Log / UMA / count this somehow? + // TODO(tim): If no added types, we could (should?) config only for + // types that are needed... but this is a rare corruption edge case or + // implies the user mucked around with their syncdb, so for now do all. + pending_config_mode_state_->added_types = + syncable::ModelTypeBitSetFromSet( + pending_config_mode_state_->initial_types); + } + pending_download_state_.reset(pending_config_mode_state_.release()); + if (using_new_syncer_thread_) { + RequestConfig(pending_download_state_->added_types); + } else { + request_nudge = true; + } + } + + if (request_nudge) + RequestNudge(); + + pending_config_mode_state_.reset(); // Notify the SyncManager about the new types. core_thread_.message_loop()->PostTask(FROM_HERE, @@ -414,26 +456,6 @@ void SyncBackendHost::ConfigureDataTypes( &SyncBackendHost::Core::DoUpdateEnabledTypes)); } -void SyncBackendHost::ScheduleSyncEventForConfigChange(bool deleted_type, - const syncable::ModelTypeBitSet& added_types) { - // We can only nudge when we've either deleted a dataype or added one, else - // we can cause unnecessary syncs. Unit tests cover this. - if (using_new_syncer_thread_) { - if (added_types.size() > 0) - RequestConfig(added_types); - - // TODO(tim): Bug 76233. Fix this once only one impl exists. - if (deleted_type) { - core_thread_.message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(core_.get(), - &SyncBackendHost::Core::DeferNudgeForCleanup)); - } - } else if (deleted_type || - !core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) { - RequestNudge(); - } -} - void SyncBackendHost::EncryptDataTypes( const syncable::ModelTypeSet& encrypted_types) { core_thread_.message_loop()->PostTask(FROM_HERE, @@ -450,7 +472,12 @@ void SyncBackendHost::RequestNudge() { void SyncBackendHost::RequestConfig( const syncable::ModelTypeBitSet& added_types) { DCHECK(core_->syncapi()); - core_->syncapi()->RequestConfig(added_types); + + syncable::ModelTypeBitSet types_copy(added_types); + if (IsNigoriEnabled()) + types_copy.set(syncable::NIGORI); + + core_->syncapi()->RequestConfig(types_copy); } void SyncBackendHost::ActivateDataType( @@ -579,6 +606,15 @@ void SyncBackendHost::Core::NotifyEncryptionComplete( host_->frontend_->OnEncryptionComplete(encrypted_types); } +void SyncBackendHost::Core::FinishConfigureDataTypes() { + host_->frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(this, + &SyncBackendHost::Core::FinishConfigureDataTypesOnFrontendLoop)); +} + +void SyncBackendHost::Core::FinishConfigureDataTypesOnFrontendLoop() { + host_->FinishConfigureDataTypesOnFrontendLoop(); +} + SyncBackendHost::Core::DoInitializeOptions::DoInitializeOptions( const GURL& service_url, sync_api::HttpPostProviderFactory* http_bridge_factory, @@ -865,18 +901,21 @@ void SyncBackendHost::Core::HandleSyncCycleCompletedOnFrontendLoop( // If we are waiting for a configuration change, check here to see // if this sync cycle has initialized all of the types we've been // waiting for. - if (host_->configure_ready_task_.get()) { - bool found_all = true; + if (host_->pending_download_state_.get()) { + bool found_all_added = true; for (syncable::ModelTypeSet::const_iterator it = - host_->configure_initial_sync_types_.begin(); - it != host_->configure_initial_sync_types_.end(); ++it) { - found_all &= snapshot->initial_sync_ended.test(*it); + host_->pending_download_state_->initial_types.begin(); + it != host_->pending_download_state_->initial_types.end(); + ++it) { + if (host_->pending_download_state_->added_types.test(*it)) + found_all_added &= snapshot->initial_sync_ended.test(*it); } - - if (found_all) { - host_->configure_ready_task_->Run(); - host_->configure_ready_task_.reset(); - host_->configure_initial_sync_types_.clear(); + if (!found_all_added) { + CHECK(false); + DCHECK(!host_->using_new_syncer_thread_); + } else { + host_->pending_download_state_->ready_task->Run(); + host_->pending_download_state_.reset(); } } host_->frontend_->OnSyncCycleCompleted(); diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 1349c60..b18fce9 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -388,6 +388,9 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { const std::string& name, const JsArgList& args, const JsEventHandler* sender); + // A callback from the SyncerThread when it is safe to continue config. + void FinishConfigureDataTypes(); + #if defined(UNIT_TEST) // 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 @@ -482,6 +485,8 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { const std::string& name, const JsArgList& args, const JsEventHandler* dst); + void FinishConfigureDataTypesOnFrontendLoop(); + // Return true if a model lives on the current thread. bool IsCurrentThreadSafeForModel(syncable::ModelType model_type); @@ -522,6 +527,11 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // Posts a config request on the core thread. virtual void RequestConfig(const syncable::ModelTypeBitSet& added_types); + // Called to finish the job of ConfigureDataTypes once the syncer is in + // configuration mode. + void FinishConfigureDataTypes(); + void FinishConfigureDataTypesOnFrontendLoop(); + // Allows tests to perform alternate core initialization work. virtual void InitCore(const Core::DoInitializeOptions& options); @@ -548,12 +558,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { void ConfigureAutofillMigration(); - // Depending on switches::kUseNewSyncerThread, kicks the syncapi to respond - // to a change in the set of enabled data types. - void ScheduleSyncEventForConfigChange( - bool deleted_type, - const syncable::ModelTypeBitSet& added_types); - // 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. @@ -600,13 +604,23 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // Path of the folder that stores the sync data files. FilePath sync_data_folder_path_; - // A task that should be called once data type configuration is - // complete. - scoped_ptr<CancelableTask> configure_ready_task_; + struct PendingConfigureDataTypesState { + PendingConfigureDataTypesState(); + // A task that should be called once data type configuration is + // complete. + scoped_ptr<CancelableTask> ready_task; + + // The set of types that we are waiting to be initially synced in a + // configuration cycle. + syncable::ModelTypeSet initial_types; + + // Additional details about which types were added / removed. + bool deleted_type; + syncable::ModelTypeBitSet added_types; + }; - // The set of types that we are waiting to be initially synced in a - // configuration cycle. - syncable::ModelTypeSet configure_initial_sync_types_; + scoped_ptr<PendingConfigureDataTypesState> pending_download_state_; + scoped_ptr<PendingConfigureDataTypesState> pending_config_mode_state_; // UI-thread cache of the last AuthErrorState received from syncapi. GoogleServiceAuthError last_auth_error_; diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 428fea0..584f113 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -274,13 +274,6 @@ class ProfileSyncService : public browser_sync::SyncFrontend, return passphrase_required_for_decryption_; } - // A timestamp marking the last time the service observed a transition from - // the SYNCING state to the READY state. Note that this does not reflect the - // last time we polled the server to see if there were any changes; the - // timestamp is only snapped when syncing takes place and we download or - // upload some bookmark entity. - const base::Time& last_synced_time() const { return last_synced_time_; } - // Returns a user-friendly string form of last synced time (in minutes). virtual string16 GetLastSyncedTimeString() const; diff --git a/chrome/test/sync/engine/mock_connection_manager.cc b/chrome/test/sync/engine/mock_connection_manager.cc index e4578fe..7d3c5fe 100644 --- a/chrome/test/sync/engine/mock_connection_manager.cc +++ b/chrome/test/sync/engine/mock_connection_manager.cc @@ -15,6 +15,8 @@ using browser_sync::HttpResponse; using browser_sync::ServerConnectionManager; +using browser_sync::ServerConnectionEventListener; +using browser_sync::ServerConnectionEvent2; using browser_sync::SyncerProtoUtil; using browser_sync::TestIdFactory; using std::map; @@ -608,7 +610,10 @@ void MockConnectionManager::SetServerReachable() { browser_sync::ServerConnectionEvent::STATUS_CHANGED, server_status_, server_reachable_ }; + channel_->NotifyListeners(event); + listeners_->Notify(&ServerConnectionEventListener::OnServerConnectionEvent, + ServerConnectionEvent2(server_status_, server_reachable_)); } void MockConnectionManager::SetServerNotReachable() { @@ -619,4 +624,6 @@ void MockConnectionManager::SetServerNotReachable() { server_status_, server_reachable_ }; channel_->NotifyListeners(event); + listeners_->Notify(&ServerConnectionEventListener::OnServerConnectionEvent, + ServerConnectionEvent2(server_status_, server_reachable_)); } |