SyncFS: Reorder initialization sequence of SyncFileSystemService

chrome/browser/sync_file_system/local/sync_file_system_backend.cc

Index: chrome/browser/sync_file_system/local/sync_file_system_backend.cc
diff --git a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
index ac426c9bca6f8f8b0f8c2052ebe7e0fefcc1992c..898e903e6bdfd13faaa7128a7ec8311a678eaf21 100644
--- a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
+++ b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
@@ -8,7 +8,11 @@
 #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"
@@ -16,15 +20,19 @@
 #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());
   }
 }
@@ -37,17 +45,18 @@ bool SyncFileSystemBackend::CanHandleType(
 
 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(
@@ -56,21 +65,27 @@ void SyncFileSystemBackend::OpenFileSystem(
     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*
@@ -91,9 +106,8 @@ SyncFileSystemBackend::CreateFileSystemOperation(
   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;
 
@@ -113,8 +127,7 @@ SyncFileSystemBackend::CreateFileStreamReader(
     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);
 }
 
@@ -124,13 +137,12 @@ SyncFileSystemBackend::CreateFileStreamWriter(
     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
@@ -148,15 +160,15 @@ void SyncFileSystemBackend::SetLocalFileChangeTracker(
   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(
@@ -165,4 +177,58 @@ void SyncFileSystemBackend::set_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