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..cfef84732c27f0a44a13fb19e4cd6cc6f5767566 100644
--- a/chrome/browser/thumbnails/thumbnail_tab_helper.cc
+++ b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
@@ -58,17 +58,29 @@ namespace {
 void UpdateThumbnail(const ThumbnailingContext& context,
                      const SkBitmap& thumbnail) {
   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();
+
+  // Now that we're completely finished with the web contents, balance the
+  // IncrementCapturerCount() from UpdateThumbnailIfNecessary().
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Lei Zhang, 2015/11/18 18:06:43]

This generally should be at the top of the function. The one below on like 87 is
a special case since it has to deal with the UI thread "going away" on shutdown.

[shrike, 2015/11/18 18:20:16]

I actually wasn't sure if it was needed at all (but I was thinking the
DecrementCapturerCount() needs to run from the UI thread). Should I move it to
the top or remove the check completely?

[Lei Zhang, 2015/11/18 18:24:00]

Move it to the top.

+  context.web_contents()->DecrementCapturerCount();
 }
 
 void ProcessCapturedBitmap(scoped_refptr<ThumbnailingContext> context,
                            scoped_refptr<ThumbnailingAlgorithm> algorithm,
                            const SkBitmap& bitmap,
                            content::ReadbackResponse response) {
-  if (response != content::READBACK_SUCCESS)
+  // Was the web contents destroyed before the thumbnail could be generated?
+  if (!context->web_contents())
+    return;
+
+  if (response != content::READBACK_SUCCESS) {
+    // Balance the IncrementCapturerCount() from UpdateThumbnailIfNecessary().
+    context->web_contents()->DecrementCapturerCount();
     return;
+  }
 
   // On success, we must be on the UI thread (on failure because of shutdown we
   // are not on the UI thread).
@@ -77,12 +89,11 @@ void ProcessCapturedBitmap(scoped_refptr<ThumbnailingContext> context,
   algorithm->ProcessBitmap(context, base::Bind(&UpdateThumbnail), bitmap);
 }
 
-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 +113,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);
 }
@@ -195,12 +208,20 @@ void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     return;
   }
 
+  web_contents->IncrementCapturerCount(gfx::Size());
+  // Stop responding to notifications. 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.
+  registrar_.RemoveAll();

[Lei Zhang, 2015/11/18 18:06:43]

Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for future
RenderViews / on future hidden events.

[shrike, 2015/11/18 18:20:16]

It appears to be one per web contents (tab_helpers.cc:205).

[Lei Zhang, 2015/11/18 18:24:00]

That I understand. My question is, will a ThumbnailTabHelper make one snapshot
in its lifetime, or more than one? By clearing the notification registrar, a
ThumbnailTabHelper instance will only make one.

[shrike, 2015/11/18 19:18:06]

Sorry (need more coffee).

My assumption in turning off all notifications at this point was that the user
is switching away from the page, which is when the generator needs to generate a
thumbnail. I was thinking at that point you're done with thumbnail generation,
but I realize that the web contents may not go away completely? and the user
might switch back to its page, which means we'll want another thumbnail
generation attempt if the user switches away again. So I don't think it's a good
idea to disable notifications for good like this.

My goal is to prevent or ignore notifications that result from the call to
DecrementCapturerCount(), so perhaps a better idea is to disable this particular
notification here and to reenable it after the call to IncrementCapturerCount()?

+
   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);
+  AsyncProcessThumbnail(context, algorithm);
 }
 
 void ThumbnailTabHelper::RenderViewHostCreated(