Sync: Improve handling of database load failures

chrome/browser/sync/internal_api/sync_manager.cc

 // Copyright (c) 2011 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/internal_api/sync_manager.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/command_line.h"
@@ skipping 867 matching lines @@
   // syncing until at least the DirectoryManager broadcasts the OPENED
   // event, and a valid server connection is detected.
   if (scheduler())  // NULL during certain unittests.
     scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure());
 }
 
 bool SyncManager::SyncInternal::OpenDirectory() {
   DCHECK(!initialized_) << "Should only happen once";
 
   bool share_opened = dir_manager()->Open(username_for_share(), this);
-  DCHECK(share_opened);
   if (!share_opened) {
     LOG(ERROR) << "Could not open share for:" << username_for_share();
     return false;
   }
 
   // Database has to be initialized for the guid to be available.
   syncable::ScopedDirLookup lookup(dir_manager(), username_for_share());
   if (!lookup.good()) {
     NOTREACHED();
     return false;
@@ skipping 376 matching lines @@
   sync_notifier_.reset();
 
   if (connection_manager_.get()) {
     connection_manager_->RemoveListener(this);
   }
   connection_manager_.reset();
 
   net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
   observing_ip_address_changes_ = false;
 
-  if (dir_manager()) {
+  if (initialized_ && dir_manager()) {
     {
       // Cryptographer should only be accessed while holding a
       // transaction.
       ReadTransaction trans(FROM_HERE, GetUserShare());
       trans.GetCryptographer()->RemoveObserver(this);
       trans.GetLookup()->
           RemoveTransactionObserver(&js_mutation_event_observer_);
       RemoveChangeObserver(&js_mutation_event_observer_);
     }
     dir_manager()->FinalSaveChangesForAll();

[Nicolas Zea, 2011/11/08 22:12:18]

Perhaps just condition these two statements? We should still probably remove the
observers above, although perhaps we'd be unable to acquire that
transaction...but in that case there's no contention so it should be safe to
remove them anyways I think

[rlarocque, 2011/11/11 22:24:56]

The transaction won't work.  The Directory ends up leaving kernel_ uninitialized
when the load fails, so there's no directory kernel, and therefore no lock to
acquire.  I suspect it would end up accessing invalid memory in release mode.   

Besides, we don't need to remove the observers because we never added them in
the first place.  The js_mutation_event_observer_ would have been added to the
observer list in OpenDirectory(), but not if the attempt to open the directory
failed.  The cryptographer observer would be setup in
SyncManager::SyncInternal::Init, but not if the "signed_in" local boolean value
is false, which it would be if we failed to open the directory.  

I admit that this logic is very brittle because it is mostly a coincidence that
!initialized_ implies a directory load failure.  I'm not sure how to fix this
without larger refactoring.  

     dir_manager()->Close(username_for_share());
   }
 
   // Reset the DirectoryManager and UserSettings so they relinquish sqlite
   // handles to backing files.
   share_.dir_manager.reset();
 
   setup_for_test_mode_ = false;
   change_delegate_ = NULL;
   registrar_ = NULL;
@@ skipping 770 matching lines @@
 
   for (syncable::ModelTypeSet::const_iterator i = types.begin();
        i != types.end(); ++i) {
     if (!lookup->initial_sync_ended_for_type(*i))
       return false;
   }
   return true;
 }
 
 }  // namespace sync_api