SyncFS: Reorder initialization sequence of SyncFileSystemService

chrome/browser/sync_file_system/local/sync_file_system_backend.cc

 // Copyright 2013 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_file_system/local/sync_file_system_backend.h"
 
 #include "base/logging.h"
 #include "chrome/browser/sync_file_system/local/local_file_change_tracker.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_context.h"
 #include "chrome/browser/sync_file_system/local/syncable_file_system_operation.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
 #include "chrome/browser/sync_file_system/syncable_file_system_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "webkit/browser/fileapi/async_file_util_adapter.h"
 #include "webkit/browser/fileapi/file_system_context.h"
 #include "webkit/browser/fileapi/file_system_file_stream_reader.h"
 #include "webkit/browser/fileapi/file_system_operation_impl.h"
 #include "webkit/browser/fileapi/sandbox_file_stream_writer.h"
 #include "webkit/browser/fileapi/sandbox_quota_observer.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
+using content::BrowserThread;
+
 namespace sync_file_system {
 
-SyncFileSystemBackend::SyncFileSystemBackend()
-    : delegate_(NULL) {
+SyncFileSystemBackend::SyncFileSystemBackend(Profile* profile)
+    : context_(NULL),
+      profile_(profile),
+      skip_initialize_syncfs_service_for_testing_(false) {

[kinuko, 2013/08/23 10:02:03]

This is called on UI thread.. is it correct? Can we have an assertion?

[nhiroki, 2013/08/28 05:37:42]

Added an assertion.

I found some tests using CannedSyncableFileSystem call this on a thread other
than the UI thread, but these don't take care of Profile lifetime since
initialization routines will be skipped by |skip_initialize_...| flag and I
think it could ignore the assertion, so I introduced another creating method for
testing (i.e. CreateForTesting()).

Or, should I fix the CannedSyncableFileSystem to run this ctor on the UI thread?

[kinuko, 2013/08/28 06:51:28]

Which test fails?  In most cases I expect we could fix it by explicitly making
current message_loop UI_TYPE.  (replacing it with MessageLoopForUI or
initializing message_loop_(TYPE_UI)).

[nhiroki, 2013/08/28 07:46:46]

AFAIK, the following tests failed:

- LocalFileChangeTrackerTest
- LocalFileSyncContextTest
- SyncableFileSystemTest
- SyncableFileSystemUtilTest
- SyncableFileOperationRunnerTest

[kinuko, 2013/08/28 08:23:57]

Sorry I've overlooked this part.  I think it's ok to add a separate static
creation method (and avoid some checks for testing) but I'd like to avoid having
multiple ctors.  Can we avoid it?

Btw commonly seen pattern for safer thread dcheck is:
DCHECK(!BrowserThread::IsMessageLoopValid(UI) || BrowserThread::CurrentlyOn(UI))

 }
 
 SyncFileSystemBackend::~SyncFileSystemBackend() {
   if (change_tracker_) {
-    delegate_->file_task_runner()->DeleteSoon(
+    GetDelegate()->file_task_runner()->DeleteSoon(
         FROM_HERE, change_tracker_.release());
   }
 }
 
 bool SyncFileSystemBackend::CanHandleType(
     fileapi::FileSystemType type) const {
   return type == fileapi::kFileSystemTypeSyncable ||
          type == fileapi::kFileSystemTypeSyncableForInternalSync;
 }
 
 void SyncFileSystemBackend::Initialize(fileapi::FileSystemContext* context) {
   DCHECK(context);
-  DCHECK(!delegate_);
-  delegate_ = context->sandbox_delegate();
+  DCHECK(!context_);
+  context_ = context;
 
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
-  delegate_->AddFileUpdateObserver(
+      delegate->quota_observer(),
+      delegate->file_task_runner());
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncableForInternalSync,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
+      delegate->quota_observer(),
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::OpenFileSystem(
     const GURL& origin_url,
     fileapi::FileSystemType type,
     fileapi::OpenFileSystemMode mode,
     const OpenFileSystemCallback& callback) {
   DCHECK(CanHandleType(type));
-  DCHECK(delegate_);
-  delegate_->OpenFileSystem(origin_url, type, mode, callback,
-                            GetSyncableFileSystemRootURI(origin_url));
+
+  if (skip_initialize_syncfs_service_for_testing_) {
+    GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                  GetSyncableFileSystemRootURI(origin_url));
+    return;
+  }
+
+  SyncStatusCallback initialize_callback =
+      base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                 base::Unretained(this), origin_url, type, mode, callback);
+  InitializeSyncFileSystemService(origin_url, initialize_callback);
 }
 
 fileapi::FileSystemFileUtil* SyncFileSystemBackend::GetFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->sync_file_util();
+  return GetDelegate()->sync_file_util();
 }
 
 fileapi::AsyncFileUtil* SyncFileSystemBackend::GetAsyncFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->file_util();
+  return GetDelegate()->file_util();
 }
 
 fileapi::CopyOrMoveFileValidatorFactory*
 SyncFileSystemBackend::GetCopyOrMoveFileValidatorFactory(
     fileapi::FileSystemType type,
     base::PlatformFileError* error_code) {
   DCHECK(error_code);
   *error_code = base::PLATFORM_FILE_OK;
   return NULL;
 }
 
 fileapi::FileSystemOperation*
 SyncFileSystemBackend::CreateFileSystemOperation(
     const fileapi::FileSystemURL& url,
     fileapi::FileSystemContext* context,
     base::PlatformFileError* error_code) const {
   DCHECK(CanHandleType(url.type()));
   DCHECK(context);
   DCHECK(error_code);
 
-  DCHECK(delegate_);
   scoped_ptr<fileapi::FileSystemOperationContext> operation_context =
-      delegate_->CreateFileSystemOperationContext(url, context, error_code);
+      GetDelegate()->CreateFileSystemOperationContext(url, context, error_code);
   if (!operation_context)
     return NULL;
 
   if (url.type() == fileapi::kFileSystemTypeSyncableForInternalSync) {
     return new fileapi::FileSystemOperationImpl(
         url, context, operation_context.Pass());
   }
 
   return new SyncableFileSystemOperation(
       url, context, operation_context.Pass());
 }
 
 scoped_ptr<webkit_blob::FileStreamReader>
 SyncFileSystemBackend::CreateFileStreamReader(
     const fileapi::FileSystemURL& url,
     int64 offset,
     const base::Time& expected_modification_time,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamReader(
+  return GetDelegate()->CreateFileStreamReader(
       url, offset, expected_modification_time, context);
 }
 
 scoped_ptr<fileapi::FileStreamWriter>
 SyncFileSystemBackend::CreateFileStreamWriter(
     const fileapi::FileSystemURL& url,
     int64 offset,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamWriter(
+  return GetDelegate()->CreateFileStreamWriter(
       url, offset, context, fileapi::kFileSystemTypeSyncableForInternalSync);
 }
 
 fileapi::FileSystemQuotaUtil* SyncFileSystemBackend::GetQuotaUtil() {
-  return delegate_;
+  return GetDelegate();
 }
 
 // static
 SyncFileSystemBackend* SyncFileSystemBackend::GetBackend(
     const fileapi::FileSystemContext* file_system_context) {
   DCHECK(file_system_context);
   return static_cast<SyncFileSystemBackend*>(
       file_system_context->GetFileSystemBackend(
           fileapi::kFileSystemTypeSyncable));
 }
 
 void SyncFileSystemBackend::SetLocalFileChangeTracker(
     scoped_ptr<LocalFileChangeTracker> tracker) {
   DCHECK(!change_tracker_);
   DCHECK(tracker);
   change_tracker_ = tracker.Pass();
 
-  DCHECK(delegate_);
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
-  delegate_->AddFileChangeObserver(
+      delegate->file_task_runner());
+  delegate->AddFileChangeObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::set_sync_context(
     LocalFileSyncContext* sync_context) {
   DCHECK(!sync_context_);
   sync_context_ = sync_context;
 }
 
+fileapi::SandboxFileSystemBackendDelegate*
+SyncFileSystemBackend::GetDelegate() const {
+  DCHECK(context_);
+  DCHECK(context_->sandbox_delegate());
+  return context_->sandbox_delegate();
+}
+
+void SyncFileSystemBackend::InitializeSyncFileSystemService(
+    const GURL& origin_url,
+    const SyncStatusCallback& callback) {
+  // Repost to switch from IO thread to UI thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::InitializeSyncFileSystemService,
+                   base::Unretained(this), origin_url, callback));
+    return;
+  }
+
+  DCHECK(profile_);
+  SyncFileSystemService* service =
+      SyncFileSystemServiceFactory::GetForProfile(profile_);

[kinuko, 2013/08/23 10:02:03]

I'm not 100% sure at this point profile_ is always valid. We must be called on
IO thread via IPC, which must have been terminated before profile shutdown
happens, but here we're going back to UI thread... are we really safe?  Should
we listen to PROFILE_DESTROYED notifications on UI thread?  Or can we not to
start initialization of SyncFileSystemService when it's constructed but lazily
do so in InitializeForApp, construct it when SyncFileSystemBackend is created
(when we're sure profile_ is valid) and let it notify the backend when its
Shutdown() method is called for profile destruction?

[nhiroki, 2013/08/28 05:37:42]

Right. |profile_| could be invalidated before using here.

I adopted the former way (observing PROFILE_DESTROYED event) since it looks
simpler and the latter way might make
SyncFileSystemService-SyncFileSystemBackend relationship more tight (I'd like to
keep them simply...).

+  DCHECK(service);
+  service->InitializeForApp(context_, origin_url, callback);
+}
+
+void SyncFileSystemBackend::DidInitializeSyncFileSystemService(
+    const GURL& origin_url,
+    fileapi::FileSystemType type,
+    fileapi::OpenFileSystemMode mode,
+    const OpenFileSystemCallback& callback,
+    SyncStatusCode status) {
+  // Repost to switch from UI thread to IO thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                   base::Unretained(this),
+                   origin_url, type, mode, callback, status));
+    return;
+  }
+
+  if (status != sync_file_system::SYNC_STATUS_OK) {
+    callback.Run(GURL(), std::string(),
+                 PlatformFileErrorToSyncStatusCode(status));
+    return;
+  }
+
+  GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                GetSyncableFileSystemRootURI(origin_url));
+}
+
 }  // namespace sync_file_system