Bug 177603 – Use smart pointers for creating, adding and removing renderers

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

[Antti Koivisto]

RenderPtr everywhere.

[Antti Koivisto]

Created attachment 322080 [details]
patch

[Antti Koivisto]

Created attachment 322083 [details]
patch

[Brent Fulgham]

Yes, please! :-)

[zalan]

Comment on attachment 322083 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322083&action=review

> Source/WebCore/rendering/RenderBlock.cpp:815
> +    auto toBeDeleted = parent.removeChildInternal(child, child.hasLayer() ? NotifyChildren : DontNotifyChildren);

I am a bit worried about the potential UAF errors this introduces. (takeInternal might make it less error prone)

...
RenderObject* nextSibling = child.nextSibling();
...
// We need to scope here because of reasons.
{
    reasonForScoping();
    ...
    parent.removeChildInternal(child);
    ...
}
child.deleteLines();

> Source/WebCore/rendering/RenderGrid.cpp:71
> +    auto& child = baseAddChild<RenderBlock>(WTFMove(newChild), beforeChild);

I wish there was a better way to write this. I am sure we'll see problems like the moved newChild used later accidentally (I know this is a general problem, the only reason I am picking on this is because newChild is an argument). I've run into problems like this in the past.

[Antti Koivisto]

> I am a bit worried about the potential UAF errors this introduces.
> (takeInternal might make it less error prone)

One thing we can do is to use an annotation to make compiler warns on unused return value.

[zalan]

(In reply to Antti Koivisto from comment #5)
> > I am a bit worried about the potential UAF errors this introduces.
> > (takeInternal might make it less error prone)
> 
> One thing we can do is to use an annotation to make compiler warns on unused
> return value.
Yeah, that's a good idea.

[Antti Koivisto]

Created attachment 322181 [details]
patch

[Antti Koivisto]

Created attachment 322182 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/34742406>

[WebKit Commit Bot]

Comment on attachment 322182 [details]
patch

Rejecting attachment 322182 [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-01', 'apply-attachment', '--no-update', '--non-interactive', 322182, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
eePosition.h
patching file Source/WebCore/style/RenderTreeUpdater.cpp
patching file Source/WebCore/style/RenderTreeUpdaterFirstLetter.cpp
patching file Source/WebCore/style/RenderTreeUpdaterGeneratedContent.cpp
patching file Source/WebCore/style/RenderTreeUpdaterListItem.cpp
patching file Source/WebCore/style/RenderTreeUpdaterMultiColumn.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Zalan Bujtas']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4699618

[Antti Koivisto]

Created attachment 322283 [details]
patch

[WebKit Commit Bot]

Comment on attachment 322283 [details]
patch

Clearing flags on attachment: 322283

Committed r222679: <http://trac.webkit.org/changeset/222679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.