[Panels] Fix favicon crash from using deleted WebContents.

[Panels] Fix favicon crash from using deleted WebContents.

Updated TaskManagerPanelResourceProvider to no longer expect Panel to have web contents after web contents are disconnected.

BUG=155133
TEST=updated

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

[sky, 2012-10-11 01:03:53]

http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_host.cc (right):

http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_host.cc:207: web_contents_.reset();
WebContentsDestroyed is invoked from ~WebContents (well, ~WebContentsImpl). That
means if you get here and web_contents_ is valid you're going to attempt to
delete it again.

If this class owns the WebContents wrapped in web_contents_ and its always
deleting through the scopedptr, this code is unecessary. If someone else is
deleting the WebContents out from under you, then you need a
web_contents_.release() here.

[jennb, 2012-10-11 03:53:26]

On 2012/10/11 01:03:53, sky wrote:
>
http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
> File chrome/browser/ui/panels/panel_host.cc (right):
> 
>
http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
> chrome/browser/ui/panels/panel_host.cc:207: web_contents_.reset();
> WebContentsDestroyed is invoked from ~WebContents (well, ~WebContentsImpl).
That
> means if you get here and web_contents_ is valid you're going to attempt to
> delete it again.
> 
> If this class owns the WebContents wrapped in web_contents_ and its always
> deleting through the scopedptr, this code is unecessary. If someone else is
> deleting the WebContents out from under you, then you need a
> web_contents_.release() here.

Thanks. I think there must be something else deleting the WebContents. Will need
to track it down tomorrow. Maybe something in WebView.

[sky, 2012-10-11 16:41:26]

On 2012/10/11 03:53:26, jennb wrote:
> On 2012/10/11 01:03:53, sky wrote:
> >
>
http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
> > File chrome/browser/ui/panels/panel_host.cc (right):
> > 
> >
>
http://codereview.chromium.org/11092066/diff/1/chrome/browser/ui/panels/panel...
> > chrome/browser/ui/panels/panel_host.cc:207: web_contents_.reset();
> > WebContentsDestroyed is invoked from ~WebContents (well, ~WebContentsImpl).
> That
> > means if you get here and web_contents_ is valid you're going to attempt to
> > delete it again.
> > 
> > If this class owns the WebContents wrapped in web_contents_ and its always
> > deleting through the scopedptr, this code is unecessary. If someone else is
> > deleting the WebContents out from under you, then you need a
> > web_contents_.release() here.
> 
> Thanks. I think there must be something else deleting the WebContents. Will
need
> to track it down tomorrow. Maybe something in WebView.

If you get stuck, before deleting the webcontents remove PanelHost as an
observer and add a CHECK to WebContentsDestroyed. That'll definitively tell you
if someone else is deleting the WebContents out from under you.

[jennb, 2012-10-11 21:50:55]

On 2012/10/11 16:41:26, sky wrote:
> 
> If you get stuck, before deleting the webcontents remove PanelHost as an
> observer and add a CHECK to WebContentsDestroyed. That'll definitively tell
you
> if someone else is deleting the WebContents out from under you.

Changed this patch to simply add DCHECKs. Maybe we'll get more info from other
canary crashes as a result.

[sky, 2012-10-11 21:52:20]

DCHECKs are compiled out in release builds.

  -Scott

On Thu, Oct 11, 2012 at 2:50 PM,  <jennb@chromium.org> wrote:
> On 2012/10/11 16:41:26, sky wrote:
>
>> If you get stuck, before deleting the webcontents remove PanelHost as an
>> observer and add a CHECK to WebContentsDestroyed. That'll definitively
>> tell
>
> you
>>
>> if someone else is deleting the WebContents out from under you.
>
>
> Changed this patch to simply add DCHECKs. Maybe we'll get more info from
> other
> canary crashes as a result.
>
>
>
> http://codereview.chromium.org/11092066/

[jennb, 2012-10-13 00:54:54]

sky - Ready for another look. Added trace upload without crash and adjusted
checks.

Jenn

[jennb, 2012-10-13 04:49:53]

I think I accidentally stumbled on the cause of the bug by adding the CHECK in
WebContentsDestroyed, which revealed that web_contents_.get() still returns the
pointer if used mid-destruction.

WebView watches for web contents being destroyed, and in DetachWebContents(),
there's a call to ShowWindow() which could be responsible for the OnPaint() in
the crash stack.

Hoping this is the fix so I removed the trace dump.

[sky, 2012-10-15 22:47:10]

On Fri, Oct 12, 2012 at 9:49 PM,  <jennb@chromium.org> wrote:
> I think I accidentally stumbled on the cause of the bug by adding the CHECK
> in
> WebContentsDestroyed, which revealed that web_contents_.get() still returns
> the
> pointer if used mid-destruction.

This is because destruction hasn't completed.

> WebView watches for web contents being destroyed, and in
> DetachWebContents(),
> there's a call to ShowWindow() which could be responsible for the OnPaint()
> in
> the crash stack.

How would that happen? By that I mean how would that trigger a crash?
FavIconTabHelper is still valid at this point.

  -Scott

> Hoping this is the fix so I removed the trace dump.
>
> http://codereview.chromium.org/11092066/

[jennb, 2012-10-15 23:01:33]

I suspect the crash isn't because FavIconTabHelper is NULL, but rather
something inside GetFavIcon() is using an invalid pointer and it's just not
showing up in the stack trace. E.g. maybe the nav controller is no longer
valid. GetFavIcon() uses that without checking.


On Mon, Oct 15, 2012 at 3:47 PM, Scott Violet <sky@chromium.org> wrote:

> On Fri, Oct 12, 2012 at 9:49 PM,  <jennb@chromium.org> wrote:
> > I think I accidentally stumbled on the cause of the bug by adding the
> CHECK
> > in
> > WebContentsDestroyed, which revealed that web_contents_.get() still
> returns
> > the
> > pointer if used mid-destruction.
>
> This is because destruction hasn't completed.
>
> > WebView watches for web contents being destroyed, and in
> > DetachWebContents(),
> > there's a call to ShowWindow() which could be responsible for the
> OnPaint()
> > in
> > the crash stack.
>
> How would that happen? By that I mean how would that trigger a crash?
> FavIconTabHelper is still valid at this point.
>
>   -Scott
>
> > Hoping this is the fix so I removed the trace dump.
> >
> > http://codereview.chromium.org/11092066/
>

[sky, 2012-10-15 23:06:25]

NavigationController should still be valid too. I don't see
WebContentsDestroyed in the trace of the crash, which indicates it's
after the destructor has run. Assuming that is even what is going on.

  -Scott

On Mon, Oct 15, 2012 at 4:01 PM, Jenn Braithwaite (胡慧鋒)
<jennb@chromium.org> wrote:
> I suspect the crash isn't because FavIconTabHelper is NULL, but rather
> something inside GetFavIcon() is using an invalid pointer and it's just not
> showing up in the stack trace. E.g. maybe the nav controller is no longer
> valid. GetFavIcon() uses that without checking.
>
>
> On Mon, Oct 15, 2012 at 3:47 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Fri, Oct 12, 2012 at 9:49 PM,  <jennb@chromium.org> wrote:
>> > I think I accidentally stumbled on the cause of the bug by adding the
>> > CHECK
>> > in
>> > WebContentsDestroyed, which revealed that web_contents_.get() still
>> > returns
>> > the
>> > pointer if used mid-destruction.
>>
>> This is because destruction hasn't completed.
>>
>> > WebView watches for web contents being destroyed, and in
>> > DetachWebContents(),
>> > there's a call to ShowWindow() which could be responsible for the
>> > OnPaint()
>> > in
>> > the crash stack.
>>
>> How would that happen? By that I mean how would that trigger a crash?
>> FavIconTabHelper is still valid at this point.
>>
>>   -Scott
>>
>> > Hoping this is the fix so I removed the trace dump.
>> >
>> > http://codereview.chromium.org/11092066/
>
>

[jennb, 2012-10-15 23:15:55]

On Mon, Oct 15, 2012 at 4:06 PM, Scott Violet <sky@chromium.org> wrote:

> NavigationController should still be valid too. I don't see
> WebContentsDestroyed in the trace of the crash, which indicates it's
> after the destructor has run. Assuming that is even what is going on.
>
>
I'm also puzzled by the lack of evidence of the web contents being
destroyed (I looked in every thread's stack trace in case OnPaint happens
on a different thread).

Circumstantial evidence makes this seem related to webcontents destruction:
The crashes started happening after the favicon_tab_helper was moved into
WebContentsUserData. Prior, the favicon_tab_helper was the "guard" in
GetPageIcon() and it was destroyed before web_contents_.reset() was
invoked. After the change, which made web_contents_.get() the "guard",
crashes started occurring.

If I'm wrong, this change will not prevent the crash, but the
release-before-delete is still needed to allow me to add a CHECK() in
WebContentsDestroyed() to catch if there is any rogue destruction causing
problems.




>   -Scott
>
> On Mon, Oct 15, 2012 at 4:01 PM, Jenn Braithwaite (胡慧鋒)
> <jennb@chromium.org> wrote:
> > I suspect the crash isn't because FavIconTabHelper is NULL, but rather
> > something inside GetFavIcon() is using an invalid pointer and it's just
> not
> > showing up in the stack trace. E.g. maybe the nav controller is no longer
> > valid. GetFavIcon() uses that without checking.
> >
> >
> > On Mon, Oct 15, 2012 at 3:47 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> On Fri, Oct 12, 2012 at 9:49 PM,  <jennb@chromium.org> wrote:
> >> > I think I accidentally stumbled on the cause of the bug by adding the
> >> > CHECK
> >> > in
> >> > WebContentsDestroyed, which revealed that web_contents_.get() still
> >> > returns
> >> > the
> >> > pointer if used mid-destruction.
> >>
> >> This is because destruction hasn't completed.
> >>
> >> > WebView watches for web contents being destroyed, and in
> >> > DetachWebContents(),
> >> > there's a call to ShowWindow() which could be responsible for the
> >> > OnPaint()
> >> > in
> >> > the crash stack.
> >>
> >> How would that happen? By that I mean how would that trigger a crash?
> >> FavIconTabHelper is still valid at this point.
> >>
> >>   -Scott
> >>
> >> > Hoping this is the fix so I removed the trace dump.
> >> >
> >> > http://codereview.chromium.org/11092066/
> >
> >
>

[sky, 2012-10-15 23:34:17]

http://codereview.chromium.org/11092066/diff/16002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_host.cc (right):

http://codereview.chromium.org/11092066/diff/16002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_host.cc:88: DCHECK(favicon_tab_helper);
Make this a CHECK so we can know for sure if its NULL.

[jennb, 2012-10-16 00:17:07]

http://codereview.chromium.org/11092066/diff/16002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_host.cc (right):

http://codereview.chromium.org/11092066/diff/16002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_host.cc:88: DCHECK(favicon_tab_helper);
On 2012/10/15 23:34:17, sky wrote:
> Make this a CHECK so we can know for sure if its NULL.

Done.

[sky, 2012-10-16 15:39:13]

LGTM

[commit-bot: I haz the power, 2012-10-16 17:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/11092066/18005

[commit-bot: I haz the power, 2012-10-16 19:12:38]

Change committed as 162200