Reland fix for thumbnail generation.

chrome/browser/thumbnails/thumbnail_tab_helper.cc

Index: chrome/browser/thumbnails/thumbnail_tab_helper.cc
diff --git a/chrome/browser/thumbnails/thumbnail_tab_helper.cc b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
index b9a57b31e1b03a4c2703e47a397c071c55491afc..d9563a011273760fc1f355f7bbf739e30d000b50 100644
--- a/chrome/browser/thumbnails/thumbnail_tab_helper.cc
+++ b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
@@ -54,35 +54,56 @@ using thumbnails::ThumbnailingAlgorithm;
 
 namespace {
 
+// Balance the call to IncrementCapturerCount() from
+// UpdateThumbnailIfNecessary(), and inform the tab helper that thumbnail
+// generation is complete.
+void DecrementCapturerCount(const ThumbnailingContext& context) {

[Lei Zhang, 2015/11/20 20:01:04]

If you make these ThumbnailTabHelper methods, and assuming we work out the fact
that ThumbnailingContext can indeed be not refcounted, then ThumbnailTabHelper
can take ownership of the context, and we may not need to hand the WeakPtr to
the context.

Then in ThumbnailTabHelper::AsyncProcessThumbnail(), it can
Bind(&ThumbnailTabHelper::ProcessCapturedBitmap, weak_factory_.GetWeakPtr(),
algorithm);

[Lei Zhang, 2016/01/07 22:15:29]

I just reiterated this idea in person. :)

[shrike, 2016/01/08 18:32:36]

I think I found a way around having to hand the thumbnail helper to the context,
so that will get rid of the weakptr there.
After digging into this, the big complication is threads. WeakPtr says the it
can only be accessed from the thread that created it. AsyncProcessThumbnail()
calls render_widget_host->CopyFromBackingStore() with the Bind() runnable, and
there's a check for the UI thread on the other side in ProcessCapturedBitmap(),
so you could maybe assume everything in between runs in the UI thread (maybe).
However, ProcessCapturedBitmap() passes the context off to the thumbnail
algorithm - I assume the WeakPtr and all it contains must be accessed from the
creation thread, so the context, being tightly owned by the thumbnail helper,
would have to be accessed from the UI thread as well. There are no guarantees
about which thread is doing the work within the thumbnail algorithm.
 

+  if (context.web_contents()) {
+    context.web_contents()->DecrementCapturerCount();
+  }
+  // When ProcessCapturedBitmap receives a failed response, and that failure
+  // is due to shutdown, the current thread is not the UI thread. However the
+  // tab helper weakptr can only be dereferenced from the UI thread.
+  if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
+    return;
+  }
+  ThumbnailTabHelper* tab_helper = context.source_tab_helper().get();
+  if (tab_helper) {
+    tab_helper->set_thumbnail_generation_in_progress(false);
+  }
+}
+
 // Feed the constructed thumbnail to the thumbnail service.
 void UpdateThumbnail(const ThumbnailingContext& context,
                      const SkBitmap& thumbnail) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   gfx::Image image = gfx::Image::CreateFrom1xBitmap(thumbnail);
-  context.service->SetPageThumbnail(context, image);
-  DVLOG(1) << "Thumbnail taken for " << context.url << ": "
-           << context.score.ToString();
+  context.service()->SetPageThumbnail(context, image);
+  DVLOG(1) << "Thumbnail taken for " << context.GetURL() << ": "
+           << context.score().ToString();
+
+  DecrementCapturerCount(context);
 }
 
 void ProcessCapturedBitmap(scoped_refptr<ThumbnailingContext> context,
                            scoped_refptr<ThumbnailingAlgorithm> algorithm,
                            const SkBitmap& bitmap,
                            content::ReadbackResponse response) {
-  if (response != content::READBACK_SUCCESS)
-    return;
-
-  // On success, we must be on the UI thread (on failure because of shutdown we
-  // are not on the UI thread).
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  algorithm->ProcessBitmap(context, base::Bind(&UpdateThumbnail), bitmap);
+  if (context->web_contents() && response == content::READBACK_SUCCESS) {
+    // On success, we must be on the UI thread (on failure because of shutdown
+    // we are not on the UI thread).
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    algorithm->ProcessBitmap(context, base::Bind(&UpdateThumbnail), bitmap);
+  } else {
+    DecrementCapturerCount(*context);
+  }
 }
 
-void AsyncProcessThumbnail(content::WebContents* web_contents,
-                           scoped_refptr<ThumbnailingContext> context,
+void AsyncProcessThumbnail(scoped_refptr<ThumbnailingContext> context,
                            scoped_refptr<ThumbnailingAlgorithm> algorithm) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   RenderWidgetHost* render_widget_host =
-      web_contents->GetRenderViewHost()->GetWidget();
+      context->web_contents()->GetRenderViewHost()->GetWidget();
   content::RenderWidgetHostView* view = render_widget_host->GetView();
   if (!view)
     return;
@@ -102,14 +123,16 @@ void AsyncProcessThumbnail(content::WebContents* web_contents,
   ui::ScaleFactor scale_factor =
       ui::GetSupportedScaleFactor(
           ui::GetScaleFactorForNativeView(view->GetNativeView()));
-  context->clip_result = algorithm->GetCanvasCopyInfo(
+  gfx::Size requested_copy_size;
+  context->set_clip_result(algorithm->GetCanvasCopyInfo(
       copy_rect.size(),
       scale_factor,
       &copy_rect,
-      &context->requested_copy_size);
+      &requested_copy_size));
+  context->set_requested_copy_size(requested_copy_size);
   render_widget_host->CopyFromBackingStore(
       copy_rect,
-      context->requested_copy_size,
+      requested_copy_size,
       base::Bind(&ProcessCapturedBitmap, context, algorithm),
       kN32_SkColorType);
 }
@@ -118,7 +141,9 @@ void AsyncProcessThumbnail(content::WebContents* web_contents,
 
 ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents)
     : content::WebContentsObserver(contents),
-      load_interrupted_(false) {
+      load_interrupted_(false),
+      thumbnail_generation_in_progress_(false),
+      weak_factory_(this) {
   // Even though we deal in RenderWidgetHosts, we only care about its
   // subclass, RenderViewHost when it is in a tab. We don't make thumbnails
   // for RenderViewHosts that aren't in tabs, or RenderWidgetHosts that
@@ -165,6 +190,10 @@ void ThumbnailTabHelper::DidStartLoading() {
   load_interrupted_ = false;
 }
 
+void ThumbnailTabHelper::set_thumbnail_generation_in_progress(bool flag) {

[Lei Zhang, 2015/11/20 20:01:04]

Since we only call this with flag = false, make it
reset_thumbnail_generation_in_progress() ?

[shrike, 2016/01/08 18:32:35]

This method will go away in my revised patchset.

+  thumbnail_generation_in_progress_ = flag;
+}
+
 void ThumbnailTabHelper::NavigationStopped() {
   // This function gets called when the page loading is interrupted by the
   // stop button.
@@ -173,6 +202,11 @@ void ThumbnailTabHelper::NavigationStopped() {
 
 void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     WebContents* web_contents) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Ignore thumbnail update requests if one is already being generated.
+  if (thumbnail_generation_in_progress_) {
+    return;
+  }
   // Destroying a WebContents may trigger it to be hidden, prompting a snapshot
   // which would be unwise to attempt <http://crbug.com/130097>. If the
   // WebContents is in the middle of destruction, do not risk it.
@@ -195,12 +229,20 @@ void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     return;
   }
 
+  web_contents->IncrementCapturerCount(gfx::Size());
+  // It turns out that the corresponding DecrementCapturerCount() can trigger a
+  // call to content::WebContentsImpl::WasHidden(), which eventually calls
+  // ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the thumbnail
+  // generation process all over again. Break this cycle by noting that we're
+  // currently generating a thumbnail.
+  thumbnail_generation_in_progress_ = true;
   scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm(
       thumbnail_service->GetThumbnailingAlgorithm());
 
   scoped_refptr<ThumbnailingContext> context(new ThumbnailingContext(
-      web_contents, thumbnail_service.get(), load_interrupted_));
-  AsyncProcessThumbnail(web_contents, context, algorithm);
+      web_contents, thumbnail_service.get(), weak_factory_.GetWeakPtr(),
+      load_interrupted_));
+  AsyncProcessThumbnail(context, algorithm);
 }
 
 void ThumbnailTabHelper::RenderViewHostCreated(