Avoid generating thumbnails while WebContents is destructed.

chrome/browser/tab_contents/thumbnail_generator.cc

Index: chrome/browser/tab_contents/thumbnail_generator.cc
diff --git a/chrome/browser/tab_contents/thumbnail_generator.cc b/chrome/browser/tab_contents/thumbnail_generator.cc
index ab0d5f32358714e27951e0479291a16bfb2f6476..09ce31ac785b45f036c4a5c25e822216b662a5bf 100644
--- a/chrome/browser/tab_contents/thumbnail_generator.cc
+++ b/chrome/browser/tab_contents/thumbnail_generator.cc
@@ -483,8 +483,13 @@ SkBitmap ThumbnailGenerator::GetClippedBitmap(const SkBitmap& 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();
@@ -501,7 +506,6 @@ void ThumbnailGenerator::UpdateThumbnailIfNecessary(
 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();