Bug 180627 - FloatingObjects/FloatingObject classes should hold weak references to renderers
Summary: FloatingObjects/FloatingObject classes should hold weak references to renderers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-09 15:55 PST by zalan
Modified: 2017-12-11 11:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (38.05 KB, patch)
2017-12-09 16:07 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (38.09 KB, patch)
2017-12-09 16:58 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2017-12-11 09:42 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2017-12-11 09:49 PST, 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 2017-12-09 15:55:51 PST
ssia
Comment 1 zalan 2017-12-09 15:56:27 PST
This is in relation to rdar://problem/35953017
Comment 2 zalan 2017-12-09 16:07:12 PST
Created attachment 328920 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2017-12-09 16:55:58 PST
<rdar://problem/35954069>
Comment 4 zalan 2017-12-09 16:58:50 PST
Created attachment 328925 [details]
Patch
Comment 5 Antti Koivisto 2017-12-11 08:34:16 PST
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.
Comment 6 zalan 2017-12-11 08:38:14 PST
(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.
Comment 7 zalan 2017-12-11 09:42:14 PST
Created attachment 328988 [details]
Patch
Comment 8 WebKit Commit Bot 2017-12-11 09:43:29 PST
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
Comment 9 zalan 2017-12-11 09:49:53 PST
Created attachment 328989 [details]
Patch
Comment 10 WebKit Commit Bot 2017-12-11 10:23:52 PST
Comment on attachment 328989 [details]
Patch

Clearing flags on attachment: 328989

Committed r225748: <https://trac.webkit.org/changeset/225748>
Comment 11 WebKit Commit Bot 2017-12-11 10:23:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Antti Koivisto 2017-12-11 10:43:05 PST
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?
Comment 13 zalan 2017-12-11 11:00:30 PST
(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.