Refactor TabContentManager to async create ThumbnailCache.

chrome/browser/android/compositor/tab_content_manager.cc

Index: chrome/browser/android/compositor/tab_content_manager.cc
diff --git a/chrome/browser/android/compositor/tab_content_manager.cc b/chrome/browser/android/compositor/tab_content_manager.cc
index 0340aac2e6c08de88c0b6449ad06a400dd81f596..6c1b2d00b302014640c82c527c05623a3420d3a4 100644
--- a/chrome/browser/android/compositor/tab_content_manager.cc
+++ b/chrome/browser/android/compositor/tab_content_manager.cc
@@ -128,28 +128,14 @@ TabContentManager* TabContentManager::FromJavaObject(jobject jobj) {
 }
 
 TabContentManager::TabContentManager(JNIEnv* env,
-                                     jobject obj,
-                                     jstring disk_cache_path,
-                                     jint default_cache_size,
-                                     jint approximation_cache_size,
-                                     jint compression_queue_max_size,
-                                     jint write_queue_max_size,
-                                     jboolean use_approximation_thumbnail)
-    : weak_java_tab_content_manager_(env, obj), weak_factory_(this) {
-  std::string disk_cache_path_str =
-      base::android::ConvertJavaStringToUTF8(env, disk_cache_path);
-  thumbnail_cache_ = make_scoped_ptr(new ThumbnailCache(
-      disk_cache_path_str, (size_t)default_cache_size,
-      (size_t)approximation_cache_size, (size_t)compression_queue_max_size,
-      (size_t)write_queue_max_size, use_approximation_thumbnail));
-  thumbnail_cache_->AddThumbnailCacheObserver(this);
-}
+                                     jobject obj)
+    : weak_java_tab_content_manager_(env, obj), weak_factory_(this) {}
 
 TabContentManager::~TabContentManager() {
 }
 
 void TabContentManager::Destroy(JNIEnv* env, jobject obj) {
-  thumbnail_cache_->RemoveThumbnailCacheObserver(this);
+  GetThumbnailCache()->RemoveThumbnailCacheObserver(this);
   delete this;
 }
 
@@ -163,7 +149,7 @@ void TabContentManager::SetUIResourceProvider(JNIEnv* env,
 
 void TabContentManager::SetUIResourceProvider(
     ui::UIResourceProvider* ui_resource_provider) {
-  thumbnail_cache_->SetUIResourceProvider(ui_resource_provider);
+  GetThumbnailCache()->SetUIResourceProvider(ui_resource_provider);
 }
 
 scoped_refptr<cc::Layer> TabContentManager::GetLiveLayer(int tab_id) {
@@ -177,7 +163,8 @@ scoped_refptr<cc::Layer> TabContentManager::GetLiveLayer(int tab_id) {
 scoped_refptr<ThumbnailLayer> TabContentManager::GetStaticLayer(
     int tab_id,
     bool force_disk_read) {
-  Thumbnail* thumbnail = thumbnail_cache_->Get(tab_id, force_disk_read, true);
+  Thumbnail* thumbnail =
+      GetThumbnailCache()->Get(tab_id, force_disk_read, true);
   scoped_refptr<ThumbnailLayer> static_layer = static_layer_cache_[tab_id];
 
   if (!thumbnail || !thumbnail->ui_resource_id()) {
@@ -237,10 +224,28 @@ void TabContentManager::OnFinishDecompressThumbnail(int tab_id,
       java_bitmap.obj());
 }
 
+void TabContentManager::CreateThumbnailCache(
+    JNIEnv* env,
+    jobject obj,
+    jstring disk_cache_path,
+    jint default_cache_size,
+    jint approximation_cache_size,
+    jint compression_queue_max_size,
+    jint write_queue_max_size,
+    jboolean use_approximation_thumbnail) {
+  std::string disk_cache_path_str =
+      base::android::ConvertJavaStringToUTF8(env, disk_cache_path);
+  thumbnail_cache_ = make_scoped_ptr(new ThumbnailCache(
+      disk_cache_path_str, (size_t)default_cache_size,
+      (size_t)approximation_cache_size, (size_t)compression_queue_max_size,
+      (size_t)write_queue_max_size, use_approximation_thumbnail));
+  GetThumbnailCache()->AddThumbnailCacheObserver(this);

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

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.

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

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?

+}
+
 jboolean TabContentManager::HasFullCachedThumbnail(JNIEnv* env,
                                                    jobject obj,
                                                    jint tab_id) {
-  return thumbnail_cache_->Get(tab_id, false, false) != nullptr;
+  return GetThumbnailCache()->Get(tab_id, false, false) != nullptr;
 }
 
 void TabContentManager::CacheTab(JNIEnv* env,
@@ -257,14 +262,14 @@ void TabContentManager::CacheTab(JNIEnv* env,
       content::ContentViewCore::GetNativeContentViewCore(env,
                                                          content_view_core);
 
-  if (thumbnail_cache_->CheckAndUpdateThumbnailMetaData(tab_id, url)) {
+  if (GetThumbnailCache()->CheckAndUpdateThumbnailMetaData(tab_id, url)) {
     if (!view ||
         !view->GetWebContents()
              ->GetRenderViewHost()
              ->CanCopyFromBackingStore() ||
         pending_tab_readbacks_.find(tab_id) != pending_tab_readbacks_.end() ||
         pending_tab_readbacks_.size() >= kMaxReadbacks) {
-      thumbnail_cache_->Remove(tab_id);
+      GetThumbnailCache()->Remove(tab_id);
       return;
     }
 
@@ -293,7 +298,7 @@ void TabContentManager::CacheTabWithBitmap(JNIEnv* env,
   SkBitmap skbitmap = gfx::CreateSkBitmapFromJavaBitmap(java_bitmap_lock);
   skbitmap.setImmutable();
 
-  if (thumbnail_cache_->CheckAndUpdateThumbnailMetaData(tab_id, url))
+  if (GetThumbnailCache()->CheckAndUpdateThumbnailMetaData(tab_id, url))
     PutThumbnailIntoCache(tab_id, thumbnail_scale, skbitmap);
 }
 
@@ -301,7 +306,7 @@ void TabContentManager::InvalidateIfChanged(JNIEnv* env,
                                             jobject obj,
                                             jint tab_id,
                                             jstring jurl) {
-  thumbnail_cache_->InvalidateThumbnailIfChanged(
+  GetThumbnailCache()->InvalidateThumbnailIfChanged(
       tab_id, GURL(base::android::ConvertJavaStringToUTF8(env, jurl)));
 }
 
@@ -315,7 +320,7 @@ void TabContentManager::UpdateVisibleIds(JNIEnv* env,
     priority_ids.push_back(static_cast<int>(ints[i]));
 
   env->ReleaseIntArrayElements(priority, ints, JNI_ABORT);
-  thumbnail_cache_->UpdateVisibleIds(priority_ids);
+  GetThumbnailCache()->UpdateVisibleIds(priority_ids);
 }
 
 void TabContentManager::RemoveTabThumbnail(JNIEnv* env,
@@ -325,14 +330,14 @@ void TabContentManager::RemoveTabThumbnail(JNIEnv* env,
       pending_tab_readbacks_.find(tab_id);
   if (readback_iter != pending_tab_readbacks_.end())
     readback_iter->second->SetToDropAfterReadback();
-  thumbnail_cache_->Remove(tab_id);
+  GetThumbnailCache()->Remove(tab_id);
 }
 
 void TabContentManager::RemoveTabThumbnailFromDiskAtAndAboveId(
     JNIEnv* env,
     jobject obj,
     jint min_forbidden_id) {
-  thumbnail_cache_->RemoveFromDiskAtAndAboveId(min_forbidden_id);
+  GetThumbnailCache()->RemoveFromDiskAtAndAboveId(min_forbidden_id);
 }
 
 void TabContentManager::GetDecompressedThumbnail(JNIEnv* env,
@@ -341,8 +346,8 @@ void TabContentManager::GetDecompressedThumbnail(JNIEnv* env,
   base::Callback<void(bool, SkBitmap)> decompress_done_callback =
       base::Bind(&TabContentManager::OnFinishDecompressThumbnail,
                  weak_factory_.GetWeakPtr(), reinterpret_cast<int>(tab_id));
-  thumbnail_cache_->DecompressThumbnailFromFile(reinterpret_cast<int>(tab_id),
-                                                decompress_done_callback);
+  GetThumbnailCache()->DecompressThumbnailFromFile(
+      reinterpret_cast<int>(tab_id), decompress_done_callback);
 }
 
 void TabContentManager::OnFinishedThumbnailRead(int tab_id) {
@@ -351,6 +356,16 @@ void TabContentManager::OnFinishedThumbnailRead(int tab_id) {
       env, weak_java_tab_content_manager_.get(env).obj(), tab_id);
 }
 
+ThumbnailCache* TabContentManager::GetThumbnailCache() {
+  if (thumbnail_cache_ == nullptr) {
+    JNIEnv* env = base::android::AttachCurrentThread();
+
+    Java_TabContentManager_blockOnThumbnailCacheCreation(
+        env, weak_java_tab_content_manager_.get(env).obj());
+  }
+  return thumbnail_cache_.get();
+}
+
 void TabContentManager::PutThumbnailIntoCache(int tab_id,
                                               float thumbnail_scale,
                                               const SkBitmap& bitmap) {
@@ -361,7 +376,7 @@ void TabContentManager::PutThumbnailIntoCache(int tab_id,
     pending_tab_readbacks_.erase(tab_id);
 
   if (thumbnail_scale > 0 && !bitmap.empty())
-    thumbnail_cache_->Put(tab_id, bitmap, thumbnail_scale);
+    GetThumbnailCache()->Put(tab_id, bitmap, thumbnail_scale);
 }
 
 bool RegisterTabContentManager(JNIEnv* env) {
@@ -372,18 +387,8 @@ bool RegisterTabContentManager(JNIEnv* env) {
 // Native JNI methods
 // ----------------------------------------------------------------------------
 
-jlong Init(JNIEnv* env,
-           jobject obj,
-           jstring disk_cache_path,
-           jint default_cache_size,
-           jint approximation_cache_size,
-           jint compression_queue_max_size,
-           jint write_queue_max_size,
-           jboolean use_approximation_thumbnail) {
-  TabContentManager* manager = new TabContentManager(
-      env, obj, disk_cache_path, default_cache_size, approximation_cache_size,
-      compression_queue_max_size, write_queue_max_size,
-      use_approximation_thumbnail);
+jlong Init(JNIEnv* env, jobject obj) {
+  TabContentManager* manager = new TabContentManager(env, obj);
   return reinterpret_cast<intptr_t>(manager);
 }