Bug 144601

Summary: RenderWidget::setWidgetGeometry() can end up destroying *this*.
Product: WebKit Reporter: alan <zalan>
Component: Layout and RenderingAssignee: alan <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

alan
Reported 2015-05-04 15:26:51 PDT
Refcount RenderWidget so that we don't end up with an invalid *this* during layout.
Attachments
Patch (8.67 KB, patch)
2015-05-04 15:43 PDT, alan 		no flags
DetailsFormatted DiffDiff
Patch (8.39 KB, patch)
2015-05-04 16:35 PDT, alan 		no flags
DetailsFormatted DiffDiff
Patch (8.39 KB, patch)
2015-05-04 16:45 PDT, alan 		no flags
DetailsFormatted DiffDiff
Patch (8.39 KB, patch)
2015-05-04 16:48 PDT, alan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
alan
Comment 1 2015-05-04 15:27:29 PDT
rdar://problem/20753994
alan
Comment 2 2015-05-04 15:43:24 PDT
Created attachment 252342 [details] Patch
Andreas Kling
Comment 3 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.
Andreas Kling
Comment 4 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.
alan
Comment 5 2015-05-04 16:35:13 PDT
Created attachment 252350 [details] Patch
Andreas Kling
Comment 6 2015-05-04 16:37:24 PDT
Comment on attachment 252350 [details] Patch r=me
Chris Dumez
Comment 7 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();
alan
Comment 8 2015-05-04 16:45:49 PDT
Created attachment 252351 [details] Patch
alan
Comment 9 2015-05-04 16:48:51 PDT
Created attachment 252353 [details] Patch
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2015-05-04 20:23:59 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.