Summary: | FloatingObjects/FloatingObject classes should hold weak references to renderers | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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-12-09 15:55:51 PST
This is in relation to rdar://problem/35953017 Created attachment 328920 [details]
Patch
Created attachment 328925 [details]
Patch
Comment on attachment 328925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328925&action=review > Source/WebCore/ChangeLog:3 > + FloatingObjects/FloatingObject classes should not hold references to renderers A weak reference is still a reference. This should probably be retitled. > Source/WebCore/rendering/FloatingObjects.h:50 > - RenderBox& renderer() const { return m_renderer; } > + RenderBox* renderer() const { return m_renderer.get(); } I think you should still be returning a reference as this is semantically still never supposed to return a null. Call sites don't test for a null renderer either. You'll get the same (safe) nullptr dereference crash either way. (In reply to Antti Koivisto from comment #5) > Comment on attachment 328925 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328925&action=review > > > Source/WebCore/ChangeLog:3 > > + FloatingObjects/FloatingObject classes should not hold references to renderers > > A weak reference is still a reference. This should probably be retitled. > > > Source/WebCore/rendering/FloatingObjects.h:50 > > - RenderBox& renderer() const { return m_renderer; } > > + RenderBox* renderer() const { return m_renderer.get(); } > > I think you should still be returning a reference as this is semantically > still never supposed to return a null. Call sites don't test for a null > renderer either. You'll get the same (safe) nullptr dereference crash either > way. With this patch, now they all do. However I think it's okay to go back to the original RenderBox& renderer() as long as it's not stability critical. I'll make that patch and leave this here just in case. Created attachment 328988 [details]
Patch
Comment on attachment 328988 [details] Patch Rejecting attachment 328988 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 328988, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/5616414 Created attachment 328989 [details]
Patch
Comment on attachment 328989 [details] Patch Clearing flags on attachment: 328989 Committed r225748: <https://trac.webkit.org/changeset/225748> All reviewed patches have been landed. Closing bug. Comment on attachment 328989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328989&action=review > Source/WebCore/rendering/FloatingObjects.cpp:128 > + ComputeFloatOffsetAdapter(RenderBlockFlow& renderer, LayoutUnit lineTop, LayoutUnit lineBottom, LayoutUnit offset) > + : m_renderer(makeWeakPtr(renderer)) It would be nice to keep the const. Is the problem that WeakPtr<const RenderBlockFlow> doesn't work? (In reply to Antti Koivisto from comment #12) > Comment on attachment 328989 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328989&action=review > > > Source/WebCore/rendering/FloatingObjects.cpp:128 > > + ComputeFloatOffsetAdapter(RenderBlockFlow& renderer, LayoutUnit lineTop, LayoutUnit lineBottom, LayoutUnit offset) > > + : m_renderer(makeWeakPtr(renderer)) > > It would be nice to keep the const. Is the problem that > > WeakPtr<const RenderBlockFlow> > > doesn't work? Yeah. |