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

Issue 700655 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Not on Chrome anymore
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , All , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Crash in blink::FirstLetterPseudoElement::attachFirstLetterTextLayoutObjects

Project Member Reported by ClusterFuzz, Mar 11 2017

Issue description

Cc: msrchandra@chromium.org
Components: Blink>DOM
Labels: Test-Predator-Correct-CLs M-58
Owner: nainar@chromium.org
Status: Assigned (was: Untriaged)
Assigning to the concern owner from Predator results --
The result is a list of CLs that change the crashed files. 

Author: nainar
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src/+/f4c976fc1f4578b0c75ba304553ef2ff6134f551
Time: Mon Mar 06 04:05:17 2017
Lines 1321-1332 of file ContainerNode.cpp which potentially caused crash are changed in this cl (frame #5, "blink::ContainerNode::rebuildChildrenLayoutTrees"). 

Lines 2082-2087, 2093-2101, 2116-2122 of file Element.cpp which potentially caused crash are changed in this cl (frame #2, "blink::Element::rebuildLayoutTree"; frame #3, "reattachPseudoElementLayoutTree"; frame #4, "blink::Element::rebuildLayoutTree"; frame #6, "blink::Element::rebuildLayoutTree"). 

File Node.cpp is changed in this cl (and is part of stack frame #1, "blink::Node::reattachLayoutTree")
Minimum distance from crash line to modified line: 0. (file: ContainerNode.cpp, crashed on: 1321, modified: 1321).

@nainar -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

Comment 2 by nainar@chromium.org, Mar 15 2017

Labels: -OS-Mac OS-All

Comment 3 by nainar@chromium.org, Mar 15 2017

Further minimized test case:

<style>
.c3 { -webkit-appearance: button; }
.c3:nth-last-child(even) { display: table-row; }
.c14 { float: left; }
.c14::first-letter { float: inherit; }
</style>

<del id="del" class="c14">
	<figcaption id="figcaption" class="c3">
		Test passes if it doesn't crash.
	</figcaption>
</del>
<script>
strong = document.createElement('strong');
setTimeout('del.appendChild(strong);', 0);
</script>

Project Member

Comment 4 by ClusterFuzz, Mar 16 2017

Labels: OS-Android OS-Mac

Comment 5 by nainar@chromium.org, Mar 16 2017

Cc: esprehn@chromium.org
Status: Started (was: Assigned)
Sorry Elliott thought I had added you with my previous comment. 

Reduced the test case further to not include weird elements. 

<style>
.c3 { -webkit-appearance: button; }
.c3:nth-last-child(even) { display: table-row; }
.c14 { float: left; }
.c14::first-letter { float: inherit; }
</style>

<div id="div" class="c14">
	<div class="c3">
		Test passes if it doesn't crash.
	</div>
</div>
<script>
setTimeout('div.appendChild(document.createElement("div"));', 0);
</script>

The issue lies in Element::reattachPseudoElementLayoutTree
we need to remove the firstletter pseudoElement from the rareData in the case we change float. This is to mirror Element::updatePseudoElement(). Sending you a patch in ten minutes or so. Just running tests. 

Comment 6 by nainar@chromium.org, Mar 17 2017

Half baked patch here for any passerby: https://codereview.chromium.org/2752163003
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 6 2017

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

commit 6d202487ff823def4672599f51e9f13880e6b877
Author: nainar <nainar@chromium.org>
Date: Thu Apr 06 06:32:49 2017

Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject

This patch edits Element::rebuildPseudoElementLayoutTree() to make sure
it mirrors Element::updatePseudoElement() called in
Element::recaclStyle() by calling Element::updateFirstLetter() in the
case the PseudoElement is firstLetter as is done in
Element::updateFirstLetter() to ensure we don't call
Element::retachLayoutTree() uncoditionally.

Adds crash test and expectation as well.

BUG= 700655 

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

[add] https://crrev.com/6d202487ff823def4672599f51e9f13880e6b877/third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects-expected.html
[add] https://crrev.com/6d202487ff823def4672599f51e9f13880e6b877/third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html
[modify] https://crrev.com/6d202487ff823def4672599f51e9f13880e6b877/third_party/WebKit/Source/core/dom/Element.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 6 2017

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

commit 7773939ef302371795ef39138e10225f3d493aa1
Author: foolip <foolip@chromium.org>
Date: Thu Apr 06 10:18:46 2017

Revert of Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject (patchset #4 id:60001 of https://codereview.chromium.org/2752163003/ )

Reason for revert:
The added test is failing on many platforms due to a mismatch between actual expected (not a crash).

BUG= 708967 

Original issue's description:
> Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject
>
> This patch edits Element::rebuildPseudoElementLayoutTree() to make sure
> it mirrors Element::updatePseudoElement() called in
> Element::recaclStyle() by calling Element::updateFirstLetter() in the
> case the PseudoElement is firstLetter as is done in
> Element::updateFirstLetter() to ensure we don't call
> Element::retachLayoutTree() uncoditionally.
>
> Adds crash test and expectation as well.
>
> BUG= 700655 
>
> Review-Url: https://codereview.chromium.org/2752163003
> Cr-Commit-Position: refs/heads/master@{#462373}
> Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9f13880e6b877

TBR=esprehn@chromium.org,nainar@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 700655 

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

[delete] https://crrev.com/9e51d07999515ab5f8567ba8b446e316137eb0b8/third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects-expected.html
[delete] https://crrev.com/9e51d07999515ab5f8567ba8b446e316137eb0b8/third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html
[modify] https://crrev.com/7773939ef302371795ef39138e10225f3d493aa1/third_party/WebKit/Source/core/dom/Element.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 7 2017

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

commit 9e923986fcf79015b0ff237c06207648f8606560
Author: nainar <nainar@chromium.org>
Date: Fri Apr 07 00:23:39 2017

Clear pseudoElement from ElementRareData when FirstLetterPseudoElement doesn't have a LayoutObject

This patch edits Element::rebuildPseudoElementLayoutTree() to make sure
it mirrors Element::updatePseudoElement() called in
Element::recaclStyle() by calling Element::updateFirstLetter() in the
case the PseudoElement is firstLetter as is done in
Element::updateFirstLetter() to ensure we don't call
Element::retachLayoutTree() uncoditionally.

Adds crash test and expectation as well.

BUG= 700655 

Review-Url: https://codereview.chromium.org/2752163003
Cr-Original-Commit-Position: refs/heads/master@{#462373}
Committed: https://chromium.googlesource.com/chromium/src/+/6d202487ff823def4672599f51e9f13880e6b877
Review-Url: https://codereview.chromium.org/2752163003
Cr-Commit-Position: refs/heads/master@{#462705}

[add] https://crrev.com/9e923986fcf79015b0ff237c06207648f8606560/third_party/WebKit/LayoutTests/fast/css/crash-in-attachFirstLetterTextLayoutObjects.html
[modify] https://crrev.com/9e923986fcf79015b0ff237c06207648f8606560/third_party/WebKit/Source/core/dom/Element.cpp

Project Member

Comment 10 by ClusterFuzz, Apr 9 2017

ClusterFuzz has detected this issue as fixed in range 458746:463137.

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

Fuzzer: marty_html_twiddler
Job Type: mac_asan_chrome
Platform Id: mac

Crash Type: UNKNOWN READ
Crash Address: 0x000000000000
Crash State:
  blink::FirstLetterPseudoElement::attachFirstLetterTextLayoutObjects
  blink::Node::reattachLayoutTree
  blink::Element::rebuildLayoutTree
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=454815:454828
Fixed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=458746:463137

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv96h2GQY-jiXONNEKMOWz0UAjb5onHkHQwtsg0D49s4f5Smokx-rlw_89KrHmK6gnJyCPxkPH6DdNET18NS-lazt-l4UxuAyIH45JZJf4JmguwfdK4HDUvDdswWvCXFJm-Pxay6mmxqa04gx0pb3WkdWC1KhhjElAYRCa053ntzqMA1iZFvy3KR4bKv-IY2a6zohKZ-WAbndNNZc8hzqDNNS92aiEwMMVXbXBSPIEUQFpGDtZ34-VHpWfIFc0kGNmUns8X9eW8JOKgy1xz1ZGT_WNFA8Ww0SDafrjk-OImTLtHdGwItflEWpV8h4d2ieeEc8W7Gatc-aqK3-rQbqxHkt_JDIvhW4gkdy4p8cGC6_CzFXobE?testcase_id=6492939313479680


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.
Project Member

Comment 11 by ClusterFuzz, Apr 9 2017

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

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

Sign in to add a comment