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

Issue 835631 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Security CHECK failure: !cached_item->IsTombstone() in paint_controller.cc

Project Member Reported by ClusterFuzz, Apr 22 2018

Issue description

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

Fuzzer: ochang_domfuzzer
Job Type: windows_asan_content_shell
Platform Id: windows

Crash Type: Security CHECK failure
Crash Address: 
Crash State:
  !cached_item->IsTombstone() in paint_controller.cc
  blink::PaintController::CopyCachedSubsequence
  blink::PaintController::UseCachedSubsequenceIfPossible
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=537543:537544

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Apr 22 2018

Components: Blink>Paint
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Comment 2 by narjan...@gmail.com, Apr 22 2018

Narjane99@gmail.com
Cc: brajkumar@chromium.org
Labels: -Type-Bug M-66 Test-Predator-Wrong Type-Bug-Regression
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.

Using Code Search for the file, "paint_controller.cc" suspecting the below Cl might have caused this issue

Suspect CL: https://chromium.googlesource.com/chromium/src/+/d5803f330a4355c1d11e86e388d1bbb2e25b2b08%5E%21/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp

wangxianzhu@ -- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks!

Reproduced this with
  out/Release/content_shell --content-shell-host-window-size=1830x361 --run-layout-test fuzz-112-viewspec-viewtargetparams.html
Cc: mstensho@chromium.org chrishtr@chromium.org
Reduced test case attached.

This is a weird case:
- The HTML element is multi-column;
- The BODY element is paged and column-span:all;
- The OBJECT element is painted multi-paged, but it doesn't know because its PaginationContainer() is nullptr, and the content HTML creates subsequences.

The OBJECT element's layer has null PaginationContainer() because we found it's under a column span: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_flow_thread.cc?rcl=b41c63db5b5efa88c4205225cd329a44a18b3492&l=69. Should we continue the outer loop because there might be paged-y container?



tombstone.html
191 bytes View Download
Neat! I haven't debugged it, but here's what it looks like to me: Paged overflow on BODY is propagated to the viewport, just like any other overflow value. So the paged container should actually end up being established by LayoutView, not by BODY. So the paged container actually becomes the ancestor of the of the multicol container, not the other way around. BODY should still be remain a spanner (as a regular block, not a paged container), though. If my guess is correct, you should be able to create the same situation with regular DIVs (rather than BODY and HTML): A paged container with a multicol child with a spanner child, with this OBJECT thing inside.

Regarding the code you point to: The containing flow thread of BODY and its children should ideally be the paged container. This is bug 757947 (that we just return nullptr like this), which I'm not sure if it's a good idea to fix.

Just assign this one to me if you don't have any reasonable work-around on the paint side for it.
Is it the HTML element which creates multiple subsequences? How exactly?
It's the HTML element in the subdocument in the OBJECT element creating multiple subsequences.

We have the mechanism to prevent the issue in subdocuments painted in fragments: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/embedded_content_painter.cc?rcl=73a64ffde3f3b64df576aa1f2b5baebf7ec964ba&l=35, but in this case, the OBJECT element doesn't know it's painted in fragments because its EnclosingPaginationLayer() is null.
I see...nested multicol plus column-span: all..

I'd prefer to fix the bug by plumbing the fact that there is some ancestor with multicol
to EmbeddedContentPainter, even if the parent is not. It seems we could do that more easily
and safer than fully fixing issue 757947.

Do you mean we always walk up the ancestor chain to see if the object has a LayoutFlowThread ancestor? I think that would degrade performance.

I think regardless the root cause, https://chromium-review.googlesource.com/c/chromium/src/+/1034114 can be a fail-safe for duplicated subsequences. It can avoid the security CHECK, though still fails DCHECKs. As the test case is unlikely to exist in real world, I think DCHECK failure is fine, and I'd prefer to leave the root cause to bug 757947.
Ok.
Project Member

Comment 12 by bugdroid1@chromium.org, May 1 2018

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

commit ecad646aff6cc66dbb18e785d3b203bdde7a27e0
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue May 01 04:40:46 2018

[PE] Avoid SECURITY_CHECK failure on duplicated subsequences

For now in rare cases a client may create multiple subsequences, e.g.
the bug case. Previously this led to a SECURITY_CHECK failure.

Bug:  835631 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ifb3ef4b9e00d3ef00b4f3666120b7399733cfb9b
Reviewed-on: https://chromium-review.googlesource.com/1034114
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554990}
[modify] https://crrev.com/ecad646aff6cc66dbb18e785d3b203bdde7a27e0/third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
[modify] https://crrev.com/ecad646aff6cc66dbb18e785d3b203bdde7a27e0/third_party/blink/renderer/platform/graphics/paint/paint_controller_test.cc

Project Member

Comment 13 by ClusterFuzz, May 1 2018

ClusterFuzz has detected this issue as fixed in range 554989:554991.

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

Fuzzer: ochang_domfuzzer
Job Type: windows_asan_content_shell
Platform Id: windows

Crash Type: Security CHECK failure
Crash Address: 
Crash State:
  !cached_item->IsTombstone() in paint_controller.cc
  blink::PaintController::CopyCachedSubsequence
  blink::PaintController::UseCachedSubsequenceIfPossible
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=537543:537544
Fixed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=554989:554991

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

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 14 by ClusterFuzz, May 1 2018

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

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

Comment 15 by bugdroid1@chromium.org, Jun 5 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f5e98ab81a127dbc59fe87205e0082dcf41773b

commit 5f5e98ab81a127dbc59fe87205e0082dcf41773b
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Jun 05 21:34:22 2018

[PE] Avoid SECURITY_CHECK failure on duplicated subsequences

For now in rare cases a client may create multiple subsequences, e.g.
the bug case. Previously this led to a SECURITY_CHECK failure.

TBR=wangxianzhu@chromium.org

(cherry picked from commit ecad646aff6cc66dbb18e785d3b203bdde7a27e0)

Bug:  835631 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ifb3ef4b9e00d3ef00b4f3666120b7399733cfb9b
Reviewed-on: https://chromium-review.googlesource.com/1034114
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#554990}
Reviewed-on: https://chromium-review.googlesource.com/1087790
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#754}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/5f5e98ab81a127dbc59fe87205e0082dcf41773b/third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
[modify] https://crrev.com/5f5e98ab81a127dbc59fe87205e0082dcf41773b/third_party/blink/renderer/platform/graphics/paint/paint_controller_test.cc

Merged for bug 847120.

Sign in to add a comment