Summary: | RenderButton should not hold raw pointers to its direct children. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, commit-queue, koivisto, simon.fraser, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
zalan
2017-10-05 13:00:25 PDT
Created attachment 322888 [details]
Patch
Comment on attachment 322888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review > Source/WebCore/rendering/RenderButton.cpp:83 > // violated. > if (&oldChild == m_inner || !m_inner || oldChild.parent() == this) { > ASSERT(&oldChild == m_inner || !m_inner); The comment above talks about "security problems" that I don't think exist with the recent improvements. Maybe the comment, oldChild.parent() == this test and the assert can go? Comment on attachment 322888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review > Source/WebCore/rendering/RenderButton.cpp:139 > + return { }; Is this as efficient as returning emptyString()? > Source/WebCore/rendering/RenderButton.h:74 > + WeakPtr<RenderBlock> m_inner; Do we actually need to reference this, or can we find it via a tree walk? Comment on attachment 322888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review >> Source/WebCore/rendering/RenderButton.cpp:139 >> + return { }; > > Is this as efficient as returning emptyString()? That returns a null string, not an empty string (and is more efficient). (In reply to Antti Koivisto from comment #3) > Comment on attachment 322888 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322888&action=review > > > Source/WebCore/rendering/RenderButton.cpp:83 > > // violated. > > if (&oldChild == m_inner || !m_inner || oldChild.parent() == this) { > > ASSERT(&oldChild == m_inner || !m_inner); > > The comment above talks about "security problems" that I don't think exist > with the recent improvements. Maybe the comment, oldChild.parent() == this > test and the assert can go? The comment/assert actually attempts to catch cases where the RenderButton's inner subtree gets split up for whatever reason (the original bug was column spanning). So I think this is still valid (though not super useful, I admit). Comment on attachment 322888 [details] Patch Clearing flags on attachment: 322888 Committed r222932: <http://trac.webkit.org/changeset/222932> All reviewed patches have been landed. Closing bug. |