handle store open or load failure correctly by making exiting early and also notifying the UI layer of the failure which then calls ShutDown.

BUG=93313
TEST=manually by simulating a store load failure.


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

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

3 files changed, 23 insertions, 18 deletions

diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index 3ee96aa..af552a8 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -813,22 +813,32 @@ bool SyncManager::SyncInternal::Init(
 
   bool signed_in = SignIn(credentials);
 
-  if (signed_in && scheduler()) {
-    scheduler()->Start(
-        browser_sync::SyncScheduler::CONFIGURATION_MODE, NULL);
-  }
+  if (signed_in || setup_for_test_mode_) {
+    if (scheduler()) {
+      scheduler()->Start(
+          browser_sync::SyncScheduler::CONFIGURATION_MODE, NULL);
+    }
 
-  initialized_ = true;
+    initialized_ = true;
 
-  // The following calls check that initialized_ is true.
-  BootstrapEncryption(restored_key_for_bootstrapping);
+    // The following calls check that initialized_ is true.
+    BootstrapEncryption(restored_key_for_bootstrapping);
+  }
 
-  // Notify that initialization is complete.
+  // Notify that initialization is complete. Note: This should be the last to
+  // execute if |signed_in| is false. Reason being in that case we would
+  // post a task to shutdown sync. But if this function posts any other tasks
+  // on the UI thread and if shutdown wins then that tasks would execute on
+  // a freed pointer. This is because UI thread is not shut down.
   ObserverList<SyncManager::Observer> temp_obs_list;
   CopyObservers(&temp_obs_list);
   FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
                     OnInitializationComplete(
-                        WeakHandle<JsBackend>(weak_ptr_factory_.GetWeakPtr())));
+                        WeakHandle<JsBackend>(weak_ptr_factory_.GetWeakPtr()),
+                        signed_in));
+
+  if (!signed_in && !setup_for_test_mode_)
+    return false;
 
   sync_notifier_->AddObserver(this);
 
@@ -894,11 +904,6 @@ bool SyncManager::SyncInternal::OpenDirectory() {
   bool share_opened = dir_manager()->Open(username_for_share(), this);
   DCHECK(share_opened);
   if (!share_opened) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                      OnStopSyncingPermanently());
-
     LOG(ERROR) << "Could not open share for:" << username_for_share();
     return false;
   }
diff --git a/chrome/browser/sync/internal_api/sync_manager.h b/chrome/browser/sync/internal_api/sync_manager.h
index fdd9bbd..a86fdc8 100644
--- a/chrome/browser/sync/internal_api/sync_manager.h
+++ b/chrome/browser/sync/internal_api/sync_manager.h
@@ -365,7 +365,7 @@ class SyncManager {
 
     virtual void OnInitializationComplete(
         const browser_sync::WeakHandle<browser_sync::JsBackend>&
-            js_backend) = 0;
+            js_backend, bool success) = 0;
 
     // We are no longer permitted to communicate with the server. Sync should
     // be disabled and state cleaned up at once.  This can happen for a number
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index d4ea301..abc56a9 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -696,8 +696,8 @@ class SyncManagerObserverMock : public SyncManager::Observer {
   MOCK_METHOD1(OnChangesComplete, void(ModelType));  // NOLINT
   MOCK_METHOD1(OnSyncCycleCompleted,
                void(const SyncSessionSnapshot*));  // NOLINT
-  MOCK_METHOD1(OnInitializationComplete,
-               void(const WeakHandle<JsBackend>&));  // NOLINT
+  MOCK_METHOD2(OnInitializationComplete,
+               void(const WeakHandle<JsBackend>&, bool));  // NOLINT
   MOCK_METHOD1(OnAuthError, void(const GoogleServiceAuthError&));  // NOLINT
   MOCK_METHOD1(OnPassphraseRequired,
                void(sync_api::PassphraseRequiredReason));  // NOLINT
@@ -762,7 +762,7 @@ class SyncManagerTest : public testing::Test,
         WillOnce(Invoke(this, &SyncManagerTest::SyncNotifierRemoveObserver));
 
     sync_manager_.AddObserver(&observer_);
-    EXPECT_CALL(observer_, OnInitializationComplete(_)).
+    EXPECT_CALL(observer_, OnInitializationComplete(_, _)).
         WillOnce(SaveArg<0>(&js_backend_));
 
     EXPECT_FALSE(sync_notifier_observer_);