Refactor TabContentManager to async create ThumbnailCache.

Refactor TabContentManager to async create ThumbnailCache.

This avoids a StrictMode policy violation due to I/O on the UI thread.

BUG=473356
Committed: https://crrev.com/5c291c869f931d80f0d916c732510b46c47c6ca5
Cr-Commit-Position: refs/heads/master@{#329426}

[Peter Wen, 2015-05-06 20:48:13]

wnwen@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Peter Wen, 2015-05-06 20:48:14]

Hi David,

Would you mind taking a look at this refactor? This CL extracts ThumbnailCache
creation and fires an async task for it. If it is needed before the async task
completes, GetThumbnailCache blocks on that task until it is ready.

Thanks,

Peter

[David Trainor- moved to gerrit, 2015-05-07 17:57:36]

Hey Peter!  Some initial feedback.  Thanks for tackling this! :)

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:148:
nativeCreateThumbnailCache(mNativeTabContentManager, diskCachePath,
This doesn't look thread safe.  What if someone calls destroy() on this
TabContentManager in between the != 0 and the nativeCreateThumbnailCache call?

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/browser/andro...
File chrome/browser/android/compositor/tab_content_manager.cc (right):

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/browser/andro...
chrome/browser/android/compositor/tab_content_manager.cc:242:
GetThumbnailCache()->AddThumbnailCacheObserver(this);
Also doesn't feel thread safe.  I can set thumbnail_cache_ and get context
switched out in the middle of the AddThumbnailCacheObserver call, then someone
else can call AddThumbnailCacheObserver which would break the containers IIRC.

A solution for this would be to store the ThumbnailCache in a temporary variable
until you're done initializing it.  But honestly that *still* doesn't feel 100%
safe (I don't know if scoped_ptr's = operator works properly across threads).

There are two ways I would recommend approaching this (depending on how you
address the other race condition above):
First (if you can address the other issue separately):
1. Call this method something like CreateThumbnailCacheOnWorkerThread.
2. Build the ThumbnailCache object and store it as a temp var.
3. Use PostTask to call another method on the UI thread and pass the
ThumbnailCache there.
4. In that method on the UI thread call AddThumbnailCacheObserver, then set the
permanent variable.

Second (probably better):
1. Make this a static method.  Have it return the ThumbnailCache object.
2. Have the Java AsyncTask#onPostExecute call a native SetThumbnailCache if the
native TabContentsManager still exists.  If not, have it call another static
method that erases the ThumbnailCache (in case we get destroyed while running
the AsyncTask).

Going with 2 fixes both immediate race condition issues.  I would probably do
that.  You still might want to look into storing thumbnail_cache_ in a special
class though (see thread_proxy.h/.cc for how CC deals with using inner classes
for some threading issues).  But I'm fine with you not doing that as long as you
document it really well in the header file.

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/browser/andro...
File chrome/browser/android/compositor/tab_content_manager.h (right):

https://chromiumcodereview.appspot.com/1124333002/diff/1/chrome/browser/andro...
chrome/browser/android/compositor/tab_content_manager.h:112:
scoped_ptr<ThumbnailCache> thumbnail_cache_;
Lots of comments explaining how (not) to access this variable... In fact it
would be better to probably add an inner class with a get so we can hide the
thumbnail_cache_ variable from everywhere else.

[Peter Wen, 2015-05-07 22:08:10]

Thanks for your detailed review and finding the race condition! Really
appreciate you outlining your two solutions as I'm fairly new to this code. :)

PTAL.

https://codereview.chromium.org/1124333002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://codereview.chromium.org/1124333002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:148:
nativeCreateThumbnailCache(mNativeTabContentManager, diskCachePath,
On 2015/05/07 17:57:35, David Trainor wrote:
> This doesn't look thread safe.  What if someone calls destroy() on this
> TabContentManager in between the != 0 and the nativeCreateThumbnailCache call?

Done.

https://codereview.chromium.org/1124333002/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/tab_content_manager.cc (right):

https://codereview.chromium.org/1124333002/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/tab_content_manager.cc:242:
GetThumbnailCache()->AddThumbnailCacheObserver(this);
On 2015/05/07 17:57:36, David Trainor wrote:
> Also doesn't feel thread safe.  I can set thumbnail_cache_ and get context
> switched out in the middle of the AddThumbnailCacheObserver call, then someone
> else can call AddThumbnailCacheObserver which would break the containers IIRC.
> 
> A solution for this would be to store the ThumbnailCache in a temporary
variable
> until you're done initializing it.  But honestly that *still* doesn't feel
100%
> safe (I don't know if scoped_ptr's = operator works properly across threads).
> 
> There are two ways I would recommend approaching this (depending on how you
> address the other race condition above):
> First (if you can address the other issue separately):
> 1. Call this method something like CreateThumbnailCacheOnWorkerThread.
> 2. Build the ThumbnailCache object and store it as a temp var.
> 3. Use PostTask to call another method on the UI thread and pass the
> ThumbnailCache there.
> 4. In that method on the UI thread call AddThumbnailCacheObserver, then set
the
> permanent variable.
> 
> Second (probably better):
> 1. Make this a static method.  Have it return the ThumbnailCache object.
> 2. Have the Java AsyncTask#onPostExecute call a native SetThumbnailCache if
the
> native TabContentsManager still exists.  If not, have it call another static
> method that erases the ThumbnailCache (in case we get destroyed while running
> the AsyncTask).
> 
> Going with 2 fixes both immediate race condition issues.  I would probably do
> that.  You still might want to look into storing thumbnail_cache_ in a special
> class though (see thread_proxy.h/.cc for how CC deals with using inner classes
> for some threading issues).  But I'm fine with you not doing that as long as
you
> document it really well in the header file.

Done. thread_proxy seems to just group unsafe variables in a struct with a
comment saying to not use the struct directly but use the accessor method. Is my
comment sufficient?

https://codereview.chromium.org/1124333002/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/tab_content_manager.h (right):

https://codereview.chromium.org/1124333002/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/tab_content_manager.h:112:
scoped_ptr<ThumbnailCache> thumbnail_cache_;
On 2015/05/07 17:57:36, David Trainor wrote:
> Lots of comments explaining how (not) to access this variable... In fact it
> would be better to probably add an inner class with a get so we can hide the
> thumbnail_cache_ variable from everywhere else.

Acknowledged.

[David Trainor- moved to gerrit, 2015-05-08 06:12:38]

lgtm

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:45:
private long mNativeThumbnailCache;
You can probably remove this

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:170:
private void updateThumbnailCache(Long resultThumbnailCache) {
use long instead of Long here.  Or you could probably just inline this inside
onPostExecute if you want :).

https://codereview.chromium.org/1124333002/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/tab_content_manager.cc (right):

https://codereview.chromium.org/1124333002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/tab_content_manager.cc:231: thumbnail_cache_ =
make_scoped_ptr(
DCHECK(!thumbnail_cache_).  Or handle the case where it's not null (remove
observer from old one), but I'm fine with the DCHECK and assuming this is only
called once.

[Peter Wen, 2015-05-11 16:48:20]

Thanks, PTAL for the double-call issue with early blocking.

I feel like there should be a better way to refactor this code in order to avoid
unnecessary early loading of ThumbnailCache. :-/

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:45:
private long mNativeThumbnailCache;
On 2015/05/08 06:12:38, David Trainor wrote:
> You can probably remove this

Added comment to explain reason for storing this.

https://codereview.chromium.org/1124333002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:170:
private void updateThumbnailCache(Long resultThumbnailCache) {
On 2015/05/08 06:12:38, David Trainor wrote:
> use long instead of Long here.  Or you could probably just inline this inside
> onPostExecute if you want :).

Fixed to use long. Also used in blockOnThumbnailCacheCreation.

https://codereview.chromium.org/1124333002/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/tab_content_manager.cc (right):

https://codereview.chromium.org/1124333002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/tab_content_manager.cc:231: thumbnail_cache_ =
make_scoped_ptr(
On 2015/05/08 06:12:38, David Trainor wrote:
> DCHECK(!thumbnail_cache_).  Or handle the case where it's not null (remove
> observer from old one), but I'm fine with the DCHECK and assuming this is only
> called once.

Added DCHECK.

We either DCHECK here and keep the guard in TabContentManager.java or add
deletion and observer removal here, since if we block early then it's called
twice.

[David Trainor- moved to gerrit, 2015-05-11 17:18:23]

https://codereview.chromium.org/1124333002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://codereview.chromium.org/1124333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:48:
private long mNativeThumbnailCache;
Ah I see your point!  .get() returns before onPostExecute() is run.  In that
case lets just check natively and handle that properly there :).  Good catch!

[Peter Wen, 2015-05-11 20:33:48]

PTAL.

Native should now be resilient to setting the same pointer twice.

https://codereview.chromium.org/1124333002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
(right):

https://codereview.chromium.org/1124333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:48:
private long mNativeThumbnailCache;
On 2015/05/11 17:18:22, David Trainor wrote:
> Ah I see your point!  .get() returns before onPostExecute() is run.  In that
> case lets just check natively and handle that properly there :).  Good catch!

Done.

[David Trainor- moved to gerrit, 2015-05-11 22:10:20]

lgtm

[Peter Wen, 2015-05-12 16:36:09]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-12 16:36:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124333002/60001

[Peter Wen, 2015-05-12 16:39:18]

The CQ bit was unchecked by wnwen@chromium.org

[Peter Wen, 2015-05-12 16:39:24]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2015-05-12 16:40:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124333002/60001

[commit-bot: I haz the power, 2015-05-12 17:02:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 17:03:49]

Patchset 4 (id:??) landed as
https://crrev.com/5c291c869f931d80f0d916c732510b46c47c6ca5
Cr-Commit-Position: refs/heads/master@{#329426}