Null-dereference READ in cc::Layer::transform_tree_index |
|||||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5745150130388992 Fuzzer: ifratric-browserfuzzer-v3 Job Type: mac_asan_chrome Platform Id: mac Crash Type: Null-dereference READ Crash Address: 0x000000000020 Crash State: cc::Layer::transform_tree_index void cc::BuildPropertyTreesInternal<cc::Layer> void cc::BuildPropertyTreesInternal<cc::Layer> Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=479094:479114 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5745150130388992 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Jul 17 2017
I've kicked off a re-do of almost all steps here. It seems this is not reproducible so maybe it has been fixed.
,
Jul 18 2017
Clusterfuzz came back with some updated numbers. Looks like this was a regression from https://chromium.googlesource.com/chromium/src/+/dfd95a106eaaf3a5e6712f8f21e9979dfda4aa16. I've attached a smaller testcase that still requires some timing to hit. Rob, could you look into this? I don't think it was actually caused by Chris's patch.
,
Jul 22 2017
,
Aug 1 2017
,
Aug 4 2017
Thanks for the repro case. It looks like we haven't updated the sticky position constraints after removing the intermediate sticky layer shifting the sticky containing block. Investigating what's gone wrong.
,
Aug 6 2017
The layer for a sticky <html> element is getting squashed but still referenced from a child layer as the ancestor layer shifting the containing block. This isn't supposed to happen, we should be adding the kCompositingReasonPositionFixedOrStickyWithCompositedDescendants reason to promote any ancestors with descendant promoted sticky layers. Still investigating how we didn't find this reason.
,
Aug 6 2017
I found the cause and have a fix at https://chromium-review.googlesource.com/603148. I'm just working out a good test. In https://codereview.chromium.org/2929873002 we changed when we set sticky position constraints to not do anything if the scroller wasn't scrollable - however this means that we would never clear the sticky position constraints on a previously sticky composited layer. These constraints could then reference an ancestor shifting sticky layer which could become no longer composited while the descendant remains composited for another reason. This would leave a reference to a layer which doesn't exist and so we crash.
,
Aug 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9b28a24206db91f827363e29b669d990359dcdc commit b9b28a24206db91f827363e29b669d990359dcdc Author: Robert Flack <flackr@chromium.org> Date: Wed Aug 09 15:11:54 2017 Clear sticky position constraints if a sticky layer no longer moves. When a scrollable area is no longer scrollable we stop setting composited sticky position parameters on composited sticky layers and stop promoting them. We need to ensure that we correctly clear a previously set sticky position parameter as it may reference an ancestor layer assuming it is also promoted which may no longer exist. BUG= 744061 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I9b5a8cf9470b91f44f4db121b282d8a5c8daf13c Reviewed-on: https://chromium-review.googlesource.com/603148 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Robert Flack <flackr@chromium.org> Cr-Commit-Position: refs/heads/master@{#492985} [modify] https://crrev.com/b9b28a24206db91f827363e29b669d990359dcdc/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp [modify] https://crrev.com/b9b28a24206db91f827363e29b669d990359dcdc/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
,
Aug 9 2017
Requesting merge since the CL which introduced the potential crash was in 61.
,
Aug 9 2017
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 9 2017
Before we approve merge to M61, please answer followings: * Is this M61 regression? Is it critical? * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61? * Any other important details to justify the merge. Please note M61 is already in Beta, so merge bar is very high. Thank you.
,
Aug 10 2017
ClusterFuzz has detected this issue as fixed in range 492866:493054. Detailed report: https://clusterfuzz.com/testcase?key=5745150130388992 Fuzzer: ifratric-browserfuzzer-v3 Job Type: mac_asan_chrome Platform Id: mac Crash Type: Null-dereference READ Crash Address: 0x000000000020 Crash State: cc::Layer::transform_tree_index void cc::BuildPropertyTreesInternal<cc::Layer> void cc::BuildPropertyTreesInternal<cc::Layer> Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=479094:479114 Fixed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=492866:493054 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5745150130388992 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Aug 10 2017
ClusterFuzz testcase 5745150130388992 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Aug 11 2017
Re #12, this is an M61 crash regression though I'm not sure how frequent it is. The change just landed 2 days ago but is a pretty minor change. It may be worth giving it a few days in canary before merging.
,
Aug 11 2017
Thank you flackr@. OK, lets wait until Monday. Please update the bug with Canary result.
,
Aug 14 2017
The NextAction date has arrived: 2017-08-14
,
Aug 14 2017
Regarding whether it is critical, I've only managed to find one crash that matches this: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20stable_signature%20CONTAINS%20%27transform_tree_index%27%20AND%20product.Version%20CONTAINS%20%2761%27%20AND%20ReportID%3D%2718fe9ce178000000%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=&index=0. I'm not sure whether there would be more when this hits stable? Canary seems to have accepted my change with no additional crashes so far.
,
Aug 14 2017
Thank you flackr@. Per comment #13, so far only one crash is reported due to this. + abdulsyed@, is it worth to take this merge in for M61?
,
Aug 14 2017
Rejecting merge to M61 after discussing with abdulsyed@. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by msrchandra@chromium.org
, Jul 17 2017Components: Internals>Compositing
Labels: Test-Predator-Wrong-CLs
Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)