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

chrome/browser/chromeos/gdata/gdata_file_system.cc

Index: chrome/browser/chromeos/gdata/gdata_file_system.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index 19d26ef89c28d9856623f832d5164e8aeddfff90..971b8d971464e7183a3d16cd12beeeb8ded2ec5b 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -2138,23 +2138,21 @@ GDataEntry* GDataFileSystem::GetGDataEntryByPath(
 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(
@@ -2163,9 +2161,7 @@ void GDataFileSystem::GetCacheStateOnUIThread(
     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);