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

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

BUG=chromium:125536
TEST=Tested the following:
1. Run "unit_tests --gtest_filter='*GData*'"
2. Verify on a debug build that GetCacheStateOnUIThread no longer fails with a lock assertion.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134518

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

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2141: // Always post a task
to the UI thread to call GetCacheStateOnUIThread even if
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?

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4412: void
GDataFileSystem::UnsafeInitializeCacheIfNecessary() {
We could remove UnsafeInitializeCacheIfNecessary as it's only called by
GDataCacheState.

[satorux1, 2012-04-30 05:52:05]

http://codereview.chromium.org/10272006/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10272006/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2141: // Always post a task
to the UI thread to call GetCacheStateOnUIThread even if
On 2012/04/30 04:37:21, Ben Chan wrote:
> 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?

Sounds good. Please add

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

http://codereview.chromium.org/10272006/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4412: void
GDataFileSystem::UnsafeInitializeCacheIfNecessary() {
On 2012/04/30 04:37:21, Ben Chan wrote:
> We could remove UnsafeInitializeCacheIfNecessary as it's only called by
> GDataCacheState.


Sounds great. Please remove it.

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

PTAL

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2141: // Always post a task
to the UI thread to call GetCacheStateOnUIThread even if
On 2012/04/30 05:52:06, satorux1 wrote:
> On 2012/04/30 04:37:21, Ben Chan wrote:
> > 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?
> 
> Sounds good. Please add
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
>       BrowserThread::CurrentlyOn(BrowserThread::IO));

Done.

https://chromiumcodereview.appspot.com/10272006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4412: void
GDataFileSystem::UnsafeInitializeCacheIfNecessary() {
On 2012/04/30 05:52:06, satorux1 wrote:
> On 2012/04/30 04:37:21, Ben Chan wrote:
> > We could remove UnsafeInitializeCacheIfNecessary as it's only called by
> > GDataCacheState.
> 
> 
> Sounds great. Please remove it.

Done.

[satorux1, 2012-04-30 06:22:09]

LGTM

http://codereview.chromium.org/10272006/diff/5004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (left):

http://codereview.chromium.org/10272006/diff/5004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:1158: void
UnsafeInitializeCacheIfNecessary();
one less function is great!

[commit-bot: I haz the power, 2012-04-30 06:38:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benchan@chromium.org/10272006/5004

[commit-bot: I haz the power, 2012-04-30 09:03:00]

Change committed as 134518