Needs bisect, don't think it's from my patch, but none of the other patches look suspect.

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

I'm having a quick look at this now. Adding eae since it seems to be in layout.

+robhogan, our resident float expert.

So I poked around for a while, and all I found was a weird thing, demonstrated in this CL:
https://codereview.chromium.org/2733173002

in LayoutBlockFlow::addOverhangingFloats, one of the FloatingObjects in m_floatingObjects contains a hanging pointer to a LayoutObject in its m_layoutObject member. I didn't figure out how that happened by looking at the code, hopefully one of the Layout folks knows!

Proximate cause is likely to be https://codereview.chromium.org/2687273002

The test case is not very reducible, but I'm pretty sure this is because we delete floats from the orthogonal subtree in layoutOrthogonalWritingModeRoots() before laying out the main tree. (Crash goes away if I remove the vertical-mode-lr or remove removeFloatingObjectsForSubtreeRoot()).

kojii@ - this looks like one for you I'm afraid! :(

We're close to promoting M58 to Beta (next week March 16th). This bug is listed as RB-Beta-blocker. Can you please take a look at this bug and resolve before Beta promotion?

Looking.

This is reproducible for me as of ToT, and since it has float and writing-mode, layoutOrthogonalWritingModeRoots() is one possible suspect.

From the stack trace, the LayoutObject is deleted in surroundContents(). Then within the event handler from surroundContents(), it calls surroundContents(), and do it recursively. Finally when surroundContents() tries to set selection, we read the deleted LayoutObject.

Talked with tkent@. The suspected CL in #10 https://codereview.chromium.org/2687273002 changes the selection behavior, so that might have triggered this problem.

Also, surroundContents() not suppressing events until its operation is done isn't probably desired, but he suspects this could be reproducible even if surroundContents() suppressed events until its operation is done.

I'll look into more on layoutOrthogonalWritingModeRoots() and floats, then consult again with tkent@ if needed.

This is probably similar to issue 656419, where too much removeFloatingObjects() may lead to problem.

I still don't have good idea why it can be so, instinct tells me opposite, but issue 656419 fixed by reducing removeFloatingObjects(). If I skip removeFloatingObjects() all together, this is gone.

Looking for where we rebuild m_floatingObjects and we're probably not applying exactly the same condition, but this doesn't look easy.

Or maybe another approach; we disable our optimization to delay removeFloatingObjects() until next layout under certain conditions, maybe when crossing writing-modes boundary?

Either way, I will need to understand how we rebuild m_floatingObjects.

Need to borrow the bug, to be able to see the fuzzer report.

You're more than welcome to take it if you'd like ;-)

WIP sent for review
https://codereview.chromium.org/2737253003

Some notes on repro.

While minimizing JS isn't easy, CSS is easy. We need 4 properties:
  float:left or right
  display:table
  writing-mode:vertical-lr
  margin-bottom:15in

tkent@ and yosin@ helped me to analyze DOM parts.
  Range.surroundContents(newParent);
can be re-written as:
  while (newParent.firstChild)
    newParent.removeChild(newParent.lastChild);
  var fragment = range.extractContents();
  range.insertNode(newParent);
  newParent.appendChild(fragment);
  document.body.offsetTop;
When I replaced Range.surroundContents() with this JS, it crashes in document.body.offsetTop when it calls updateLayout(). So I considered batching events in Range.surroundContents() but the bug should be reproducible by the above JS.

CL in #10 https://codereview.chromium.org/2687273002 might have triggered this because it changes the selection code, which may call updateLayout() at the end of Range.surroundContents(). Reverting it might fix this repro, but the root cause is likely to be different.

What is happening, as far as I understand at this point, is:
1. A float (probably in a table cell?) has large height (margin-bottom:15in probably makes this) to overhang to next cell or next block after the table, and that its pointer is copied to multiple ancestors by LayoutBlockFlow::addOverhangingFloats().
2. Create a situation where one of such ancestors has needsLayout() but its descendants that has pointers to the float are layout-clean.
3. Have an orthogonal root that has the ancestor as its containing block, or one of the descendants.
4. A child of an anonymous LayoutBlockFlow keeps the pointer. I'm not sure if this is related.
5. Either 3 (the orthogonal subtree layout) or the table layout algorithm might help to create the situation 2.

After the floating object was destroyed, next new LayoutObject is allocated at the same address, both on Win and Linux. This is probably unrelated, but as a record in case.

Reduced test case. Haven't tried to debug it yet.

Deleted: tc.html 1.3 KB

tc.html 1.3 KB View Download

Superb, thank you Morten!

Here's a smaller test case. It doesn't crash, neither width nor without ASAN. Instead it shows a visual rendering error, and it pertains to the same bug. There are pointers to dead LayoutObjects when this happens. Disabling removeFloatingObjectsForSubtreeRoot() corrects the situation. But I suppose we're not ready for a fix like that.

For testing, I stuck this into FrameView.cpp and called it before and after any layout operation, to look for pointers to dead objects. And it fails with the test case attached.

static void assertAgainstDeath(const LayoutView* view) {
  for (const LayoutObject* walker = view; walker;
       walker = walker->nextInPreOrder()) {
    if (!walker->isLayoutBlockFlow())
      continue;
    const FloatingObjects* flobs = toLayoutBlockFlow(walker)->floatingObjects();
    if (!flobs)
      continue;
    const FloatingObjectSet& floatingObjectSet = flobs->set();
    FloatingObjectSetIterator end = floatingObjectSet.end();
    for (FloatingObjectSetIterator it = floatingObjectSet.begin(); it != end;
         ++it) {
      const FloatingObject& floatingObject = *it->get();

      // Looks like our debug allocator sets deleted memory to a
      // 0xcdcdcdcdcdcdcdcd pattern, and the following check will fail when used
      // on dead objects, at least with the current LayoutObject bitfield
      // layout.
      DCHECK(floatingObject.layoutObject()->getSelectionState() <= SelectionBoth);
    }
  }
}

I've added comments in the test case to explain what I think is going on here.

The comment in removeFloatingObjectsForSubtreeRoot() suggests that we deliberately leave pointers to dead LayoutObject objects around for the next layout. This sounds horrible and should be fixed! Then we can just get rid of removeFloatingObjectsForSubtreeRoot().

As long as we allow leaving pointers to dead objects around, however, kojii's fix in https://codereview.chromium.org/2737253003 seems like the right thing to me.

I don't intend to work on an actual fix, so, while kojii's offer in #c17 was very generous, I humbly decline, and assign it back to you, kojii. :)

Deleted: tc-visual.html 2.0 KB

tc-visual.html 2.0 KB View Download

ClusterFuzz has detected this issue as fixed in range 455700:456019.

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

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_asan_chrome_v8_arm
Platform Id: linux

Crash Type: Heap-use-after-free READ 4
Crash Address: 0xbb131a94
Crash State:
  blink::LayoutBlockFlow::addOverhangingFloats
  blink::LayoutBlockFlow::layoutBlockChild
  blink::LayoutBlockFlow::layoutBlockChildren
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_v8_arm&range=449916:449922
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_v8_arm&range=455700:456019

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv95KOh7zhnaaon78hz5gINd0iHKtZNEzXgqcc3ePOGSpGzm3LbWs6haG2QBnhQqEw9t6IGnbU6feT7_dAs0Kxu0eWuR7qJ-lzp5MyrHPT27Tn9S8jGirCocVimC-NjSRCxF_BOK-GW0PboMTiVZiYrQcVuBzRp2U4b9uQsA6DczSkYXeNzusvmQpBuHAHO73qL3HUdW4SC163NAaA0SS-QUPc1pzth-LH2FCtwVfnwVhCRhf9UiZtM1KtpJp1gc4AfB4keCXJs_WeSaowiiWTKNW8SNjuMhXDyoBitv4wXu0mQod6wuBLIc0CSuYxbXqtraHKQ-mdJvsRhzVlAgo_36sdDtreoT9v4VynxdeyhWya6Cju7E?testcase_id=4730470358319104


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 4730470358319104 is verified as fixed, so closing issue.

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

Probably fixed by https://chromium.googlesource.com/chromium/src/+/caf3c3acbe69ac013e2aaafad5c8110bb66e435a.

There's still something to fix here, if we can still test for it.

Thank you robhogan@ for the analysis, yeah, we can still crash by replacing surroundContents() with equivalent JS, or mstensho@'s case from #19 still crashes. The second case from #21 doesn't crash though.

But #21 helps me a lot to understand what's happening behind. I've been wondering why clearing could make things worse. Thank you both for this much help!!

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

commit c020f6a22577978ce1fe89fc1a397f2a651c48a8
Author: kojii <kojii@chromium.org>
Date: Sun Mar 12 10:20:54 2017

Remove floating objects from descendants of subtree roots

The pre-layout for orthogonal flow clears floats from its containing
block in [1]. However, this can cause trouble for
markAllDescendantsWithFloatsForLayout() to mark descendants for layout.

When preceding floats overhanging to following blocks are gone, such
floats are referred from multiple following blocks including
descendants. Failure to mark descendants for layout may leave preceding
floats.

This patch ensures such floats are removed.

[1] https://codereview.chromium.org/2025543002

BUG= 698455 

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

[add] https://crrev.com/c020f6a22577978ce1fe89fc1a397f2a651c48a8/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash-3.html
[modify] https://crrev.com/c020f6a22577978ce1fe89fc1a397f2a651c48a8/third_party/WebKit/Source/core/frame/FrameView.cpp

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

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

commit 285585b19c24f98a5b25d8536e5950c18db3a847
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Mar 16 06:59:23 2017

Merge 3029: Remove floating objects from descendants of subtree roots

The pre-layout for orthogonal flow clears floats from its containing
block in [1]. However, this can cause trouble for
markAllDescendantsWithFloatsForLayout() to mark descendants for layout.

When preceding floats overhanging to following blocks are gone, such
floats are referred from multiple following blocks including
descendants. Failure to mark descendants for layout may leave preceding
floats.

This patch ensures such floats are removed.

[1] https://codereview.chromium.org/2025543002

BUG= 698455 

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

Review-Url: https://codereview.chromium.org/2752193002 .
Cr-Commit-Position: refs/branch-heads/3029@{#230}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[add] https://crrev.com/285585b19c24f98a5b25d8536e5950c18db3a847/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash-3.html
[modify] https://crrev.com/285585b19c24f98a5b25d8536e5950c18db3a847/third_party/WebKit/Source/core/frame/FrameView.cpp

Thank you Andrew.

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