[webkit-reviews] review canceled: [Bug 74976] Element still flowed below parent after changing from block to inline-block : [Attachment 147100] Patch

Tue Jun 12 15:41:02 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has canceled Abhishek Arya
<inferno at chromium.org>'s request for review:
Bug 74976: Element still flowed below parent after changing from block to
inline-block
https://bugs.webkit.org/show_bug.cgi?id=74976

Attachment 147100: Patch
https://bugs.webkit.org/attachment.cgi?id=147100&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147100&action=review

> Source/WebCore/rendering/RenderBlock.cpp:1241
> +	       RenderObject* parentRenderer = parent();

This could be removed. parent() is inlined and we don't expect it to change
during the loop. I don't mind keeping it but in this case, |parentRenderer|
should be const.

> Source/WebCore/rendering/RenderBlock.cpp:1243
> +	       while (curr) {

I really prefer |for| loops, especially for very simple case like this one as
it makes it obvious what is going on. It seems you disagree with me on that.

> Source/WebCore/rendering/RenderBlock.cpp:1254
> +		  if (curr->isRenderInline())
> +		      toRenderInline(curr)->setContinuation(nextContinuation);
> +		  else if (curr->isRenderBlock())
> +		      toRenderBlock(curr)->setContinuation(nextContinuation);
> +		  else
> +		      ASSERT_NOT_REACHED();

A friend declaration in RenderBoxModelObject for this function looks better to
me than this. As you pointed out, we don't want to expose setContinuation() to
everyone to prevent misuse.

> Source/WebCore/rendering/RenderBlock.cpp:1257
> +	       destroy();

> I had checked all the callers of RenderBlock::removeChild, including Ruby
ones. They don't seem to use |this| afterwards. Anyway, ClusterFuzz will tell
us if we cause some UAF here. Also, i don't see a way for delay cleanup later.

I don't agree that a clusterFuzz based approach is a good idea. It's definitely
a cool tool but it doesn't make something dangerous less dangerous or turn bad
practices into good ones. Here a huge issue is that someone comes and changes
RenderRubyRun then we have a vulnerability. Also |removeChild| is called by
some other functions, I bet you haven't reviewed all the callers to see if
|this| was touched.

There are only 2 places where we call destroy() on |this|:
RenderObject::destroyAndCleanupAnonymousWrappers (fine as it was designed
carefully around destroy()) and RenderRubyRun::removeChild (...). This makes me
wonder if we couldn't get a scaffolding to remove those at a safer time (maybe
resurrecting the idea of pre-layout hooks / phases)

> Source/WebCore/rendering/RenderObject.h:195
> +    RenderObject* previousInPreOrder(const RenderObject* stayWithin = 0)
const;

Please split this in 2 functions, like it is done for nextInPreOrder. Most
callers don't care about staying inside a subtree and will pay an unneeded cost
for that.

> LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:2
> +<html style="font-family: ahem; font-size: 50px; -webkit-font-smoothing:
none;">

Why do we need to use Ahem here? Ref-tests are pretty immune to font family
usually.

> LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:4
> +<!-- WebKit Bug 88022 - Cleanup empty anonymous block continuation -->
> +<!-- Orange and purple boxes should be on the same line -->

If we don't need Ahem, this should be dumped too.