Refactor TabContentManager to async create ThumbnailCache.

chrome/browser/android/compositor/tab_content_manager.cc

 // Copyright 2014 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/android/compositor/tab_content_manager.h"
 
 #include <android/bitmap.h>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
@@ skipping 110 matching lines @@
 // static
 TabContentManager* TabContentManager::FromJavaObject(jobject jobj) {
   if (!jobj)
     return NULL;
   return reinterpret_cast<TabContentManager*>(
       Java_TabContentManager_getNativePtr(base::android::AttachCurrentThread(),
                                           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;
 }
 
 void TabContentManager::SetUIResourceProvider(JNIEnv* env,
                                               jobject obj,
                                               jlong ui_resource_provider_ptr) {
   ui::UIResourceProvider* ui_resource_provider =
       reinterpret_cast<ui::UIResourceProvider*>(ui_resource_provider_ptr);
   SetUIResourceProvider(ui_resource_provider);
 }
 
 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) {
   scoped_refptr<cc::Layer> layer = live_layer_list_[tab_id];
   if (!layer.get())
     return NULL;
 
   return layer;
 }
 
 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()) {
     if (static_layer.get()) {
       static_layer->layer()->RemoveFromParent();
       static_layer_cache_.erase(tab_id);
     }
     return NULL;
   }
 
@@ skipping 39 matching lines @@
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jobject> java_bitmap;
   if (success)
     java_bitmap = gfx::ConvertToJavaBitmap(&bitmap);
 
   Java_TabContentManager_notifyDecompressBitmapFinished(
       env, weak_java_tab_content_manager_.get(env).obj(), 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,
                                  jobject obj,
                                  jobject tab,
                                  jobject content_view_core,
                                  jfloat thumbnail_scale) {
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DCHECK(tab_android);
   int tab_id = tab_android->GetAndroidId();
   GURL url = tab_android->GetURL();
 
   content::ContentViewCore* view =
       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;
     }
 
     TabReadbackCallback readback_done_callback =
         base::Bind(&TabContentManager::PutThumbnailIntoCache,
                    weak_factory_.GetWeakPtr(), tab_id);
     scoped_ptr<TabReadbackRequest> readback_request =
         make_scoped_ptr(new TabReadbackRequest(
             content_view_core, thumbnail_scale, readback_done_callback));
     pending_tab_readbacks_.set(tab_id, readback_request.Pass());
     pending_tab_readbacks_.get(tab_id)->Run();
   }
 }
 
 void TabContentManager::CacheTabWithBitmap(JNIEnv* env,
                                            jobject obj,
                                            jobject tab,
                                            jobject bitmap,
                                            jfloat thumbnail_scale) {
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DCHECK(tab_android);
   int tab_id = tab_android->GetAndroidId();
   GURL url = tab_android->GetURL();
 
   gfx::JavaBitmap java_bitmap_lock(bitmap);
   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);
 }
 
 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)));
 }
 
 void TabContentManager::UpdateVisibleIds(JNIEnv* env,
                                          jobject obj,
                                          jintArray priority) {
   std::list<int> priority_ids;
   jsize length = env->GetArrayLength(priority);
   jint* ints = env->GetIntArrayElements(priority, NULL);
   for (jsize i = 0; i < length; ++i)
     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,
                                            jobject obj,
                                            jint tab_id) {
   TabReadbackRequestMap::iterator readback_iter =
       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,
                                                  jobject obj,
                                                  jint tab_id) {
   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) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_TabContentManager_notifyListenersOfThumbnailChange(
       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) {
   TabReadbackRequestMap::iterator readback_iter =
       pending_tab_readbacks_.find(tab_id);
 
   if (readback_iter != pending_tab_readbacks_.end())
     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) {
   return RegisterNativesImpl(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);
 }
 
 }  // namespace android
 }  // namespace chrome