Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.

BUG=530707

Review URL: https://codereview.chromium.org/1461463002

Cr-Commit-Position: refs/heads/master@{#370809}

chrome/browser/thumbnails/thumbnail_tab_helper.cc

@@ -53,73 +53,10 @@ using thumbnails::ClipResult;
 using thumbnails::ThumbnailingContext;
 using thumbnails::ThumbnailingAlgorithm;
 
-namespace {
-
-// Feed the constructed thumbnail to the thumbnail service.
-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();
-}
-
-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);
-}
-
-void AsyncProcessThumbnail(content::WebContents* web_contents,
-                           scoped_refptr<ThumbnailingContext> context,
-                           scoped_refptr<ThumbnailingAlgorithm> algorithm) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  RenderWidgetHost* render_widget_host =
-      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(
-      copy_rect.size(),
-      scale_factor,
-      &copy_rect,
-      &context->requested_copy_size);
-  render_widget_host->CopyFromBackingStore(
-      copy_rect,
-      context->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),
+      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
@@ -172,20 +109,29 @@ void ThumbnailTabHelper::NavigationStopped() {
   load_interrupted_ = true;
 }
 
-void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
-    WebContents* web_contents) {
+void ThumbnailTabHelper::UpdateThumbnailIfNecessary() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Ignore thumbnail update requests if one is already in progress. This can
+  // happen at the end of thumbnail generation when
+  // CleanUpFromThumbnailGeneration() calls DecrementCapturerCount(), triggering
+  // a call to content::WebContentsImpl::WasHidden() which eventually calls
+  // ThumbnailTabHelper::UpdateThumbnailIfNecessary().
+  if (thumbnailing_context_) {
+    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())
+  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())
+  if (web_contents()->GetController().GetPendingEntry())
     return;
-  const GURL& url = web_contents->GetURL();
+  const GURL& url = web_contents()->GetURL();
   Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
 
   scoped_refptr<thumbnails::ThumbnailService> thumbnail_service =
       ThumbnailServiceFactory::GetForProfile(profile);
@@ -196,12 +142,102 @@ void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     return;
   }
 
+  // Prevent the web contents from disappearing before the async thumbnail
+  // generation code executes. See https://crbug.com/530707 .
+  web_contents()->IncrementCapturerCount(gfx::Size());
+
+  AsyncProcessThumbnail(thumbnail_service);
+}
+
+void ThumbnailTabHelper::AsyncProcessThumbnail(
+    scoped_refptr<thumbnails::ThumbnailService> thumbnail_service) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  RenderWidgetHost* render_widget_host =
+      web_contents()->GetRenderViewHost()->GetWidget();
+  content::RenderWidgetHostView* view = render_widget_host->GetView();
+  if (!view || !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;
+  }
+
   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);
+  thumbnailing_context_ = new ThumbnailingContext(web_contents(),
+                                                  thumbnail_service.get(),
+                                                  load_interrupted_);
+
+  ui::ScaleFactor scale_factor =
+      ui::GetSupportedScaleFactor(
+          ui::GetScaleFactorForNativeView(view->GetNativeView()));
+  thumbnailing_context_->clip_result = algorithm->GetCanvasCopyInfo(
+      copy_rect.size(),
+      scale_factor,
+      &copy_rect,
+      &thumbnailing_context_->requested_copy_size);
+  render_widget_host->CopyFromBackingStore(
+      copy_rect,
+      thumbnailing_context_->requested_copy_size,
+      base::Bind(&ThumbnailTabHelper::ProcessCapturedBitmap,
+                 weak_factory_.GetWeakPtr(),
+                 algorithm),
+      kN32_SkColorType);
+}
+
+void ThumbnailTabHelper::ProcessCapturedBitmap(
+    scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm,
+    const SkBitmap& bitmap,
+    content::ReadbackResponse response) {
+  if (response == content::READBACK_SUCCESS) {
+    // On success, we must be on the UI thread.
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    algorithm->ProcessBitmap(thumbnailing_context_,
+                             base::Bind(&ThumbnailTabHelper::UpdateThumbnail,
+                                        weak_factory_.GetWeakPtr()),
+                             bitmap);
+  } else {
+    // On failure because of shutdown we are not on the UI thread, so ensure
+    // that cleanup happens on that thread.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&ThumbnailTabHelper::CleanUpFromThumbnailGeneration,
+                   weak_factory_.GetWeakPtr()));
+  }
+}
+
+void ThumbnailTabHelper::CleanUpFromThumbnailGeneration() {
+  if (web_contents()) {
+    // Balance the call to IncrementCapturerCount() made in
+    // UpdateThumbnailIfNecessary().
+    web_contents()->DecrementCapturerCount();
+  }
+
+  // Make a note that thumbnail generation is complete.
+  thumbnailing_context_ = nullptr;
+}
+
+void ThumbnailTabHelper::UpdateThumbnail(
+    const thumbnails::ThumbnailingContext& context,
+    const SkBitmap& thumbnail) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Feed the constructed thumbnail to the thumbnail service.
+  gfx::Image image = gfx::Image::CreateFrom1xBitmap(thumbnail);
+  context.service->SetPageThumbnail(context, image);
+  DVLOG(1) << "Thumbnail taken for " << context.url << ": "
+      << context.score.ToString();
+
+  CleanUpFromThumbnailGeneration();
 }
 
 void ThumbnailTabHelper::RenderViewHostCreated(
@@ -219,5 +255,5 @@ void ThumbnailTabHelper::RenderViewHostCreated(
 }
 
 void ThumbnailTabHelper::WidgetHidden(RenderWidgetHost* widget) {
-  UpdateThumbnailIfNecessary(web_contents());
+  UpdateThumbnailIfNecessary();
 }

chrome/browser/thumbnails/thumbnail_tab_helper.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_THUMBNAILS_THUMBNAIL_TAB_HELPER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/thumbnails/thumbnailing_context.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -40,7 +41,25 @@ class ThumbnailTabHelper
   void NavigationStopped() override;
 
   // Update the thumbnail of the given tab contents if necessary.
-  void UpdateThumbnailIfNecessary(content::WebContents* web_contents);
+  void UpdateThumbnailIfNecessary();
+
+  // Initiate asynchronous generation of a thumbnail from the web contents.
+  void AsyncProcessThumbnail(
+      scoped_refptr<thumbnails::ThumbnailService> thumbnail_service);
+
+  // Create a thumbnail from the web contents bitmap.
+  void ProcessCapturedBitmap(
+      scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm,
+      const SkBitmap& bitmap,
+      content::ReadbackResponse response);
+
+  // Pass the thumbnail to the thumbnail service.
+  void UpdateThumbnail(
+      const thumbnails::ThumbnailingContext& context,
+      const SkBitmap& thumbnail);
+
+  // Clean up after thumbnail generation has ended.
+  void CleanUpFromThumbnailGeneration();
 
   // Called when a render view host was created for a WebContents.
   void RenderViewHostCreated(content::RenderViewHost* renderer);
@@ -49,9 +68,12 @@ class ThumbnailTabHelper
   void WidgetHidden(content::RenderWidgetHost* widget);
 
   content::NotificationRegistrar registrar_;
+  scoped_refptr<thumbnails::ThumbnailingContext> thumbnailing_context_;
 
   bool load_interrupted_;
 
+  base::WeakPtrFactory<ThumbnailTabHelper> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ThumbnailTabHelper);
 };
 

chrome/browser/ui/exclusive_access/fullscreen_controller.cc

@@ -93,13 +93,14 @@ bool FullscreenController::IsUserAcceptedFullscreen() const {
 
 bool FullscreenController::IsFullscreenForTabOrPending(
     const WebContents* web_contents) const {
+  if (IsFullscreenForCapturedTab(web_contents))
+    return true;
   if (web_contents == exclusive_access_tab()) {
     DCHECK(web_contents ==
            exclusive_access_manager()->context()->GetActiveWebContents());
-    DCHECK(web_contents->GetCapturerCount() == 0);
     return true;
   }
-  return IsFullscreenForCapturedTab(web_contents);
+  return false;
 }
 
 bool FullscreenController::IsFullscreenCausedByTab() const {

content/browser/compositor/delegated_frame_host.cc

@@ -410,7 +410,7 @@ void DelegatedFrameHost::SwapDelegatedFrame(
     }
     last_output_surface_id_ = output_surface_id;
   }
-  bool immediate_ack = !compositor_;
+  bool skip_frame = false;
   pending_delegated_ack_count_++;
 
   if (frame_size.IsEmpty()) {
@@ -447,10 +447,10 @@ void DelegatedFrameHost::SwapDelegatedFrame(
 
       gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
       if (desired_size != frame_size_in_dip && !desired_size.IsEmpty())
-        immediate_ack = true;
+        skip_frame = true;
 
       cc::SurfaceFactory::DrawCallback ack_callback;
-      if (compositor_ && !immediate_ack) {
+      if (compositor_ && !skip_frame) {
         ack_callback = base::Bind(&DelegatedFrameHost::SurfaceDrawn,
                                   AsWeakPtr(), output_surface_id);
       }
@@ -486,7 +486,9 @@ void DelegatedFrameHost::SwapDelegatedFrame(
     client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
         damage_rect_in_dip);
 
-  if (immediate_ack) {
+  // Note that |compositor_| may be reset by SetShowSurface or
+  // SetShowDelegatedContent above.
+  if (!compositor_ || skip_frame) {
     SendDelegatedFrameAck(output_surface_id);
   } else if (!use_surfaces_) {
     std::vector<ui::LatencyInfo>::const_iterator it;

content/browser/renderer_host/render_widget_host_view_mac.mm

@@ -892,7 +892,7 @@ void RenderWidgetHostViewMac::WasOccluded() {
   // occur in this specific order. However, because thumbnail generation is
   // asychronous, that operation won't run before SuspendBrowserCompositorView()
   // completes. As a result you won't get a thumbnail for the page unless you
-  // happen to switch back to it. See http://crbug.com/530707 .
+  // execute these two statements in this specific order.
   render_widget_host_->WasHidden();
   SuspendBrowserCompositorView();
 }