+awhalley@, will this be a blocker for M58 stable refresh release? 

govind@ - nope, but thanks for the ping

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6200096875347968

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

Job Type: linux_asan_chrome_mp
Crash Type: Heap-use-after-free READ 8
Crash Address: 0x612000040d18
Crash State:
  blink::ShapeOutsideInfo::IsEnabledFor
  blink::LayoutBox::GetShapeOutsideInfo
  blink::ComputeFloatOffsetForLineLayoutAdapter<
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=417611:417632

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


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

The only relevant CL in the regression range seems to be https://chromium.googlesource.com/chromium/src/+/2ad494069b0447cc2d32c844ce6eb991cc129bd1

ymalik: Can you please take a look? Thanks.

+some layout people.

+eae

Sheriff ping: can someone please take a look and triage?

Rob, your change r387862 is in the regression range. Would you mind taking a look?

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

The regression ranges keep moving around on this one. I can make the failure to remove the float from the float-lists go away if I remove the multicol style on the testcase so multicol might be involved here.

I haven't managed to reduce it very much but here is a test case that reliably fails.

What is happening is:

 - Element::UpdateStyle() changes <div id="first"> from a normal flow block to a float. So LayoutBlockFlow::StyleDidChange() marks it and its descendants for layout because they all contain overhanging floats, i.e. floats from previous siblings and ancestors that intrude into it and that it tracks in order to avoid. The idea is that at next layout <div id="first"> will clear down its list of floats to track and build new ones given its new style.
 - Then Element:RebuildLayoutTree() happens and starts removing various items in the tree, including some of the floats that <div id="first"> used to track. However this removal process fails to clear them from the div's float lists.
 - A crash eventually happens, because we have dangling pointers in float lists pointing to objects that no longer exist.

bugsnash/nainar: Are we rebuilding the layout tree after style change more aggressively than we used to with the changes introduced by the squad team? The style change code has always assumed it was safe enough to mark objects for layout in order to rebuild their float lists at layout time rather than clear them down immediately because the floats their pointing to might go away before layout happens. So this is a surprising bug to find and clusterfuzz's inability to find a clear regression range suggests something slightly non-deterministic has started happening only recently.

Deleted: 714440.html 1.2 KB

714440.html 1.2 KB View Download

I can't seem to get the testcase to fail on ToT. 

However, building Chromium both before and after https://codereview.chromium.org/2473743003 and then running the testcase provided shows that we call detach/attach the same number of times before and after and in the same order as well. So I don't think this has anything to do with Squad.

(borrowing the bug, in order to be able to access the fuzzer reports)

I cannot reproduce it reliably (I can't reproduce it at all, it seems) with 714440.html, but I can with the test I'm attaching now (using an ASAN build).

Ideally I'd like to see a test where we've got rid of all CSS selectors and just use style attributes instead, but so far I've been unable to do so.

The :only-of-type seems relevant. If I understand correctly, this one cannot be evaluated until we have closed the parent element. In the case of the topmost DIV, it probably goes from being multicol (because of .CLASS5) to being non-multicol (when we realize that it's the only DIV child), but I haven't checked this.

Deleted: tc.html 392 bytes

tc.html 392 bytes View Download

(and I forgot to mention that tc.html is based on the test in the first fuzzer report in this bug)

Very nice reduction mstensho - you must show me how you managed that!

Yeah, that testcase was truly unruly, until I found the missing magical document.body.offsetTop. :)

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

commit a925ec8c709dcc3768dfeb349bf19c766d6f5c7d
Author: robhogan <robhogan@gmail.com>
Date: Mon May 08 21:16:29 2017

Flowthread should move its floatlists to container when evacuating

This ensures that if any of the floats gets deleted the new
container will be able to delete from its new descendants'
float-lists.

BUG= 714440 

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

[add] https://crrev.com/a925ec8c709dcc3768dfeb349bf19c766d6f5c7d/third_party/WebKit/LayoutTests/fast/multicol/flowthread-with-floats-destroyed-crash.html
[modify] https://crrev.com/a925ec8c709dcc3768dfeb349bf19c766d6f5c7d/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/a925ec8c709dcc3768dfeb349bf19c766d6f5c7d/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp

ClusterFuzz has detected this issue as fixed in range 470101:470142.

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

Fuzzer: ochang_domfuzzer
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x61100003c058
Crash State:
  blink::ShapeOutsideInfo::IsEnabledFor
  blink::LayoutBox::GetShapeOutsideInfo
  blink::ComputeFloatOffsetForLineLayoutAdapter<
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=353966:353986
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=470101:470142

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


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 has detected this issue as fixed in range 470101:470142.

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

Job Type: linux_asan_chrome_mp
Crash Type: Heap-use-after-free READ 8
Crash Address: 0x612000040d18
Crash State:
  blink::ShapeOutsideInfo::IsEnabledFor
  blink::LayoutBox::GetShapeOutsideInfo
  blink::ComputeFloatOffsetForLineLayoutAdapter<
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=417611:417632
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=470101:470142

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


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

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

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(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/+/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59

commit 37b22ee298b3cde4c44ffc5ab83449c26b0dfe59
Author: Robert Hogan <robhogan@gmail.com>
Date: Thu May 11 21:55:12 2017

Flowthread should move its floatlists to container when evacuating

This ensures that if any of the floats gets deleted the new
container will be able to delete from its new descendants'
float-lists.

BUG= 714440 

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

Review-Url: https://codereview.chromium.org/2876943002 .
Cr-Commit-Position: refs/branch-heads/3071@{#519}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[add] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/fast/multicol/flowthread-with-floats-destroyed-crash.html
[modify] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp

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