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

Issue 802970 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug



Sign in to add a comment

null check should be done on document paint definition

Project Member Reported by xidac...@chromium.org, Jan 17 2018

Issue description

In a few methods of CSSPaintImageGeneratorImpl class, we never do null check on document paint definition, and that could lead to a crash.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 19 2018

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

commit c1f66677277a28a186de6251d31b77fb921f7769
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Jan 19 06:46:11 2018

[PaintWorklet] Do null check for document paint definition

In quite a few functions in the CSSPaintImageGeneratorImpl class, we
check whether the document paint definition of a particular name exists
or not, but we never do null check on that definition. This could lead
to crash if the document paint definition is a null ptr.

This CL fixes the problem by doing null check. It also changes one
layout test to execise this code path. The change to the layout test
also fixes the problem that this layout test passes in the browser
that doesn't support paint worklet.

Bug:  802970 ,  768683 
Change-Id: Ia952ad977b63af643410b0973cc8034fac504f9f
Reviewed-on: https://chromium-review.googlesource.com/869891
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530441}
[modify] https://crrev.com/c1f66677277a28a186de6251d31b77fb921f7769/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/parse-input-arguments-018-ref.html
[modify] https://crrev.com/c1f66677277a28a186de6251d31b77fb921f7769/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/parse-input-arguments-018.https.html
[modify] https://crrev.com/c1f66677277a28a186de6251d31b77fb921f7769/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.cpp
[modify] https://crrev.com/c1f66677277a28a186de6251d31b77fb921f7769/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h

Labels: -Pri-3 Merge-Request-65 OS-Linux OS-Mac OS-Windows Pri-1
Cc: gov...@chromium.org ram...@chromium.org
My CL that fixes the crash was landed after M65 branch point, so this should be merged to M65 to prevent crash. Please let me know the branch number.

Comment 4 by gov...@chromium.org, Jan 22 2018

How is the change looking in canary?
govind@: I have a test case that used to crash due to this bug, right now it is crashing due to  crbug.com/803026 , should I fix that bug first and do the merge?

Comment 6 by gov...@chromium.org, Jan 22 2018

Pls  crbug.com/803026  first and request a merge to M65. Pls wait for approval. Thank you.
Project Member

Comment 7 by sheriffbot@chromium.org, Jan 23 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Comment 8 by bugdroid1@chromium.org, Jan 24 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/629ff5d659938c0fb76eede6c54d223a0334f00c

commit 629ff5d659938c0fb76eede6c54d223a0334f00c
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Jan 24 14:42:42 2018

[PaintWorklet] Do null check for document paint definition

In quite a few functions in the CSSPaintImageGeneratorImpl class, we
check whether the document paint definition of a particular name exists
or not, but we never do null check on that definition. This could lead
to crash if the document paint definition is a null ptr.

This CL fixes the problem by doing null check. It also changes one
layout test to execise this code path. The change to the layout test
also fixes the problem that this layout test passes in the browser
that doesn't support paint worklet.

Bug:  802970 ,  768683 
Change-Id: Ia952ad977b63af643410b0973cc8034fac504f9f
Reviewed-on: https://chromium-review.googlesource.com/869891
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#530441}(cherry picked from commit c1f66677277a28a186de6251d31b77fb921f7769)
Reviewed-on: https://chromium-review.googlesource.com/883763
Reviewed-by: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#59}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/629ff5d659938c0fb76eede6c54d223a0334f00c/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/parse-input-arguments-018-ref.html
[modify] https://crrev.com/629ff5d659938c0fb76eede6c54d223a0334f00c/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/parse-input-arguments-018.https.html
[modify] https://crrev.com/629ff5d659938c0fb76eede6c54d223a0334f00c/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.cpp
[modify] https://crrev.com/629ff5d659938c0fb76eede6c54d223a0334f00c/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h

Status: Fixed (was: Assigned)
Merged to M65, closing this bug.

Sign in to add a comment