Reland fix for thumbnail generation.

chrome/browser/thumbnails/thumbnail_tab_helper.cc

 // Copyright (c) 2012 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/thumbnails/thumbnail_tab_helper.h"
 
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/thumbnails/thumbnail_service.h"
 #include "chrome/browser/thumbnails/thumbnail_service_factory.h"
@@ skipping 36 matching lines @@
 using content::RenderViewHost;
 using content::RenderWidgetHost;
 using content::WebContents;
 
 using thumbnails::ClipResult;
 using thumbnails::ThumbnailingContext;
 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;
   if (!view->IsSurfaceAvailableForCopy())
     return;
 
   gfx::Rect copy_rect = gfx::Rect(view->GetViewBounds().size());
   // Clip the pixels that will commonly hold a scrollbar, which looks bad in
   // thumbnails.
   int scrollbar_size = gfx::scrollbar_size();
   gfx::Size copy_size;
   copy_rect.Inset(0, 0, scrollbar_size, scrollbar_size);
 
   if (copy_rect.IsEmpty())
     return;
 
   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);
 }
 
 }  // namespace
 
 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
   // aren't views like select popups.
   registrar_.Add(this,
                  content::NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED,
                  content::Source<WebContents>(contents));
 }
 
 ThumbnailTabHelper::~ThumbnailTabHelper() {
@@ skipping 26 matching lines @@
     registrar_.Remove(
         this, content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
         content::Source<RenderWidgetHost>(render_view_host->GetWidget()));
   }
 }
 
 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.
   load_interrupted_ = true;
 }
 
 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.
   if (!web_contents || web_contents->IsBeingDestroyed())
     return;
   // Skip if a pending entry exists. WidgetHidden can be called while navigating
   // pages and this is not a time when thumbnails should be generated.
   if (web_contents->GetController().GetPendingEntry())
     return;
   const GURL& url = web_contents->GetURL();
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
 
   scoped_refptr<thumbnails::ThumbnailService> thumbnail_service =
       ThumbnailServiceFactory::GetForProfile(profile);
 
   // Skip if we don't need to update the thumbnail.
   if (thumbnail_service.get() == NULL ||
       !thumbnail_service->ShouldAcquirePageThumbnail(url)) {
     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(
     content::RenderViewHost* renderer) {
   // NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED is really a new
   // RenderView, not RenderViewHost, and there is no good way to get
   // notifications of RenderViewHosts. So just be tolerant of re-registrations.
   bool registered = registrar_.IsRegistered(
       this, content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
       content::Source<RenderWidgetHost>(renderer->GetWidget()));
   if (!registered) {
     registrar_.Add(this, content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
                    content::Source<RenderWidgetHost>(renderer->GetWidget()));
   }
 }
 
 void ThumbnailTabHelper::WidgetHidden(RenderWidgetHost* widget) {
   UpdateThumbnailIfNecessary(web_contents());
 }