Hi wangxianzhu@, I saw you helped out on a similar crash ( issue 610986 ), mind taking a look at this one?

Also adding some other folks from the previous bug.

It seems that a LayoutBlock with block children has a non-block child.

eae@ can you help to find the proper owner?

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Philip, could you take a look at this security regression in fullscreen? 

regression range:
https://chromium.googlesource.com/chromium/src/+log/80c04977423898bd64b1cfa91246ced5cc35d80b..ccb5d4fc7a7e1045bd11fa7710879d4a6df850ed?pretty=fuller

I have tried hard to reproduce this with no luck. I've followed the "Clusterfuzz Local Reproduction" steps and "No crash found" after 24 runs. I've tried the build from clusterfuzz, and local builds from the same commit (r408294) and master, lsan and debug, with various versions of the test case, in content_shell --run-layout-test and chrome. No crash.

Looking at the ASSERT that failed, blink::LayoutBlockFlow::layoutBlockChildren has only one call site, when !childrenInline(). Most uses of nextSiblingBox() that I can see are also guarded by a childrenInline() check or assert.

eae@, does !childrenInline() imply that all of the children are LayoutBox so that traversing with firstChildBox() and nextSiblingBox() is safe?

If it is, then the remaining possibilities that I see are memory corruption of some sort, or that the book keeping behind childrenInline() (m_bitfields.childrenInline()) is somehow broken by the Fullscreen layout hack.

In the meantime, I've checked some boxes to have clusterfuzz try this again.

Ping eae@, any input on #7?

foolip: Yes, if !childrenInline then traversing blockChildren is ok.

A good first change might be to add a release assert, that way at least it won't be a security bug and it might help track down where we go wrong.

Temporarily stealing this bug, so I can have a look at the fuzzer report.

I don't know how to parse the gestures string, but anyway, I guess we're dealing with a timing issue. One wild guess would be that the tree structure gets messed up for a very short time and "fixed" right afterwards, and that if we're unlucky enough to lay out while the tree is in this state, it will assert. And that you're not that unlucky. :)

Tree structure changes aren't necessarily immediately followed by layout, but maybe you could add some heavy ASSERTs connected to Node::attachLayoutTree() and detachLayoutTree(). Basically, ASSERT that all LayoutBlockFlow objects that have !childrenInline() only has LayoutBox-derived children, and see if you get any failures. It's probably a good idea to check all LayoutBlockFlow objects in the entire document each time a tree structure modification takes place, and not just the node in question and its parent.

Friendly ping, this is currently a Beta-blocker and needs to get fixed and merged as soon as feasible, as M54 is going to beta this Thursday 9/8

#14, no luck with heavy asserting, here's what I called at the end of Node::attachLayoutTree() and detachLayoutTree():

namespace {

void checkChildren(const Node& node)
{
    for (const LayoutObject* iter = node.document().layoutView();
         iter; iter = iter->nextInPreOrder()) {
        if (iter->isLayoutBlockFlow() && !iter->childrenInline()) {
            for (const LayoutObject* child = iter->slowFirstChild(); child;
                 child = child->nextSibling()) {
                CHECK(child->isBox());
            }
        }
    }
}

}

That sucks. :( And the fuzzer is still able to crash?

Another interesting piece of code is LayoutFullScreen::unwrapLayoutObject(), which allegedly even modifies the layout tree during layout. How about some heavy asserts there?

(Deleted comment, false flag.)

I have sprinkled the same kind of children checks in 
::wrapLayoutObject() and unwrapLayoutObject() without finding anything. (The crash in #18 was just because I did the check after the destroy() call.)

Morten, if you'd like to investigate yourself, feel free to self-assign. In any case I'll land those added CHECKs to see if that reveals anything.

Also, I'm not sure when ClusterFuzz was last able to reproduce this, I just asked it to redo everything but don't know what the ETA of that is.

Let's see what the fuzzer has to say next. If it still crashes, I might have a look.

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

commit bed5e3cfe0bff0458604c83665da1d88bc38c8f4
Author: foolip <foolip@chromium.org>
Date: Tue Sep 06 12:35:50 2016

Add speculative CHECKs to track down a failing ASSERT

BUG= 632848 
R=eae@chromium.org

Review-Url: https://codereview.chromium.org/2304143002
Cr-Commit-Position: refs/heads/master@{#416620}

[modify] https://crrev.com/bed5e3cfe0bff0458604c83665da1d88bc38c8f4/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

bustamante@, this is now fixed in the sense that the CHECKs will prevent any security issue in the wild, but the root cause is unknown so if this is memory corruption or something other than what it seems, the problem can remain. Is it appropriate to leave the issue open until it's conclusive, or should I close it to unblock M54?

ClusterFuzz has detected this issue as fixed in range 416613:416628.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6591018657120256

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: ASSERT
Crash Address: 
Crash State:
  !object || (object->isBox())
  blink::LayoutBlockFlow::layoutBlockChildren
  blink::LayoutBlockFlow::layoutBlockFlow
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=395613:395675
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=416613:416628

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95gaCVQHb6fPuRDo016Jdni2ji3BRA5XVzoyjZJ0g75I1mQG3nGNDIx0ufTZ_-HPqZ6faXWcA2RQxgNFrzuagpaXWEgze85PicJMWd_K7fvfizL9rc9xq2IFuQfk7gdMTI8FwCY1r_-NufosWDQsjE6hXbDeQ?testcase_id=6591018657120256


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

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

ClusterFuzz testcase is verified as fixed, closing issue.

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

ClusterFuzz found it again in  issue 645224 , now failing the CHECKs instead :)

Your change meets the bar and is auto-approved for M54 (branch: 2840)

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

commit 84b55e937c2b97e090d50bfaf28ec2a3923cfe39
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Tue Sep 13 08:56:30 2016

Add speculative CHECKs to track down a failing ASSERT

BUG= 632848 
R=eae@chromium.org

Review-Url: https://codereview.chromium.org/2304143002
Cr-Commit-Position: refs/heads/master@{#416620}
(cherry picked from commit bed5e3cfe0bff0458604c83665da1d88bc38c8f4)

Review URL: https://codereview.chromium.org/2333203002 .

Cr-Commit-Position: refs/branch-heads/2840@{#321}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/84b55e937c2b97e090d50bfaf28ec2a3923cfe39/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

 Issue 642064  has been merged into this issue.

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

commit 84b55e937c2b97e090d50bfaf28ec2a3923cfe39
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Tue Sep 13 08:56:30 2016

Add speculative CHECKs to track down a failing ASSERT

BUG= 632848 
R=eae@chromium.org

Review-Url: https://codereview.chromium.org/2304143002
Cr-Commit-Position: refs/heads/master@{#416620}
(cherry picked from commit bed5e3cfe0bff0458604c83665da1d88bc38c8f4)

Review URL: https://codereview.chromium.org/2333203002 .

Cr-Commit-Position: refs/branch-heads/2840@{#321}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/84b55e937c2b97e090d50bfaf28ec2a3923cfe39/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

M55 was branched on october 6th. Is there any merge is needed for M55? If yes, please request ASAP. Thank you.

Per comment 25, this is fixed in M55.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot