Avoid generating thumbnails while WebContents is destructed.

Avoid generating thumbnails while WebContents is destructed.

This reverts r139939 and fixes it again.

BUG=130097, 107201
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154318

[Avi (use Gerrit), 2012-08-30 15:10:53]

mazda: Please review; would this work?
jam: Is it appropriate to promote IsBeingDestroyed to be more generally used, as
we have it here?

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

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
chrome/browser/tab_contents/thumbnail_generator.cc:489: if
(web_contents->IsBeingDestroyed())
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;

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (left):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:215: //
chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED can also cause the thumbnailer
I was forgetting that we need to deal with this case. We want to avoid
generating thumbnails here, too. WebContents::IsBeingDestroyed cannot be used
for this case because it's before entering its dtor.
I wrote an idea to deal with this case in another comment.

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

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
chrome/browser/tab_contents/thumbnail_generator.cc:489: if
(web_contents->IsBeingDestroyed())
On 2012/08/30 17:19:18, mazda wrote:
> How about the follwing code to deal with the case in another comment?

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.

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (left):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:215: //
chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED can also cause the thumbnailer
Note that NOTIFICATION_TAB_CONTENTS_DESTROYED is going away when I kill
TabContents.

[jam, 2012-08-30 18:53:48]

On 2012/08/30 15:10:53, Avi wrote:
> mazda: Please review; would this work?
> jam: Is it appropriate to promote IsBeingDestroyed to be more generally used,
as
> we have it here?

I'm not that familiar with that function. given that it's already used, I think
another usage of it is ok.

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

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
chrome/browser/tab_contents/thumbnail_generator.cc:489: if
(web_contents->IsBeingDestroyed())
On 2012/08/30 18:16:26, Avi wrote:
> On 2012/08/30 17:19:18, mazda wrote:
> > How about the follwing code to deal with the case in another comment?
> 
> 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.

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?

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (left):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:215: //
chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED can also cause the thumbnailer
On 2012/08/30 18:16:26, Avi wrote:
> Note that NOTIFICATION_TAB_CONTENTS_DESTROYED is going away when I kill
> TabContents.

That's a great. Then WebContents::IsBeingDestroyed can be used.

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

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

https://chromiumcodereview.appspot.com/10911008/diff/1/chrome/browser/tab_con...
chrome/browser/tab_contents/thumbnail_generator.cc:489: if
(web_contents->IsBeingDestroyed())
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]

lgtm

http://codereview.chromium.org/10911008/diff/1/chrome/browser/tab_contents/th...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/10911008/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.cc:489: if
(web_contents->IsBeingDestroyed())
On 2012/08/30 19:44:16, Avi wrote:
> 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?

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.

[Avi (use Gerrit), 2012-08-30 20:49:52]

Nice!

I'll commit but we'll keep an eye on it so we can revert if things go poorly.

[commit-bot: I haz the power, 2012-08-30 20:52:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10911008/1

[commit-bot: I haz the power, 2012-08-30 23:57:29]

Change committed as 154318