gdata: Fix GDataFileSystem::GetCacheStateOnUIThread to acquire lock for cache initialization.

chrome/browser/chromeos/gdata/gdata_file_system.cc

 // Copyright (c) 2012 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/chromeos/gdata/gdata_file_system.h"
 
 #include <errno.h>
 #include <sys/stat.h>
 
 #include <set>
@@ skipping 2120 matching lines @@
   lock_.AssertAcquired();
   // Find directory element within the cached file system snapshot.
   ReadOnlyFindEntryDelegate find_delegate;
   root_->FindEntryByPath(file_path, &find_delegate);
   return find_delegate.entry();
 }
 
 void GDataFileSystem::GetCacheState(const std::string& resource_id,
                                     const std::string& md5,
                                     const GetCacheStateCallback& callback) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    const bool posted = BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&GDataFileSystem::GetCacheStateOnUIThread,
-                   ui_weak_ptr_,
-                   resource_id,
-                   md5,
-                   base::Bind(&RelayGetCacheStateCallback,
-                              base::MessageLoopProxy::current(),
-                              callback)));
-    DCHECK(posted);
-    return;
-  }
-
-  GetCacheStateOnUIThread(resource_id, md5, callback);
+  // Always post a task to the UI thread to call GetCacheStateOnUIThread even if

[Ben Chan, 2012/04/30 04:37:21]

Another way to fix this issue is to have GetCacheStateOnUIThread and
GetCacheStateOnUIThreadWithLock:

GetCacheStateOnUIThread assumes no lock on GDataFileSystem and is called when
GetCacheState is called from IO thread.

GetCacheStateOnUIThreadWithLock assumes GDataFileSystem is already locked and is
called when GetCacheState is called from UI thread.

But I slightly feel that it's simpler to always post a task to call
GetCacheStateOnUIThread to make sure that GDataFileSystem is always unlocked. As
a result of that, we could also remove UnsafeInitializeCacheIfNecessary.

Any thought on this?

[satorux1, 2012/04/30 05:52:06]

Sounds good. Please add

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
      BrowserThread::CurrentlyOn(BrowserThread::IO));

[Ben Chan, 2012/04/30 06:18:58]

Done.

+  // GetCacheState is called on the UI thread. This ensures that, regardless of
+  // whether GDataFileSystem is locked or not, GDataFileSystem is unlocked when
+  // GetCacheStateOnUIThread is called.
+  const bool posted = BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&GDataFileSystem::GetCacheStateOnUIThread,
+                 ui_weak_ptr_,
+                 resource_id,
+                 md5,
+                 base::Bind(&RelayGetCacheStateCallback,
+                            base::MessageLoopProxy::current(),
+                            callback)));
+  DCHECK(posted);
 }
 
 void GDataFileSystem::GetCacheStateOnUIThread(
     const std::string& resource_id,
     const std::string& md5,
     const GetCacheStateCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // This method originates from GDataFile::GetCacheState, which already locks,
-  // so we shouldn't lock here.
-  UnsafeInitializeCacheIfNecessary();
+  InitializeCacheIfNecessary();
 
   base::PlatformFileError* error =
       new base::PlatformFileError(base::PLATFORM_FILE_OK);
   int* cache_state = new int(GDataFile::CACHE_STATE_NONE);
 
   // GetCacheStateOnIOThreadPool won't do file IO, but post it to the thread
   // pool, as it must be performed after the cache is initialized.
   PostBlockingPoolSequencedTaskAndReply(
       FROM_HERE,
       base::Bind(&GDataFileSystem::GetCacheStateOnIOThreadPool,
@@ skipping 2227 matching lines @@
   bool* has_enough_space = new bool(false);
   PostBlockingPoolSequencedTask(
       FROM_HERE,
       base::Bind(&GDataFileSystem::FreeDiskSpaceIfNeeded,
                  base::Unretained(this),
                  base::Owned(has_enough_space)));
 }
 
 //============= GDataFileSystem: internal helper functions =====================
 
 void GDataFileSystem::UnsafeInitializeCacheIfNecessary() {

[Ben Chan, 2012/04/30 04:37:21]

We could remove UnsafeInitializeCacheIfNecessary as it's only called by
GDataCacheState.

[satorux1, 2012/04/30 05:52:06]

Sounds great. Please remove it.

[Ben Chan, 2012/04/30 06:18:58]

Done.

   lock_.AssertAcquired();
 
   if (cache_initialization_started_) {
     // Cache initialization is either in progress or has completed.
     return;
   }
 
   // Need to initialize cache.
 
   cache_initialization_started_ = true;
@@ skipping 154 matching lines @@
   pref_registrar_->Init(profile_->GetPrefs());
   pref_registrar_->Add(prefs::kDisableGDataHostedFiles, this);
 }
 
 void SetFreeDiskSpaceGetterForTesting(FreeDiskSpaceGetterInterface* getter) {
   delete global_free_disk_getter_for_testing;  // Safe to delete NULL;
   global_free_disk_getter_for_testing = getter;
 }
 
 }  // namespace gdata