Bug 180627 – FloatingObjects/FloatingObject classes should hold weak references to renderers

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

ssia

[zalan]

This is in relation to rdar://problem/35953017

[zalan]

Created attachment 328920 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35954069>

[zalan]

Created attachment 328925 [details]
Patch

[Antti Koivisto]

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.

[zalan]

(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.

[zalan]

Created attachment 328988 [details]
Patch

[WebKit Commit Bot]

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

[zalan]

Created attachment 328989 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328989 [details]
Patch

Clearing flags on attachment: 328989

Committed r225748: <https://trac.webkit.org/changeset/225748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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?

[zalan]

(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.