Bug 144601

Summary: RenderWidget::setWidgetGeometry() can end up destroying *this*.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, kling, kondapallykalyan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description zalan 2015-05-04 15:26:51 PDT 
Refcount RenderWidget so that we don't end up with an invalid *this* during layout.
Comment 1 zalan 2015-05-04 15:27:29 PDT 
rdar://problem/20753994
Comment 2 zalan 2015-05-04 15:43:24 PDT 
Created attachment 252342 [details]
Patch
Comment 3 Andreas Kling 2015-05-04 16:24:47 PDT 
Comment on attachment 252342 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:361
> +    releaseProtectedRenderWidgets();

Let's move this to FrameView instead so it also works for subtree layouts.

> Source/WebCore/rendering/RenderWidget.h:77
> +    inline void ref() { ++m_refCount; }

No need to specify "inline" here.
Comment 4 Andreas Kling 2015-05-04 16:24:47 PDT 
Comment on attachment 252342 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:361
> +    releaseProtectedRenderWidgets();

Let's move this to FrameView instead so it also works for subtree layouts.

> Source/WebCore/rendering/RenderWidget.h:77
> +    inline void ref() { ++m_refCount; }

No need to specify "inline" here.
Comment 5 zalan 2015-05-04 16:35:13 PDT 
Created attachment 252350 [details]
Patch
Comment 6 Andreas Kling 2015-05-04 16:37:24 PDT 
Comment on attachment 252350 [details]
Patch

r=me
Comment 7 Chris Dumez 2015-05-04 16:37:48 PDT 
Comment on attachment 252350 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:2028
> +    if (is<RenderWidget>(this)) {

if (is<RenderWidget>(*this)) to avoid unnecessary null check

> Source/WebCore/rendering/RenderObject.cpp:2029
> +        downcast<RenderWidget>(this)->deref();

We usually do: downcast<RenderWidget>(*this).deref();
Comment 8 zalan 2015-05-04 16:45:49 PDT 
Created attachment 252351 [details]
Patch
Comment 9 zalan 2015-05-04 16:48:51 PDT 
Created attachment 252353 [details]
Patch
Comment 10 WebKit Commit Bot 2015-05-04 20:23:56 PDT 
Comment on attachment 252353 [details]
Patch

Clearing flags on attachment: 252353

Committed r183788: <http://trac.webkit.org/changeset/183788>
Comment 11 WebKit Commit Bot 2015-05-04 20:23:59 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2015-05-05 09:09:08 PDT 
I’m disappointed that we have to add back the reference counting here. And doubly disappointed that we are not using the RefCounted template for the reference counting.