Bug 144601 - RenderWidget::setWidgetGeometry() can end up destroying *this*.
Summary: RenderWidget::setWidgetGeometry() can end up destroying *this*.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-04 15:26 PDT by zalan
Modified: 2015-05-05 09:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.67 KB, patch)
2015-05-04 15:43 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2015-05-04 16:35 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2015-05-04 16:45 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2015-05-04 16:48 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.