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

Hi Max, Daniel, Emil,

Could one of you please help triage this security bug?  Feel free to adjust labels and re-assign as appropriate.

Similar to https://bugs.chromium.org/p/chromium/issues/detail?id=610985.  Max, you were looking into this other bug and closed it, I'll leave this one with you.  Feel free to adjust labels and re-assign.

CC'ing dcheng, who closed down this other exact same stack trace back in December: https://bugs.chromium.org/p/chromium/issues/detail?id=559528.

Thanks!

The testcases is marked as non reproducible (https://cluster-fuzz.appspot.com/testcase?key=5462547098763264).

[2016-09-18 11:42:09] mmoroz@google.com: Redo task(s): progression
[2016-09-18 04:53:25] clusterfuzz-windows-0004: Progression task started: r419394.
[2016-09-18 05:01:02] clusterfuzz-windows-0004: Progression task in-progress: Testing r418926:r419394.
[2016-09-18 05:01:15] clusterfuzz-windows-0004: Progression task errored out: Known crash revision 418926 did not crash.
[2016-09-18 05:01:15] clusterfuzz-windows-0004: Progression task errored out: Test case appears to be flaky.


I'd prefer to close it as a flaky non reproducible crash. 

 Issue 649510  has been merged into this issue.

 Issue 665168  has been merged into this issue.

This bug is still happening sporadically (https://cluster-fuzz.appspot.com/testcase?key=6567724890456064) - it is probably worth further investigation even if it's hard to reproduce.

Note that clusterfuzz has autoclosed these bugs in the past - this appears to be because the test is flaky, not because the bug has been fixed.

+awhalley@, this is reported as M55 Beta blocker. Should we block this week beta release on Wednesday (11/16)?

Given its flaky and been around a while I'm OK with moving it to ReleaseBlock-Stable.

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!

Also due to Thanksgiving holidays in US, please make sure fix is ready and merged to M55 latest by 5:00 PM PT Friday, 11/18/16 (sooner the better).

mmoroz: Uh oh! This issue still open and hasn't been updated in the last 58 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

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

I've just reproduce this crash https://cluster-fuzz.appspot.com/v2/testcase-detail/6567724890456064

on the following revision:
306338a1ac90527e74f9604cd5b202d1cdf2cb63
Cr-Commit-Position: refs/heads/master@{#432156}

using the following GN flags (some of them are redundant, but I put all here just for the record):

is_debug=false
use_libfuzzer=true
is_asan=true
enable_nacl=false
pdf_enable_xfa=true
proprietary_codecs=true
ffmpeg_branding="ChromeOS"
use_goma = true
optimize_for_fuzzing = true
build_sfntly_samples=true


and using the following command:
./content_shell --run-layout-test --dump-render-tree --use-gl=osmesa --disable-gl-drawing-for-tests --enable-experimental-web-platform-features fuzz-80.html



eae@, since we have a reproducible testcase, could please help to find an owner?

Would you be able to look into this first-letter text bug kojii? It's a P1 security bug with a working repro (see comment 15 for repro steps).

This looks like a lifecycle issue, the Free stack indicates we're doing something bad. Some key call flows below.

1. Removing a node from tree calls PseudoElementData::clearPseudoElements
2. PseudoElement::dispose
3. FirstLetterPseudoElement::detachLayoutTree
4. LayoutTextFragment::setTextFragment
5. LayoutText::setText
6. AXObjectCacheImpl::textChanged
7. AXNodeObject::determineAccessibilityRole
8. AXNodeObject::computeParent
9. AXNodeObject::determineAccessibilityRole
10. Document::updateStyleAndLayoutTreeForNode
11. Element::recalcOwnStyle
12. Node::reattachLayoutTree
13. Element::detachLayoutTree
14. LayoutObject::destroy
15. LayoutTextFragment::`

A few questionable call flows:
1. Why do we need to setText while disposing?
2. Why does AXObject computes role on textChanged (rather than invalidate and compute when needed)?
3. Why does determineAccessibilityRole needs to update layout tree?

#1 is questionable, but still LayoutText::setText triggering updateLayout looks quite bad, there might be other cases we call setText during layout.
#2 looks nice to do regardless of this crash, but I wonder whether it is the right way to fix the crash or not.
#3 was added in https://codereview.chromium.org/1667623002. I'm not sure if this call is avoidable.

I'll continue to look into further.

Though the stack shows suspicious call flows, the repro in comment #15 no longer reproduces.

Unfortunately out of time for M55, given the flakiness. We can consider merging once we get a fix.

The  issue 665168  looks duplicate of this issue, but I don't have access to it. Can someone add me, or resolve as duplicate to this bug?

kojii@, I cc'ed you on  issue 665168 .

Thanks, understood it's already resolved as duplicated.

Delaying determineAccessibleRole() doesn't seem to work well, since roleValue() is used in so many places that it is likely to be called even if we try to delay a bit. Discarding the WIP CL
https://codereview.chromium.org/2530343002

After trying a few approaches, I think it's best for AXNodeObject to avoid updating layout tree. Hope to send to review soon. The new WIP here
https://codereview.chromium.org/2532023002

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

commit 338b38e06760302a8010bfea008865f55db4db0c
Author: kojii <kojii@chromium.org>
Date: Tue Nov 29 16:20:12 2016

Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369,  647602 ,  665168 

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

[modify] https://crrev.com/338b38e06760302a8010bfea008865f55db4db0c/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

Though the fix is speculative, I think this is fixed now. Please reopen if clusterfuz could find any similar cases in future.

Your change meets the bar and is auto-approved for M56 (branch: 2924)

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

commit 13ae3f245af2124718f024ae13f328c27c618f09
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Dec 01 08:28:25 2016

Merge 2924: Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369,  647602 ,  665168 

Review-Url: https://codereview.chromium.org/2532023002
Cr-Commit-Position: refs/heads/master@{#435009}
(cherry picked from commit 338b38e06760302a8010bfea008865f55db4db0c)

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

Cr-Commit-Position: refs/branch-heads/2924@{#241}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/13ae3f245af2124718f024ae13f328c27c618f09/third_party/WebKit/Source/modules/accessibility/AXNodeObject.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