Bug 123763 – FrameView destructor is worried about being retained by a renderer.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

// FIXME: How could this ever happen when RenderWidget retains the Widget!?

Indeed.

[Andreas Kling]

Created attachment 215965 [details]
Patch

[Darin Adler]

Comment on attachment 215965 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215965&action=review

> Source/WebCore/ChangeLog:8
> +        There's no way we can be in ~FrameView() while also being owned by
> +        a RenderWidget. Remove some overly paranoid code that was making sure
> +        the renderer didn't have a reference on us.

Could you have put in an assertion or is this so obviously true that an assertion would be a waste?

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 215965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215965&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        There's no way we can be in ~FrameView() while also being owned by
> > +        a RenderWidget. Remove some overly paranoid code that was making sure
> > +        the renderer didn't have a reference on us.
> 
> Could you have put in an assertion or is this so obviously true that an assertion would be a waste?

I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT?

[Darin Adler]

(In reply to comment #3)
> I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT?

Yes, and no. Paranoid, yes. But catching overrelease in the destructor might be great instead of having to debug use of the deallocated memory later. I wish every class could have a check like this one. If this would help us debug a case where we someone does an extra deref, then it would be great.

Not really sure how to judge.

[WebKit Commit Bot]

Comment on attachment 215965 [details]
Patch

Clearing flags on attachment: 215965

Committed r158625: <http://trac.webkit.org/changeset/158625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

(In reply to comment #4)
> (In reply to comment #3)
> > I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT?
> 
> Yes, and no. Paranoid, yes. But catching overrelease in the destructor might be great instead of having to debug use of the deallocated memory later. I wish every class could have a check like this one. If this would help us debug a case where we someone does an extra deref, then it would be great.

Doesn't RefCounted already do something like this with the deletion-has-begun assertions?

[Darin Adler]

(In reply to comment #7)
> Doesn't RefCounted already do something like this with the deletion-has-begun assertions?

Maybe it does!