[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887

Review-Url: https://codereview.chromium.org/2568743004
Cr-Commit-Position: refs/heads/master@{#439835}

chrome/browser/sync/test/integration/single_client_directory_sync_test.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
@@ -21,6 +23,22 @@
 #include "url/gurl.h"
 
 using content::BrowserThread;
+using base::FileEnumerator;
+using base::FilePath;
+
+namespace {
+
+// USS ModelTypeStore uses the same folder as the Directory. However, all of its
+// content is in a sub-folder. By not asking for recursive files, this function
+// will avoid seeing any of those, and return iff Directory database files still
+// exist.
+bool FolderContainsFiles(const FilePath& folder) {
+  if (base::DirectoryExists(folder)) {
+    return !FileEnumerator(folder, false, FileEnumerator::FILES).Next().empty();
+  } else {
+    return false;
+  }
+}
 
 class SingleClientDirectorySyncTest : public SyncTest {
  public:
@@ -63,8 +81,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
                        StopThenDisableDeletesDirectory) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   browser_sync::ProfileSyncService* sync_service = GetSyncService(0);
-  base::FilePath directory_path = sync_service->GetDirectoryPathForTest();
-  ASSERT_TRUE(base::DirectoryExists(directory_path));
+  FilePath directory_path = sync_service->GetDirectoryPathForTest();
+  ASSERT_TRUE(FolderContainsFiles(directory_path));
   sync_service->RequestStop(browser_sync::ProfileSyncService::CLEAR_DATA);
 
   // Wait for StartupController::StartUp()'s tasks to finish.
@@ -75,8 +93,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
   // Wait for the directory deletion to finish.
   base::MessageLoop* sync_loop = sync_service->GetSyncLoopForTest();
   ASSERT_TRUE(WaitForExistingTasksOnLoop(sync_loop));
-
-  ASSERT_FALSE(base::DirectoryExists(directory_path));
+  ASSERT_FALSE(FolderContainsFiles(directory_path));
 }
 
 // Verify that when the sync directory's backing store becomes corrupted, we
@@ -100,8 +117,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
   ASSERT_TRUE(WaitForExistingTasksOnLoop(sync_loop));
 
   // Now corrupt the database.
-  const base::FilePath directory_path(sync_service->GetDirectoryPathForTest());
-  const base::FilePath sync_db(directory_path.Append(
+  const FilePath directory_path(sync_service->GetDirectoryPathForTest());
+  const FilePath sync_db(directory_path.Append(
       syncer::syncable::Directory::kSyncDatabaseFilename));
   ASSERT_TRUE(sql::test::CorruptSizeInHeaderWithLock(sync_db));
 
@@ -123,5 +140,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
   // Wait until the sync loop has processed any existing tasks and see that the
   // directory no longer exists.
   ASSERT_TRUE(WaitForExistingTasksOnLoop(sync_loop));
-  ASSERT_FALSE(base::DirectoryExists(directory_path));
+  ASSERT_FALSE(FolderContainsFiles(directory_path));
 }
+
+}  // namespace

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -313,9 +313,9 @@ syncer::SyncEngine* ProfileSyncComponentsFactoryImpl::CreateSyncEngine(
     const std::string& name,
     invalidation::InvalidationService* invalidator,
     const base::WeakPtr<syncer::SyncPrefs>& sync_prefs,
-    const base::FilePath& sync_folder) {
+    const base::FilePath& sync_data_folder) {
   return new syncer::SyncBackendHostImpl(name, sync_client_, invalidator,
-                                         sync_prefs, sync_folder);
+                                         sync_prefs, sync_data_folder);
 }
 
 std::unique_ptr<syncer::LocalDeviceInfoProvider>

components/browser_sync/profile_sync_components_factory_impl.h

@@ -76,7 +76,7 @@ class ProfileSyncComponentsFactoryImpl
       const std::string& name,
       invalidation::InvalidationService* invalidator,
       const base::WeakPtr<syncer::SyncPrefs>& sync_prefs,
-      const base::FilePath& sync_folder) override;
+      const base::FilePath& sync_data_folder) override;
   std::unique_ptr<syncer::LocalDeviceInfoProvider>
   CreateLocalDeviceInfoProvider() override;
   std::unique_ptr<syncer::AttachmentService> CreateAttachmentService(

components/browser_sync/profile_sync_service.cc

@@ -172,15 +172,6 @@ const net::BackoffEntry::Policy kRequestAccessTokenBackoffPolicy = {
 const base::FilePath::CharType kLevelDBFolderName[] =
     FILE_PATH_LITERAL("LevelDB");
 
-// Perform the actual sync data folder deletion.
-// This should only be called on the sync thread.
-void DeleteSyncDataFolder(const base::FilePath& directory_path) {
-  if (base::DirectoryExists(directory_path)) {
-    if (!base::DeleteFile(directory_path, true))
-      LOG(DFATAL) << "Could not delete the Sync Data folder.";
-  }
-}
-
 }  // namespace
 
 ProfileSyncService::InitParams::InitParams() = default;
@@ -288,10 +279,10 @@ void ProfileSyncService::Initialize() {
     // TODO(skym): Stop creating leveldb files when signed out.
     // TODO(skym): Verify using AsUTF8Unsafe is okay here. Should work as long
     // as the Local State file is guaranteed to be UTF-8.
-    device_info_service_ = base::MakeUnique<DeviceInfoSyncBridge>(
+    device_info_sync_bridge_ = base::MakeUnique<DeviceInfoSyncBridge>(
         local_device_.get(),
         base::Bind(&ModelTypeStore::CreateStore, syncer::DEVICE_INFO,
-                   directory_path_.Append(base::FilePath(kLevelDBFolderName))
+                   sync_data_folder_.Append(base::FilePath(kLevelDBFolderName))
                        .AsUTF8Unsafe(),
                    blocking_task_runner),
         base::Bind(&ModelTypeChangeProcessor::Create));
@@ -447,8 +438,8 @@ sync_sessions::FaviconCache* ProfileSyncService::GetFaviconCache() {
 syncer::DeviceInfoTracker* ProfileSyncService::GetDeviceInfoTracker() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   // One of the two should always be non-null after initialization is done.
-  if (device_info_service_) {
-    return device_info_service_.get();
+  if (device_info_sync_bridge_) {
+    return device_info_sync_bridge_.get();
   } else {
     return device_info_sync_service_.get();
   }
@@ -595,7 +586,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
 
   engine_.reset(sync_client_->GetSyncApiComponentFactory()->CreateSyncEngine(
       debug_identifier_, invalidator, sync_prefs_.AsWeakPtr(),
-      directory_path_));
+      sync_data_folder_));
 
   // Clear any old errors the first time sync starts.
   if (!IsFirstSetupComplete())
@@ -728,7 +719,9 @@ void ProfileSyncService::ShutdownImpl(syncer::ShutdownReason reason) {
       // If the engine is already shut down when a DISABLE_SYNC happens,
       // the data directory needs to be cleaned up here.
       sync_thread_->task_runner()->PostTask(
-          FROM_HERE, base::Bind(&DeleteSyncDataFolder, directory_path_));
+          FROM_HERE,
+          base::Bind(&syncer::syncable::Directory::DeleteDirectoryFiles,
+                     sync_data_folder_));
     }
     return;
   }
@@ -2477,7 +2470,7 @@ syncer::SyncableService* ProfileSyncService::GetDeviceInfoSyncableService() {
 
 syncer::ModelTypeSyncBridge* ProfileSyncService::GetDeviceInfoSyncBridge() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return device_info_service_.get();
+  return device_info_sync_bridge_.get();
 }
 
 syncer::SyncService::SyncTokenStatus ProfileSyncService::GetSyncTokenStatus()
@@ -2523,7 +2516,7 @@ void ProfileSyncService::FlushDirectory() const {
 
 base::FilePath ProfileSyncService::GetDirectoryPathForTest() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return directory_path_;
+  return sync_data_folder_;
 }
 
 base::MessageLoop* ProfileSyncService::GetSyncLoopForTest() const {

components/browser_sync/profile_sync_service.h

@@ -911,7 +911,7 @@ class ProfileSyncService : public syncer::SyncServiceBase,
   // Locally owned SyncableService and ModelTypeSyncBridge implementations.
   std::unique_ptr<sync_sessions::SessionsSyncManager> sessions_sync_manager_;
   std::unique_ptr<syncer::DeviceInfoSyncService> device_info_sync_service_;
-  std::unique_ptr<syncer::DeviceInfoSyncBridge> device_info_service_;
+  std::unique_ptr<syncer::DeviceInfoSyncBridge> device_info_sync_bridge_;
 
   std::unique_ptr<syncer::NetworkResources> network_resources_;
 

components/sync/driver/glue/sync_backend_host_core.cc

@@ -28,6 +28,7 @@
 #include "components/sync/engine/sync_backend_registrar.h"
 #include "components/sync/engine/sync_manager.h"
 #include "components/sync/engine/sync_manager_factory.h"
+#include "components/sync/syncable/directory.h"
 
 // Helper macros to log with the syncer thread name; useful when there
 // are multiple syncers involved.
@@ -57,10 +58,10 @@ class EngineComponentsFactory;
 
 SyncBackendHostCore::SyncBackendHostCore(
     const std::string& name,
-    const base::FilePath& sync_data_folder_path,
+    const base::FilePath& sync_data_folder,
     const base::WeakPtr<SyncBackendHostImpl>& backend)
     : name_(name),
-      sync_data_folder_path_(sync_data_folder_path),
+      sync_data_folder_(sync_data_folder),
       host_(backend),
       weak_ptr_factory_(this) {
   DCHECK(backend.get());
@@ -325,12 +326,12 @@ void SyncBackendHostCore::DoInitialize(SyncEngine::InitParams params) {
   // Blow away the partial or corrupt sync data folder before doing any more
   // initialization, if necessary.
   if (params.delete_sync_data_folder) {
-    DeleteSyncDataFolder();
+    syncable::Directory::DeleteDirectoryFiles(sync_data_folder_);
   }
 
   // Make sure that the directory exists before initializing the backend.
   // If it already exists, this will do no harm.
-  if (!base::CreateDirectory(sync_data_folder_path_)) {
+  if (!base::CreateDirectory(sync_data_folder_)) {
     DLOG(FATAL) << "Sync Data directory creation failed.";
   }
 
@@ -345,7 +346,7 @@ void SyncBackendHostCore::DoInitialize(SyncEngine::InitParams params) {
   sync_manager_->AddObserver(this);
 
   SyncManager::InitArgs args;
-  args.database_location = sync_data_folder_path_;
+  args.database_location = sync_data_folder_;
   args.event_handler = params.event_handler;
   args.service_url = params.service_url;
   args.enable_local_sync_backend = params.enable_local_sync_backend;
@@ -480,7 +481,7 @@ void SyncBackendHostCore::DoShutdown(ShutdownReason reason) {
   registrar_ = nullptr;
 
   if (reason == DISABLE_SYNC)
-    DeleteSyncDataFolder();
+    syncable::Directory::DeleteDirectoryFiles(sync_data_folder_);
 
   host_.Reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
@@ -601,14 +602,6 @@ void SyncBackendHostCore::DisableDirectoryTypeDebugInfoForwarding() {
     sync_manager_->UnregisterDirectoryTypeDebugInfoObserver(this);
 }
 
-void SyncBackendHostCore::DeleteSyncDataFolder() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (base::DirectoryExists(sync_data_folder_path_)) {
-    if (!base::DeleteFile(sync_data_folder_path_, true))
-      SLOG(DFATAL) << "Could not delete the Sync Data folder.";
-  }
-}
-
 void SyncBackendHostCore::StartSavingChanges() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!save_changes_timer_.get());

components/sync/driver/glue/sync_backend_host_core.h

@@ -39,7 +39,7 @@ class SyncBackendHostCore
       public TypeDebugInfoObserver {
  public:
   SyncBackendHostCore(const std::string& name,
-                      const base::FilePath& sync_data_folder_path,
+                      const base::FilePath& sync_data_folder,
                       const base::WeakPtr<SyncBackendHostImpl>& backend);
 
   // MemoryDumpProvider implementation.
@@ -173,11 +173,6 @@ class SyncBackendHostCore
   // Disables forwarding of directory type debug counters.
   void DisableDirectoryTypeDebugInfoForwarding();
 
-  // Delete the sync data folder to cleanup backend data.  Happens the first
-  // time sync is enabled for a user (to prevent accidentally reusing old
-  // sync databases), as well as shutdown when you're no longer syncing.
-  void DeleteSyncDataFolder();
-
   // Tell the sync manager to persist its state by writing to disk.
   // Called on the sync thread, both by a timer and, on Android, when the
   // application is backgrounded.
@@ -206,7 +201,7 @@ class SyncBackendHostCore
   const std::string name_;
 
   // Path of the folder that stores the sync data files.
-  const base::FilePath sync_data_folder_path_;
+  const base::FilePath sync_data_folder_;
 
   // Our parent SyncBackendHostImpl.
   WeakHandle<SyncBackendHostImpl> host_;

components/sync/driver/glue/sync_backend_host_impl.cc

@@ -47,13 +47,13 @@ SyncBackendHostImpl::SyncBackendHostImpl(
     SyncClient* sync_client,
     invalidation::InvalidationService* invalidator,
     const base::WeakPtr<SyncPrefs>& sync_prefs,
-    const base::FilePath& sync_folder)
+    const base::FilePath& sync_data_folder)
     : sync_client_(sync_client),
       name_(name),
       sync_prefs_(sync_prefs),
       invalidator_(invalidator),
       weak_ptr_factory_(this) {
-  core_ = new SyncBackendHostCore(name_, sync_folder,
+  core_ = new SyncBackendHostCore(name_, sync_data_folder,
                                   weak_ptr_factory_.GetWeakPtr());
 }
 

components/sync/driver/glue/sync_backend_host_impl.h

@@ -49,12 +49,11 @@ class SyncBackendHostImpl : public SyncEngine, public InvalidationHandler {
  public:
   typedef SyncStatus Status;
 
-  SyncBackendHostImpl(
-      const std::string& name,
-      SyncClient* sync_client,
-      invalidation::InvalidationService* invalidator,
-      const base::WeakPtr<SyncPrefs>& sync_prefs,
-      const base::FilePath& sync_folder);
+  SyncBackendHostImpl(const std::string& name,
+                      SyncClient* sync_client,
+                      invalidation::InvalidationService* invalidator,
+                      const base::WeakPtr<SyncPrefs>& sync_prefs,
+                      const base::FilePath& sync_data_folder);
   ~SyncBackendHostImpl() override;
 
   // SyncEngine implementation.

components/sync/driver/sync_service_base.cc

@@ -61,7 +61,7 @@ SyncServiceBase::SyncServiceBase(std::unique_ptr<SyncClient> sync_client,
       signin_(std::move(signin)),
       channel_(channel),
       base_directory_(base_directory),
-      directory_path_(
+      sync_data_folder_(
           base_directory_.Append(base::FilePath(kSyncDataFolderName))),
       debug_identifier_(debug_identifier),
       sync_prefs_(sync_client_->GetPrefService()) {}

components/sync/driver/sync_service_base.h

@@ -76,8 +76,10 @@ class SyncServiceBase : public SyncService, public SyncEngineHost {
   // information.
   const base::FilePath base_directory_;
 
-  // The full path to the sync data directory.
-  const base::FilePath directory_path_;
+  // The full path to the sync data folder. The folder is not fully deleted when
+  // sync is disabled, since it holds both Directory and ModelTypeStore data.
+  // Directory files will be selectively targeted instead.
+  const base::FilePath sync_data_folder_;
 
   // An identifier representing this instance for debugging purposes.
   const std::string debug_identifier_;

components/sync/syncable/directory.cc

@@ -11,7 +11,9 @@
 #include <utility>
 
 #include "base/base64.h"
+#include "base/files/file_enumerator.h"
 #include "base/guid.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
@@ -432,6 +434,23 @@ bool Directory::ReindexParentId(BaseWriteTransaction* trans,
   return true;
 }
 
+// static
+void Directory::DeleteDirectoryFiles(const base::FilePath& directory_path) {
+  // We assume that the directory database files are all top level files, and
+  // use no folders. We also assume that there might be child folders under
+  // |directory_path| that are used for non-directory things, like storing
+  // ModelTypeStore/LevelDB data, and we expressly do not want to delete those.
+  if (base::DirectoryExists(directory_path)) {
+    base::FileEnumerator fe(directory_path, false, base::FileEnumerator::FILES);
+    for (base::FilePath current = fe.Next(); !current.empty();
+         current = fe.Next()) {
+      if (!base::DeleteFile(current, false)) {
+        LOG(DFATAL) << "Could not delete all sync directory files.";
+      }
+    }
+  }
+}
+
 void Directory::RemoveFromAttachmentIndex(
     const ScopedKernelLock& lock,
     const int64_t metahandle,

components/sync/syncable/directory.h

@@ -527,6 +527,12 @@ class Directory {
   Kernel* kernel();
   const Kernel* kernel() const;
 
+  // Delete the directory database files from the sync data folder to cleanup
+  // backend data. This should happen the first time sync is enabled for a user,
+  // to prevent accidentally reusing old sync data, as well as shutdown when the
+  // user is no longer syncing.
+  static void DeleteDirectoryFiles(const base::FilePath& directory_path);
+
  private:
   friend class SyncableDirectoryTest;
   friend class syncer::TestUserShare;