1112014 - inDOMUtils.CssPropertySupportsType returns false negatives for some properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

e.g.
DOMUtils.CssPropertySupportsType("box-shadow", DOMUtils.TYPE_COLOR) returns false.

This is because some CSS properties use a custom parser. Validating property values through the parser instead of simply checking the type will be a lot more reliable.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1112014-fix-cssPropertySupportsType.patch

Just a few words by way of explanation:
- Putting strings through the parser is more accurate than checking parser types. It is still not perfect as it will give false negatives for properties that require a color plus something else. Still a lot more accurate than we currently have. This is useful for another patch I have which will improve CSS autocompletion via GetCSSValuesForProperty().
- I noticed that IsValueValidForProperty() fails for property values containing URLs as there was no principle passed. I wrote this method and it is only used by domutils so we now assume we are in the system principle when initializing the scanner.

Comment 2

[Cameron McCormack (:heycam)]

Sorry for the delay here.

Another option would be to leave the current variant parser-based heuristics, and add in the box-shadow (and probably other) hard coded cases.  Then you could have a mochitest that checks each of these type of property values against all of the properties in property_database.js, failing if we successfully parse a property but don't return the corresponding flag from GetCSSValuesForProperty.  That would avoid having to do the parsing check at run time.  WDYT?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Sorry for the delay here.
> 
> Another option would be to leave the current variant parser-based
> heuristics, and add in the box-shadow (and probably other) hard coded cases.
> Then you could have a mochitest that checks each of these type of property
> values against all of the properties in property_database.js, failing if we
> successfully parse a property but don't return the corresponding flag from
> GetCSSValuesForProperty.  That would avoid having to do the parsing check at
> run time.  WDYT?

So we would hard code properties that return the wrong types so that we get the correct types? That would work great if I could somehow generate a test that checks for all types but the logic for that is in the first patch.

Yup, I prefer that approach over forcing strings through the parser.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> So we would hard code properties that return the wrong types so that we get
> the correct types? That would work great if I could somehow generate a test
> that checks for all types but the logic for that is in the first patch.

Yeah I think you can basically translate that part into JS.

> Yup, I prefer that approach over forcing strings through the parser.

Great, let's do that then.

Comment 5

[Tom Tromey :tromey]

I didn't really understand the test plan in comment #2.

I did something somewhat ad hoc -- namely, tested the subset of properties we
currently care about, which is those that could possibly hold a color.
I wrote the test in a reasonably extensible way.

I'm happy to rewrite it, but I'd need more guidance.

Barring some sort of introspectable parser, I don't see how to hit the
ideal goal, which would be to ensure that all changes to the css parser
are reflected in inDOMUtils::CssPropertySupportsType.

Comment 6

[Tom Tromey :tromey]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)

> - I noticed that IsValueValidForProperty() fails for property values
> containing URLs as there was no principle passed. I wrote this method and it
> is only used by domutils so we now assume we are in the system principle
> when initializing the scanner.

How can I reproduce this?
I tried:

  ok(utils.cssPropertyIsValid('background', 'url(http://example.com/)'));

... but that passed.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Tom Tromey :tromey from comment #5)
> I didn't really understand the test plan in comment #2.
> 

We don't need property_database.js for this but I know where heycam was coming from. The idea is that we add a mochitest that does something like this:
```
for each property {
  parses = inDOMUtils::CssPropertyIsValid("color: red");
  typeSupported = inDOMUtils::CssPropertySupportsType(TYPE_COLOR);
  is(parses, !typeSupported, "inDOMUtils::CssPropertySupportsTypeProperty(<property>, TYPE_COLOR) returns true. If this test fails then please add a special case for this property in inDOMUtils::CssPropertySupportsType()");

  // We don't care about !parses, typeSupported as this will [hopefully] only happen for shorthand properties that require multiple values e.g. transition.

  // Same check for all other CSS types
}
```

This would give us a reliable list of properties for which we would return a false negative for a particular type. There are some strings in my rejected patch that can be used in the test.

This test should live with the CssParser tests so that we are covered when devs add new properties.

In the event of a property not passing the correct flag we would simply add a special condition for that property e.g.
```
inDOMUtils::CssPropertySupportsType()
{
...
  case TYPE_COLOR:
    if (aProperty == NS_LITERAL_STRING("box-shadow")) {
      *_retval = true;
      return NS_OK;
    }
  break;
...
}
```

> I did something somewhat ad hoc -- namely, tested the subset of properties we
> currently care about, which is those that could possibly hold a color.
> I wrote the test in a reasonably extensible way.
> 

We care about more than just color as we can use the same system to get CSS autocompletion returning the correct properties.

> I'm happy to rewrite it, but I'd need more guidance.
> 

The test is pretty simple so I am happy to write it if you would like me to. If you are starting to regret taking this bug I am more than happy to fix it as I know exactly what needs doing and it won't take me long.

> Barring some sort of introspectable parser, I don't see how to hit the
> ideal goal, which would be to ensure that all changes to the css parser
> are reflected in inDOMUtils::CssPropertySupportsType.

The test I mention above would force devs that have added properties to special case the properties in inDOMUtils::CssPropertySupportsType() when the test detects false negatives. This is a little ugly but the best of our available choices.

(In reply to Tom Tromey :tromey from comment #6)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> 
> > - I noticed that IsValueValidForProperty() fails for property values
> > containing URLs as there was no principle passed. I wrote this method and it
> > is only used by domutils so we now assume we are in the system principle
> > when initializing the scanner.
> 
> How can I reproduce this?
> I tried:
> 
>   ok(utils.cssPropertyIsValid('background', 'url(http://example.com/)'));
> 
> ... but that passed.

Yes, it seems like heycam fixed that:
Bug 1138788 - Allow CSSParserImpl::SetURLValue assertion to succeed when under IsValueValidForProperty, even with no sheet principal.

Comment 8

[Tom Tromey :tromey]

Thanks for your response.  I understand now.

I had an idea for how to write a test that would ensure that future changes to the
CSS parser don't cause regressions in this area.  The gist is to instrument the
parser a tiny bit to record what it does, and then expose this information via
inIDOMUtils.  Then, we can use property_database to exercise the parser, and finally
validate the results against CssPropertySupportsType.

Comment 9

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType

Here is the patch.

I instrumented the CSS parser a tiny bit and added a new API to query
it to see what variants were actually considered during a parse. Then,
using property_database.js, I compared these results to
inIDOMUtils.CssPropertySupportsType.

From this, with a lot of reading of nsCSSParser as well, I wrote the
special cases in inDOMUtils.cpp.

I chose this somewhat odd approach because I think it gives us a good
shot at being future-proof: if a new CSS property is added, or if one
is changed; and if the change introduces new code into
property_database.js (which of course it should), then any failures
will be caught by this new test case with no further work.

A bit of hackiness was required in the parser, due to the somewhat
strange contract of CssPropertySupportsType.  Namely, if a property
supports some "higher-level" type like TYPE_IMAGE_RECT, then it isn't
considered to support the constituent variants, like VARIANT_NUMBER.
So, at various spots in the parser we save the variant mask.

This patch is not quite ready.  It fails for the "border" property.
nsCSSProps.cpp:gBorderSubpropTable claims that "border" includes
border-image-outset (and others), but there is no parser support for
these.  I am not sure what is going on here; either a bug in the
parser or in that table, or this is a shorthand that doesn't include
the full syntax of the longhand properties it expands to.

Advice here would be great.  One option of course is to simply add a
special case for eCSSProperty_border.

Comment 10

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #9)

> Advice here would be great.  One option of course is to simply add a
> special case for eCSSProperty_border.

I got some help on #layout and in email.

I think the subprop tables are there to explain the possible effects
of the shorthand -- but not necessarily to declare what possible
syntaxes may be accepted by it.

Comment 11

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType

This patch addresses the problem with the "background" property by
adding a special case to the test.  I chose to put the special case
here because I think it provides better future-proofing, one of the
main issues addressed by the patch.

Comment 12

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType

While testing a fix for bug 1154356, I triggered a crash in the code
touched by this patch.  The problem was that PropertySupportsVariant
called nsCSSProps::IsShorthand with eCSSPropertyExtra_variable,
triggering an assertion.

This version of the patch includes a fix for this issue and a
regression test.

Comment 13

[Cameron McCormack (:heycam)]

Instead of storing the variants that we end up parsing under CSSParserImpl::ParseVariant so that cssPropertyParseCheckType can return it, what about having a known list of values to try parsing?  That way we know up front what type they would be, e.g.:

  * try parsing "rgb(0, 0, 0)", if success then we know VARIANT_COLOR is supported
  * try parsing "100%", if success then we know VARIANT_PERCENT is supported
  * etc.

This would need us to assume that each of these values is a valid value for all properties that accept that type (e.g. some properties don't accept a zero length value) but I think we can find a value that works for all properties.

Comment 14

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType

Here's a version of the patch that tests the CssPropertySupportsType
by passing in various strings.  It has some special cases for various
properties that can't accept the canonical string for a type, and
which instead need a more specialized string.

This found some bugs in the previous patch; some perhaps due to
missing sub-cases in property_database.js, but others due to not
preserving the parser state in the correct spots.

I had to remove the DEBUG conditionals around mSheetPrincipalRequired,
because one caller of SetValueToURL checks its return value.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8598860 [details] [diff] [review]
avoid false negatives in CssPropertySupportsType

Review of attachment 8598860 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: layout/inspector/inDOMUtils.cpp
@@ +680,5 @@
> +      nsCSSProps::PropertyParseType(aPropertyID) == CSS_PROPERTY_PARSE_FUNCTION) {
> +    // These must all be special-cased.
> +    uint32_t supported;
> +    switch (aPropertyID) {
> +    case eCSSProperty_border_image_slice:

Please indent cases by one additional level.

::: layout/style/test/test_bug1112014.html
@@ +17,5 @@
> +  let testValues = {
> +    TYPE_LENGTH: "10px",
> +    TYPE_PERCENTAGE: "50%",
> +    TYPE_COLOR: "rgb(3,3,3)",
> +    TYPE_URL: "url(mozilla.org)",

Although it is fine, note that this isn't the same as http://mozilla.org. :)

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2882dc585cbc

Comment 17

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType; r=heycam

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8599371 [details] [diff] [review]
avoid false negatives in CssPropertySupportsType; r=heycam

This has the indentation fixed.
Carrying over the r+

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b242594427fd

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/19ae19cbe5be because the test it added is failing on b2g:
https://treeherder.mozilla.org/logviewer.html#?job_id=9495931&repo=mozilla-inbound

Comment 21

[Tom Tromey :tromey]

(In reply to Wes Kocher (:KWierso) from comment #20)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/19ae19cbe5be because
> the test it added is failing on b2g:
> https://treeherder.mozilla.org/logviewer.html#?job_id=9495931&repo=mozilla-
> inbound

This seems odd, because the test in fact completed:

10:41:41 INFO - 109 INFO TEST-PASS | layout/style/test/test_bug1112014.html | cssPropertySupportsType returns false for variable 

... that's the last test run.

Maybe the test is taking too long and is coincidentally timing out just
as it completes?  I don't know if that can happen.

The log also says:

10:41:48 INFO - 111 ERROR [SimpleTest.finish()] this test already called finish!
10:41:48 INFO - 112 INFO TEST-UNEXPECTED-ERROR | layout/style/test/test_bug1112014.html | called finish() multiple times 

I don't understand what that's about, as the new test doesn't call finish()
at all.

I'm not totally sure what to do here, but I'll see if requesting a longer timeout helps.

Comment 22

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=569126670f08

Comment 23

[Tom Tromey :tromey]

Requesting a longer timeout seems to have worked ok.
I re-ran M(20) a few times and still saw some reds, but
examining the logs, those seem to be bug 906716.

Comment 24

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType

Request longer timeout.

Comment 25

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac0e991207b1

Comment 26

[Tom Tromey :tromey]

Attachment: avoid false negatives in CssPropertySupportsType; r=heycam IGNORE IDL

Update commit message for "IGNORE IDL".

Comment 27

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb08bbe456fb

Comment 28

[Tom Tromey :tromey]

This try run seems to have suffered from some infrastructural issues (like 
some others I have done recently).

Note that here the M(20) timeout is not related to this patch.
It is some other intermittent timeout bug.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f1fbdb4a49

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7f1fbdb4a49