Bug 234018 – nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded

• Status: REOPENED
• Component: Layout and Rendering

[Gabriel Nava Marino]

We are accessing a renderer for a floatingObject that has already been destroyed because the floatingObject was never cleaned up from the current block.

[Gabriel Nava Marino]

RenderTree structure from attached test case with proposed patch. Notice the two floating objects:
Content-Type: text/plain
layer at (0,0) size 816x585
  RenderView at (0,0) size 800x585
layer at (0,0) size 816x16
  RenderBlock (floating) {HTML} at (0,0) size 816x16
    RenderBlock (anonymous) at (0,0) size 816x0
      RenderInline {SPAN} at (0,0) size 0x0
      RenderBody {BODY} at (8,8) size 800x0
        RenderBlock {DIV} at (0,0) size 800x0
        RenderBlock (anonymous) at (0,0) size 800x0
          RenderInline {SPAN} at (0,0) size 0x0
          RenderBlock (floating) {HTML} at (0,0) size 800x0
            RenderInline {Q} at (0,0) size 14x18
              RenderInline (generated) at (0,0) size 8x18
                RenderQuote at (0,0) size 8x18
                  RenderText at (150,0) size 8x18
                    text run at (150,0) width 8: "\""
              RenderInline (generated) at (0,0) size 7x18
                RenderQuote at (0,0) size 7x18
                  RenderText at (157,0) size 7x18
                    text run at (157,0) width 7: "\""
            RenderTextControl {INPUT} at (2,2) size 147x19 [bgcolor=#FFFFFF] [border: (2px inset #000000)]
    RenderBlock {DIV} at (0,0) size 816x0
layer at (13,13) size 141x13
  RenderBlock {DIV} at (3,3) size 141x13
layer at (8,31) size 800x400
  RenderVideo {VIDEO} at (0,23) size 800x400
#EOF
#EOF
#EOF

[Gabriel Nava Marino]

Created attachment 446394 [details]
Patch

[Gabriel Nava Marino]

<rdar://problem/85503666>

[zalan]

Comment on attachment 446394 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        or be able to shrink to avoid floats. However, we can have a scenario where the current child block
> +        doesnât have a float, but one of its descendants does. In this case, although we should continue to

and we don't propagate this "contains float" state to the ancestors when we insert the float to a descendant block? I wonder how this works during layout when we need to discover these floats.

[Gabriel Nava Marino]

(In reply to zalan from comment #4)
> Comment on attachment 446394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446394&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        or be able to shrink to avoid floats. However, we can have a scenario where the current child block
> > +        doesnât have a float, but one of its descendants does. In this case, although we should continue to
> 
> and we don't propagate this "contains float" state to the ancestors when we
> insert the float to a descendant block? I wonder how this works during
> layout when we need to discover these floats.

Currently this "contains float" state is only a function of the block's `m_floatingObjects` set:
```
bool containsFloats() const override { return m_floatingObjects && !m_floatingObjects->set().isEmpty(); }
```

and

```
bool RenderBlockFlow::containsFloat(RenderBox& renderer) const
{
    return m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
}
```

so only the object that contains this float will have this state. There is no analogous flag.

It is not a flag.

[EWS]

Committed r286866 (245097@main): <https://commits.webkit.org/245097@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446394 [details].

[Darin Adler]

Comment on attachment 446394 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> +    bool contains = m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
> +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> +        if (!is<RenderBlockFlow>(block))
> +            continue;
> +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> +        contains |= blockFlow.subtreeContainsFloat(renderer);
> +    }
> +    return contains;

Since this boolean only gets more true over time, we should not use a local variable. We should just return true when we find something, and then return false at the end of the function.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2133
> +    bool contains = m_floatingObjects && !m_floatingObjects->set().isEmpty();
> +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> +        if (!is<RenderBlockFlow>(block))
> +            continue;
> +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> +        contains |= blockFlow.subtreeContainsFloats();
> +    }
> +    return contains;

Ditto.

[Gabriel Nava Marino]

Reopening to attach new patch.

[Gabriel Nava Marino]

Created attachment 446840 [details]
Patch

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #7)
> Comment on attachment 446394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446394&action=review
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> > +    bool contains = m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
> > +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> > +        if (!is<RenderBlockFlow>(block))
> > +            continue;
> > +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> > +        contains |= blockFlow.subtreeContainsFloat(renderer);
> > +    }
> > +    return contains;
> 
> Since this boolean only gets more true over time, we should not use a local
> variable. We should just return true when we find something, and then return
> false at the end of the function.
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2133
> > +    bool contains = m_floatingObjects && !m_floatingObjects->set().isEmpty();
> > +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> > +        if (!is<RenderBlockFlow>(block))
> > +            continue;
> > +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> > +        contains |= blockFlow.subtreeContainsFloats();
> > +    }
> > +    return contains;
> 
> Ditto.

Thank you for the suggestion Darin! I have updated the patch so that we have this early return.

[Darin Adler]

Comment on attachment 446840 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> +        return blockFlow.subtreeContainsFloat(renderer);

This is wrong. Should be:

    if (blockFlow.subtreeContainsFloat(renderer))
        return true;

One other thought: we could probably do a version of this algorithm that walks the tree without using recursion.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2136
> +        return blockFlow.subtreeContainsFloats();

Ditto.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #11)
> Comment on attachment 446840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446840&action=review
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> > +        return blockFlow.subtreeContainsFloat(renderer);
> 
> This is wrong. Should be:

Thank you for pointing this out. I now see the issue, we would be returning early on the first child, without checking the other siblings.

> 
>     if (blockFlow.subtreeContainsFloat(renderer))
>         return true;
> 
> One other thought: we could probably do a version of this algorithm that
> walks the tree without using recursion.

In WebKit, is there a preference for using one vs. the other?

> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2136
> > +        return blockFlow.subtreeContainsFloats();
> 
> Ditto.

[Gabriel Nava Marino]

Created attachment 446856 [details]
Patch

[Gabriel Nava Marino]

There are ios-wk2 failures, but they appear unrelated to the changes in this bug and tracked in Bug 234181.

[Darin Adler]

(In reply to Gabriel Nava Marino from comment #12)
> (In reply to Darin Adler from comment #11)
> > One other thought: we could probably do a version of this algorithm that
> > walks the tree without using recursion.
> 
> In WebKit, is there a preference for using one vs. the other?

I think we prefer non-recursive.

Our tree structure is designed so we can walk it without using a stack, and write non-recursive versions of algorithms that involve walking the entire tree, and I think we want to take advantage of that when possible for better efficiency.

[Gavin]

*** Bug 233554 has been marked as a duplicate of this bug. ***

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #15)
> (In reply to Gabriel Nava Marino from comment #12)
> > (In reply to Darin Adler from comment #11)
> > > One other thought: we could probably do a version of this algorithm that
> > > walks the tree without using recursion.
> > 
> > In WebKit, is there a preference for using one vs. the other?
> 
> I think we prefer non-recursive.
> 
> Our tree structure is designed so we can walk it without using a stack, and
> write non-recursive versions of algorithms that involve walking the entire
> tree, and I think we want to take advantage of that when possible for better
> efficiency.

I have attempted the non-recursive route using an iterator, but unfortunately the render descendant iterator does not currently work with const objects. That issue is tracked in bug 234325. I am moving these changes to commit-queue? for now, and filing a separate bug to bring non-recursion via the iterator blocked on 234325.

[Darin Adler]

(In reply to Gabriel Nava Marino from comment #17)
> the render descendant iterator does not currently work with
> const objects

I’d suggest using const_cast to work around that.

[Gabriel Nava Marino]

Created attachment 447254 [details]
Patch

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #18)
> (In reply to Gabriel Nava Marino from comment #17)
> > the render descendant iterator does not currently work with
> > const objects
> 
> I’d suggest using const_cast to work around that.

Thank you for the recommendation! I have updated the patch.

[Gabriel Nava Marino]

*** Bug 234326 has been marked as a duplicate of this bug. ***

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #18)
> (In reply to Gabriel Nava Marino from comment #17)
> > the render descendant iterator does not currently work with
> > const objects
> 
> I’d suggest using const_cast to work around that.

This approach worked and seems to have made EWS happy, so I have updated the cq flag to cq?.

[Darin Adler]

Comment on attachment 447254 [details]
Patch

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

This is great. Fine to land as is.

To refine further, we can take one more step by adding inclusiveDescendantsOfType to RenderDescendantIterator.h, which we already have in TypedElementDecendantIterator.h. If we do that, we can remove the check at the top of the function, and have only the loop.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2122
>          if (!is<RenderBlockFlow>(block))
>              continue;
>          auto& blockFlow = downcast<RenderBlockFlow>(block);
> -        contains |= blockFlow.subtreeContainsFloat(renderer);
> +        if (blockFlow.containsFloat(renderer))
> +            return true;

To refine further we *could* collapse these 5 lines into two:

    if (is<RenderBlockFlow>(block) && downcast<RenderBlockFlow>(block).containsFloat(renderer))
        return true;

[EWS]

Committed r287771 (245831@main): <https://commits.webkit.org/245831@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447254 [details].

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #23)
> Comment on attachment 447254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447254&action=review
> 
> This is great. Fine to land as is.
> 
> To refine further, we can take one more step by adding
> inclusiveDescendantsOfType to RenderDescendantIterator.h, which we already
> have in TypedElementDecendantIterator.h. If we do that, we can remove the
> check at the top of the function, and have only the loop.
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2122
> >          if (!is<RenderBlockFlow>(block))
> >              continue;
> >          auto& blockFlow = downcast<RenderBlockFlow>(block);
> > -        contains |= blockFlow.subtreeContainsFloat(renderer);
> > +        if (blockFlow.containsFloat(renderer))
> > +            return true;
> 
> To refine further we *could* collapse these 5 lines into two:
> 
>     if (is<RenderBlockFlow>(block) &&
> downcast<RenderBlockFlow>(block).containsFloat(renderer))
>         return true;

Thank you for the feedback. I will update these functions per the refinement recommendations.

[Gabriel Nava Marino]

Created attachment 449116 [details]
Patch

[Gabriel Nava Marino]

Created attachment 449117 [details]
Patch

[Darin Adler]

Comment on attachment 449117 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Implement InclusiveRenderDescendantIteratorAdapter and use inclusiveDescendantsOfType in
> +        RenderBlockFlow::subtreeContainsFloat and RenderBlockFlow::subtreeContainsFloats.

I suspect there is a way to implement this sharing more code with the non-inclusive case, since non-inclusive is just like inclusive, but just skipping the thing itself, but it’s more important how we *use* inclusiveDescendantsOfType than how smooth the implementation is. That can always be refined later.

I would suggest, though, that we address the const-ness issue eventually. Maybe even at the same time.

Ideally the descendantsOfType on a const reference would iterate const references, and non-const would iterate non-const. Ideally without duplicating all the code twice, so maybe using some const_cast in the implementation.

> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
> +    for (auto& block : inclusiveDescendantsOfType<RenderBlock>(const_cast<RenderBlockFlow&>(*this))) {
> +        if (is<RenderBlockFlow>(block) && downcast<RenderBlockFlow>(block).containsFloat(renderer))
>              return true;
>      }

Hmm, can we write this?

    for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
        if (block.containsFloat(renderer))
            return true;
    }

That’s the whole point of having the xxxOfType feature (and assumes the const).

[Gabriel Nava Marino]

Comment on attachment 449117 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        RenderBlockFlow::subtreeContainsFloat and RenderBlockFlow::subtreeContainsFloats.
> 
> I suspect there is a way to implement this sharing more code with the non-inclusive case, since non-inclusive is just like inclusive, but just skipping the thing itself, but it’s more important how we *use* inclusiveDescendantsOfType than how smooth the implementation is. That can always be refined later.
> 
> I would suggest, though, that we address the const-ness issue eventually. Maybe even at the same time.
> 
> Ideally the descendantsOfType on a const reference would iterate const references, and non-const would iterate non-const. Ideally without duplicating all the code twice, so maybe using some const_cast in the implementation.

Thank you very much for the suggestion Darin. I have some ideas on how I might tackle the implementation of the inclusive and non-inclusive cases as well as how one might address the const-ness issue using the recommended tips.

Given the scope of the changes required, I would prefer to tackle this separately in https://bugs.webkit.org/show_bug.cgi?id=235680, as I expect it will require updating areas of code currently using const_cast with descendantsOfType.

>> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
>>      }
> 
> Hmm, can we write this?
> 
>     for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
>         if (block.containsFloat(renderer))
>             return true;
>     }
> 
> That’s the whole point of having the xxxOfType feature (and assumes the const).

Thank you for pointing this out! I will update the patch.

However, please see my other comment. I would prefer to deal with the const issue separately in another patch for a cleaner change-set.

[Darin Adler]

Sounds great.

[Gabriel Nava Marino]

Created attachment 450152 [details]
Patch

[Darin Adler]

Comment on attachment 449117 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
>>>      }
>> 
>> Hmm, can we write this?
>> 
>>     for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
>>         if (block.containsFloat(renderer))
>>             return true;
>>     }
>> 
>> That’s the whole point of having the xxxOfType feature (and assumes the const).
> 
> Thank you for pointing this out! I will update the patch.
> 
> However, please see my other comment. I would prefer to deal with the const issue separately in another patch for a cleaner change-set.

Yes, eventually we’ll deal with const. But built-in type checking is available now.

[Darin Adler]

Comment on attachment 450152 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded

This doesn’t seem the right title any more. At this point, this patch is a refactor so the title would be:

    Simplify code by using inclusiveDescendantsOfType

The reason I suggested fixing the const, is that it’s a similar refactoring, but fine to do this one step at a time.

[Darin Adler]

Doing this clean-up and refactoring all on this original bug isn’t the ideal way to use bugs.webkit.org; I would have used a new bug. But let’s just get the change in.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #34)
> Doing this clean-up and refactoring all on this original bug isn’t the ideal
> way to use bugs.webkit.org; I would have used a new bug. But let’s just get
> the change in.

I will be sure to create a new bug for these type of clean-up and refactoring-related changes in subsequent patches.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #33)
> Comment on attachment 450152 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450152&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
> 
> This doesn’t seem the right title any more. At this point, this patch is a
> refactor so the title would be:
> 
>     Simplify code by using inclusiveDescendantsOfType
> 

Thank you for the suggestion. I will update the title. 

> The reason I suggested fixing the const, is that it’s a similar refactoring,
> but fine to do this one step at a time.

Thank you, I see. I was not sure about the scope of the required changes to fix the const issue outside of RenderBlockFlow, and so that is why I originally created the other bug.

[Gabriel Nava Marino]

Created attachment 450157 [details]
Patch