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

Issue 750479 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::BaselineContext::FindCompatibleSharedGroup

Project Member Reported by ClusterFuzz, Jul 30 2017

Issue description

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

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  blink::BaselineContext::FindCompatibleSharedGroup
  blink::BaselineContext::GetSharedGroup
  blink::LayoutGrid::GetBaselineGroupForChild
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=472654:472713

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


Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: msrchandra@chromium.org
Labels: M-62 Test-Predator-Wrong
Owner: jfernandez@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL did not provide any possible suspects.
Using Code Search for the file, "BaselineAlignment.cpp" assigning to the concern owner.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/45688c2bcf5aedeff66d2d427e099d5f65251849

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

Comment 2 by ClusterFuzz, Aug 12 2017

Labels: OS-Mac
Owner: jfernan...@igalia.com
I'll take a look ASAP
Attached a minimized test case to reproduce the crash.
issue-750479-crash.html
336 bytes View Download
Setting a longer timeout makes more noticeable what action triggers the crash. 

It seems that adding the c29 CSS class, which sets the 'content' property value to an empty string (any string will produce the same result), triggers some internal actions that lead to a crash during layout.
Still speculating about the root cause of the crash, but it seems clear that the problem comes from the fact that both grid have auto-sized tracks and the nested grid has indefinite width.  
Another interesting point is that the nested grid's child is orthogonal. Otherwise the crash is not reproducible. 
I think I finally found out the actual root cause of the crash. It seems that changes in the 'content' property implies a Style reattach. 

When applied to the nested grid's child, it makes such grid is marked as dirty, hence, needing to execute again the items placement logic.

After Attaching the new style, a new layout is performed on the whole tree, which implies computing again the nested grid's intrinsic size. This computation involves a new run of the track sizing algorithm. Since we have auto-sized tracks, the MinContentForChild logic is executed, which ends up trying to reuse the Baseline extent computed in the previous layout; we only do this when dealing with orthogonal items while computing the column track width. 

In order to figure out the Baseline extent for a child, we need to know which Baseline grout it belongs to. For this, we need to know which grid area it's placed into. Since the grid has been marked as dirty, and it still need to be laid out, the items placement logic has not been executed yet, hence there is no way to gather the Baseline related information. Actually, the grid logic has an assertion to verify the grid area structures are not used before the item's placement logic is executed. This is precisely the assert triggered by the attached test case.
Components: Blink>Layout>Grid
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 31 2017

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

commit 1b9a649524e74b55b4752fa7f5a545c9079bc541
Author: Javier Fernandez <jfernandez@igalia.com>
Date: Thu Aug 31 16:10:13 2017

[css-grid] Clearing baseline context structures when grid is dirty

When the grid is marked as dirty the Grid data structures are cleared
and it must execute again the items placement logic.

Style changes applied to certain properties will cause a style 
Reattach,hence the affected layout tree must be built again. This 
obviously implies the grid must be marked as dirty.

The LayoutGrid class holds some data structures to implement the logic
for the Baseline Alignment of its Grid items. We are not clearing these
structures when the grid is marked as dirty; instead, we do it at the
beginning of the layout operation. This is enough in most of the cases,
however, this issue proves otherwise. 
 
Another important factor to understand the issue is that we use the
track sizing algorithm to compute the grid's intrinsic size. This 
operation has to be done without doing a layout of the grid itself. 

Additionally, when dealing with orthogonal items we use the baseline
offsets if they are available, because they affect to the grid's 
intrinsic size. In order to gather the Baseline data of a grid item 
it's needed to determine the Baseline Group such item belongs to. For
that, it's also necessary to know which grid area the item is placed
in.

When the above mentioned style changes are applied to a nested grid 
we mark it as dirty, hence its basic Grid structures are cleared. As
it's also a grid item, we may need to compute its intrinsic size as
part of the layout logic on its parent grid. This computation will
imply running the whole track sizing algorithm (under intrinsic size
mode), which finally tries to use the baseline data (because of the
orthogonal children). Since we haven't performed the layout of the
nested grid, we still have the baseline data structures. This will
lead to trigger the assertion described in the bug.   

Bug:  750479 
Change-Id: If70ed652cb86ed4e86c1033a605ca276d1344c51
Reviewed-on: https://chromium-review.googlesource.com/630456
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Sergio Villar <svillar@igalia.com>
Cr-Commit-Position: refs/heads/master@{#498879}
[add] https://crrev.com/1b9a649524e74b55b4752fa7f5a545c9079bc541/third_party/WebKit/LayoutTests/fast/css-grid-layout/changing-content-property-on-nested-grid-should-not-crash-expected.txt
[add] https://crrev.com/1b9a649524e74b55b4752fa7f5a545c9079bc541/third_party/WebKit/LayoutTests/fast/css-grid-layout/changing-content-property-on-nested-grid-should-not-crash.html
[modify] https://crrev.com/1b9a649524e74b55b4752fa7f5a545c9079bc541/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Status: Fixed (was: Assigned)
This issue should be FIXED now.
Project Member

Comment 12 by ClusterFuzz, Sep 1 2017

ClusterFuzz has detected this issue as fixed in range 498874:498922.

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

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  blink::BaselineContext::FindCompatibleSharedGroup
  blink::BaselineContext::GetSharedGroup
  blink::LayoutGrid::GetBaselineGroupForChild
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=472654:472713
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=498874:498922

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

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

Comment 13 by ClusterFuzz, Sep 1 2017

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

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

Sign in to add a comment