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

Issue 660273 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

CSSParser::parseSingleValue is inconsistent in handling css-wide keywords

Project Member Reported by timloh@chromium.org, Oct 28 2016

Issue description

CSSParser::parseSingleValue first calls the fast path, and if the fast path fails it calls into CSSPropertyParser::parseSingleValue. The fast path supports the strings "initial" and "inherit" but the slow path does *not* support any css-wide keywords. So CSSParser::parseSingleValue is allowing the strings "initial"/"inherit" but not the other css-wide keywords, or usage of escapes.

From a brief look at the callers it seems like everything ends up handling it correctly, but we should fix this.
 
Labels: Update-Quarterly

Comment 2 by meade@chromium.org, Jun 20 2017

Cc: bugsnash@chromium.org
Hey Bugs! it's this again! Perhaps something to consider with Ribbon.

Comment 3 by nainar@chromium.org, Jun 30 2017

Labels: Hotlist-Squash-A-Bug
Owner: rob.b...@samsung.com
Status: Assigned (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 13 2017

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

commit 778279c9d06879103f0f0698849eff541204e159
Author: Rob Buis <rob.buis@samsung.com>
Date: Wed Sep 13 15:47:24 2017

Support unset in CSSParserFastPaths

Support unset as a CSS wide keyword [1] and add some unit tests for it.

[1] https://www.w3.org/TR/css3-values/#common-keywords

Bug: 660273
Change-Id: Ic739055b83988017e11ff3968c1a0aad48073613
Reviewed-on: https://chromium-review.googlesource.com/663820
Commit-Queue: Rob Buis <rob.buis@samsung.com>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501642}
[modify] https://crrev.com/778279c9d06879103f0f0698849eff541204e159/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
[modify] https://crrev.com/778279c9d06879103f0f0698849eff541204e159/third_party/WebKit/Source/core/css/parser/CSSParserFastPathsTest.cpp

Cc: timloh@chromium.org
I wonder if anything is left for this bug? I am pretty sure the slow path does  support css-wide keywords these days through ConsumeCSSWideKeyword.
If so, the only thing I am not sure about is the "usage of escapes" comment.

Comment 7 by timloh@chromium.org, Sep 14 2017

I think this is still an issue, let me try to clarify. I think your patch would've changed how BaseRenderingContext2D::setFilter() handles unset and maybe it's not correct now?

The call tree (for example) looks like this:

BaseRenderingContext2D::setFilter()
-> CSSParser::ParseSingleValue
-1-> CSSParserFastPaths::MaybeParseValue (handles "initial", "inherit", "unset")
-2-> CSSPropertyParser::ParseSingleValue (handles "blur(10px)", not "initial")

In the CSSPropertyParser, CSS-wide keywords are handled in ParseValueStart. So CSSParser::ParseSingleValue() will allow those handled by the fast-path (e.g. "initial"), but not cases it misses like "initi\61 l". So the API is somewhat broken because it seems to support css-wide keywords but if escapes are used it will reject.

So we need to decide on whether this function will support or reject css-wide keywords (no opinion here since I haven't worked on or looked at style code since January). I think all the callers used to explicitly rejected them, but it seems like CSSStyleValue.parse now wants to accept them:

> CSS.supports("filter", "initial")
true
> CSS.supports("filter", "initi\\61 l")
true
> CSSStyleValue.parse("filter", "initial")
CSSKeywordValue {keywordValue: "initial", cssText: "initial"}
> CSSStyleValue.parse("filter", "initi\\61 l")
null
Thanks Tim for the explanation!

>I think this is still an issue, let me try to clarify. I think your patch would've changed how BaseRenderingContext2D::setFilter() handles unset and maybe it's not correct now?

Yes from messing around with inspector it behaves differently from initial/inherit when setting the filter attribute. The callsite should just use IsCSSWideKeyword, working on a patch.

>The call tree (for example) looks like this:

>BaseRenderingContext2D::setFilter()
> -> CSSParser::ParseSingleValue
> -1-> CSSParserFastPaths::MaybeParseValue (handles "initial", "inherit", "unset")
> -2-> CSSPropertyParser::ParseSingleValue (handles "blur(10px)", not "initial")

> In the CSSPropertyParser, CSS-wide keywords are handled in ParseValueStart. So CSSParser::ParseSingleValue() will allow those handled by the fast-path (e.g. "initial"), but not cases it misses like 
> "initi\61 l". So the API is somewhat broken because it seems to support css-wide keywords but if escapes are used it will reject.

>So we need to decide on whether this function will support or reject css-wide keywords (no opinion here since I haven't worked on or looked at style code since January). I think all the callers used to >explicitly rejected them, but it seems like CSSStyleValue.parse now wants to accept them:

> CSS.supports("filter", "initial")
true
> CSS.supports("filter", "initi\\61 l")
true
> CSSStyleValue.parse("filter", "initial")
CSSKeywordValue {keywordValue: "initial", cssText: "initial"}
> CSSStyleValue.parse("filter", "initi\\61 l")
null

Sounds to me like it would be useful to know how CSSStyleValue.parse should behave in above case. Should I write the mailing list or does somebody else volunteer? :)
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 18 2017

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

commit 5c9be53b404ca9b9386200357003d8037005f804
Author: yiyix <yiyix@chromium.org>
Date: Mon Sep 18 09:23:39 2017

Simplify logic in UIResourceLayerImpl::AppendQuad 

In UIResourceLayerImpl::AppendQuad, we are always retrieving resource 
Id from UIResourceId regardless of validation of UIResourceId. I added 
a validation check prior of calling ResourceIdForUIResource in this cl. 

Bug: 660273
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Idda682cef70823520300e2842de1f488932bd285
Reviewed-on: https://chromium-review.googlesource.com/664960
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Yi Xu <yiyix@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502540}
[modify] https://crrev.com/5c9be53b404ca9b9386200357003d8037005f804/cc/layers/ui_resource_layer_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 19 2017

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

commit 2491a1d2ded89d2cc3f52856524411042488139a
Author: Rob Buis <rob.buis@samsung.com>
Date: Tue Sep 19 13:51:45 2017

Fix callsites of CSSParser::ParseSingleValue

After #501642 some callsites behave differently because they
only tested for initial/inherit, not unset. So use
IsCSSWideKeyword instead.

Bug: 660273
Change-Id: Ibe8842ac21da804c43104fd978983d84c9f65001
Reviewed-on: https://chromium-review.googlesource.com/667865
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Rob Buis <rob.buis@samsung.com>
Cr-Commit-Position: refs/heads/master@{#502843}
[modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/LayoutTests/fast/canvas/canvas-filter-value.html
[modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/Source/core/html/canvas/CanvasFontCache.cpp
[modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp

Labels: -Update-Quarterly

Comment 12 by e...@chromium.org, Apr 23 2018

Owner: ----
Status: Available (was: Assigned)

Sign in to add a comment