New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 852640 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: !next_sibling || next_sibling->IsBox() in layout_block_flow.cc

Project Member Reported by ClusterFuzz, Jun 14 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6500804354375680

Fuzzer: bj_broddelwerk
Job Type: linux_ubsan_vptr_chrome
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !next_sibling || next_sibling->IsBox() in layout_block_flow.cc
  blink::LayoutBlockFlow::LayoutBlockChildren
  blink::LayoutBlockFlow::LayoutChildren
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_chrome&range=502802:502818

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6500804354375680

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Jun 14 2018

Components: Blink>Layout
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: brajkumar@chromium.org
Labels: M-68 CF-NeedsTriage
Unable to find actual suspect through code search and also observing no suspected CL's under regression range, hence adding appropriate label and requesting someone from blink team to look in to this issue.

Thanks!

Comment 3 by e...@chromium.org, Jun 15 2018

Owner: mstensho@chromium.org
Status: Assigned (was: Untriaged)
Would you mind looking into this one Morten?
I now understand what goes wrong, and there are a couple of ways of fixing it:

A. A ruby specific fix that copes with situations that shouldn't occur (easy fix, probably safe)
B. A block-flow generic fix that makes sure this situation doesn't occur in the first place (it's causing other bugs too)

I have a reduction (attached). Writing a slightly more minimized testcase should be possible, but I need to study the ruby code a bit more first.

We have a ruby element with a mix of blocks and inlines, roughly like this: Block #1, inline, block #2, out-of-flow block. The inline is obviously wrapped inside an anonymous block. Block #2 is actually a table, but that doesn't really matter, as long as it's something block-level.
  block #1
  anon block
    inline
  block #2
  out-of-flow

Now, if we remove block #2, the out-of-flow block sibling should be wrapped inside the anonymous block wrapper of the inline. We *should* have got this:
  block #1
  anon block
    inline
    out-of-flow

But this doesn't happen. The out-of-flow block remains where it is. So we get this:
  block #1
  anon block
    inline
  out-of-flow

If we then add an inline-level element after the out-of-flow block, we'll realize that it needs to be inside an anonymous block. Since there's no anonymous block adjacent to its insertion point, we create one, and wrap the inline AND the out-of-flow block inside it (which is kind of correct). The result is this:
  block #1
  anon block
    inline
  anon block #2
    out-of-flow
    inline

So we have two sibling anonymous blocks. This should never happen (unless we have continuations).

While this is happening, something is happening to the ruby layout structure as well. We split into two ruby runs and bases, and then merge it back later. It's during this merge that we end up with a layout block marked as having block children that has an inline child, which later on triggers the CHECK failure.

It happens from LayoutRubyBase::MoveBlockChildren(). There's first some code that merges content inside the last anonymous block in the first ruby base with the first anonymous block in the second base. At this point the second base contains basically this:
  anon block #1
    inline
  anon block #2
    out-of-flow
    inline

(two nonsensical anonymous blocks, in other words - we shouldn't have any of them)

By emptying the first anonymous block into the first base, some piece of code FINALLY discovers that we don't need anonymous blocks, so all we have left in the second base:
  out-of-flow
  inline

The base is properly marked as having inline children now. Calling MoveAllChildrenIncludingFloatsTo() now without full_remove_insert is disastrous. We end up with one single base (marked as having block children) that contains:
  something block level
  anon block
    inline
  out-of-flow
  inline

And we crash.
reduction.html
1.4 KB View Download
tc.html
3.4 KB View Download
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2394d0acf8bf0b0236b2646e4c823a57de7bd941

commit 2394d0acf8bf0b0236b2646e4c823a57de7bd941
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Sat Jun 16 15:44:49 2018

Floats and out-of-flow objects may not be adjacent to anonymous blocks.

Floats and out-of-flow objects need to be true layout siblings of the
inlines, or rendering will be wrong. This means that such objects should
never be siblings of anonymous blocks, but rather inside them. This
already works correctly for initial layout tree building, and also for
many DOM manipulations. However, code was missing to satisfy this
requirement if we removed a regular block that was a sibling of an
anonymous block and either a float or out-of-flow positioned object.

This even caused a crash triggered by ruby code, which ended up mixing
inline and block children within the same container. That is not
allowed. This happened in the MoveAllChildrenIncludingFloatsTo() call
inside LayoutRubyBase::MoveBlockChildren(). Added a DCHECK to
MoveAllChildrenIncludingFloatsTo() (which could fail prior to this fix);
When moving children from one container to another, either both or none
of the containers must have inline children.

Bug:  852640 
Change-Id: I51d3de12c73ddd07d6b4c1aa55221b4f92359ca7
Reviewed-on: https://chromium-review.googlesource.com/1102690
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567882}
[add] https://crrev.com/2394d0acf8bf0b0236b2646e4c823a57de7bd941/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/abspos/remove-block-between-inline-and-abspos.html
[add] https://crrev.com/2394d0acf8bf0b0236b2646e4c823a57de7bd941/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/remove-block-between-inline-and-float.html
[add] https://crrev.com/2394d0acf8bf0b0236b2646e4c823a57de7bd941/third_party/WebKit/LayoutTests/fast/ruby/merge-base-anonymous-blocks-crash.html
[modify] https://crrev.com/2394d0acf8bf0b0236b2646e4c823a57de7bd941/third_party/blink/renderer/core/layout/layout_block_flow.cc

Project Member

Comment 7 by ClusterFuzz, Jun 17 2018

ClusterFuzz has detected this issue as fixed in range 567880:567882.

Detailed report: https://clusterfuzz.com/testcase?key=6500804354375680

Fuzzer: bj_broddelwerk
Job Type: linux_ubsan_vptr_chrome
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !next_sibling || next_sibling->IsBox() in layout_block_flow.cc
  blink::LayoutBlockFlow::LayoutBlockChildren
  blink::LayoutBlockFlow::LayoutChildren
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_chrome&range=502802:502818
Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_chrome&range=567880:567882

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6500804354375680

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 8 by ClusterFuzz, Jun 17 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 6500804354375680 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9d1ed958ab494661acc228f31d879e490352d7a

commit e9d1ed958ab494661acc228f31d879e490352d7a
Author: Abhishek Arya <inferno@chromium.org>
Date: Sun Jun 17 18:28:17 2018

Revert "Floats and out-of-flow objects may not be adjacent to anonymous blocks."

This reverts commit 2394d0acf8bf0b0236b2646e4c823a57de7bd941.

Reason for revert: Caused ClusterFuzz regressions - 853522, 853537, 853538, 853540.

Original change's description:
> Floats and out-of-flow objects may not be adjacent to anonymous blocks.
> 
> Floats and out-of-flow objects need to be true layout siblings of the
> inlines, or rendering will be wrong. This means that such objects should
> never be siblings of anonymous blocks, but rather inside them. This
> already works correctly for initial layout tree building, and also for
> many DOM manipulations. However, code was missing to satisfy this
> requirement if we removed a regular block that was a sibling of an
> anonymous block and either a float or out-of-flow positioned object.
> 
> This even caused a crash triggered by ruby code, which ended up mixing
> inline and block children within the same container. That is not
> allowed. This happened in the MoveAllChildrenIncludingFloatsTo() call
> inside LayoutRubyBase::MoveBlockChildren(). Added a DCHECK to
> MoveAllChildrenIncludingFloatsTo() (which could fail prior to this fix);
> When moving children from one container to another, either both or none
> of the containers must have inline children.
> 
> Bug:  852640 
> Change-Id: I51d3de12c73ddd07d6b4c1aa55221b4f92359ca7
> Reviewed-on: https://chromium-review.googlesource.com/1102690
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567882}

TBR=eae@chromium.org,mstensho@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  852640 
Change-Id: I0af60d9eac770f21b6a0fbeccf88bb43d5efd6fb
Reviewed-on: https://chromium-review.googlesource.com/1103503
Reviewed-by: Abhishek Arya <inferno@chromium.org>
Commit-Queue: Abhishek Arya <inferno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567914}
[delete] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/abspos/remove-block-between-inline-and-abspos.html
[delete] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/remove-block-between-inline-and-float.html
[delete] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/third_party/WebKit/LayoutTests/fast/ruby/merge-base-anonymous-blocks-crash.html
[modify] https://crrev.com/e9d1ed958ab494661acc228f31d879e490352d7a/third_party/blink/renderer/core/layout/layout_block_flow.cc

Status: Assigned (was: Verified)
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1

commit b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Mon Jun 18 22:21:54 2018

Reland: Floats and out-of-flow objects may not be adjacent to anonymous blocks.

Floats and out-of-flow objects need to be true layout siblings of the
inlines, or rendering will be wrong. This means that such objects should
never be siblings of anonymous blocks, but rather inside them. This
already works correctly for initial layout tree building, and also for
many DOM manipulations. However, code was missing to satisfy this
requirement if we removed a regular block that was a sibling of an
anonymous block and either a float or out-of-flow positioned object.

This even caused a crash triggered by ruby code, which ended up mixing
inline and block children within the same container. That is not
allowed. This happened in the MoveAllChildrenIncludingFloatsTo() call
inside LayoutRubyBase::MoveBlockChildren(). Added a DCHECK to
MoveAllChildrenIncludingFloatsTo() (which could fail prior to this fix);
When moving children from one container to another, either both or none
of the containers must have inline children.

This is a reland of https://chromium-review.googlesource.com/1102690
with some modifications to avoid  bug 853552 .

Bug:  852640 
Change-Id: I0f8a0aa5523e8fe60c841164d25aad088f4b728f
Reviewed-on: https://chromium-review.googlesource.com/1104900
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568196}
[add] https://crrev.com/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/abspos/remove-block-between-inline-and-abspos.html
[add] https://crrev.com/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/remove-block-between-inline-and-float.html
[add] https://crrev.com/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1/third_party/WebKit/LayoutTests/fast/block/toggle-float-add-continuation-crash.html
[add] https://crrev.com/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1/third_party/WebKit/LayoutTests/fast/ruby/merge-base-anonymous-blocks-crash.html
[modify] https://crrev.com/b414bf176fc6451c33c2c5c4b9609bd28ea3e5a1/third_party/blink/renderer/core/layout/layout_block_flow.cc

Status: Fixed (was: Assigned)

Sign in to add a comment