Avoid generating thumbnails while WebContents is destructed.

chrome/browser/tab_contents/thumbnail_generator.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/tab_contents/thumbnail_generator.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/bind.h"
@@ skipping 465 matching lines @@
                       clip_result);
   SkIRect src_rect = { clipping_rect.x(), clipping_rect.y(),
                        clipping_rect.right(), clipping_rect.bottom() };
   SkBitmap clipped_bitmap;
   bitmap.extractSubset(&clipped_bitmap, src_rect);
   return clipped_bitmap;
 }
 
 void ThumbnailGenerator::UpdateThumbnailIfNecessary(
     WebContents* web_contents) {
-  // Skip if a pending entry exists. WidgetHidden can be called while navigaing
-  // pages and this is not a timing when thumbnails should be generated.
+  // 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->IsBeingDestroyed())

[mazda, 2012/08/30 17:19:18]

How about the follwing code to deal with the case in another comment?

TabContents* tab_contents = TabContents::FromWebContents(web_contents);
if (!tab_contents || tab_contents->in_destructor())
  return;

[Avi (use Gerrit), 2012/08/30 18:16:26]

Is that in addition to this code or to replace it?

I'm going to kill TabContents. Adding more code that relies on it is not the
direction I want to go. If we need to add a few more features to WebContents,
that's fine. But we cannot add a dependency to TabContents. I need to update
ThumbnailGenerator so that it relies only on WebContents, and behaves properly
(not hanging, etc) in all cases.

[mazda, 2012/08/30 19:39:22]

I meant to replace it.
But when NOTIFICATION_TAB_CONTENTS_DESTROYED is not sent from the dtor of
TabContents, WebContents::IsBeingDestroyed should be fine.
Can we temporarily add dependency to TabContents, then remove it together with
NOTIFICATION_TAB_CONTENTS_DESTROYED?

[Avi (use Gerrit), 2012/08/30 19:44:16]

Adding a temporary dependency to TabContents would be fine, but I don't
understand what you propose to do here.

Are you worried that TabContents's firing of NOTIFICATION_TAB_CONTENTS_DESTROYED
will trigger some event that will cause a snapshot? What is the case when that
happens? Can we change ThumbnailGenerator to rely only on WebContents yet handle
that case?

[mazda, 2012/08/30 20:39:39]

Yes, http://crbug.com/130097 was caused by NOTIFICATION_TAB_CONTENTS_DESTROYED
as well in the following.

NOTIFICATION_TAB_CONTENTS_DESTROYED -> TabStripModel ->
NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED -> thumbnailer -> crash.

But I tested it now and it seems the behavior has changed.
NOTIFICATION_TAB_CONTENTS_DESTROYED does not end up causing
NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED to be sent any longer.
So I think we can rely on WebContents::IsBeingDestroyed as of now.

+    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());
   history::TopSites* top_sites = profile->GetTopSites();
   // Skip if we don't need to update the thumbnail.
   if (!ShouldUpdateThumbnail(profile, top_sites, url))
     return;
 
   AsyncUpdateThumbnail(web_contents);
 }
 
 void ThumbnailGenerator::UpdateThumbnail(
     WebContents* web_contents, const SkBitmap& thumbnail,
     const ClipResult& clip_result) {
-
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   history::TopSites* top_sites = profile->GetTopSites();
   if (!top_sites)
     return;
 
   // Compute the thumbnail score.
   ThumbnailScore score;
   score.at_top =
       (web_contents->GetRenderViewHost()->GetLastScrollOffset().y() == 0);
@@ skipping 118 matching lines @@
 void ThumbnailGenerator::DidStartLoading(
     content::RenderViewHost* render_view_host) {
   load_interrupted_ = false;
 }
 
 void ThumbnailGenerator::StopNavigation() {
   // This function gets called when the page loading is interrupted by the
   // stop button.
   load_interrupted_ = true;
 }