1544223 - Bad parsing of complex custom property value, results in many bogus declarations

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[Florens Verschelde :fvsch]

Attachment: inspector-css-custom-property-value-bad-parsing.jpg

Here is an extreme example of storing code in a CSS custom property value:
https://codepen.io/yuanchuan/pen/yrobOe
(Warning: resource-intensive graphics code.)

To be honest I'm not sure how it works, it seems it's using a custom property to pass more CSS code to run (including @keyframes declarations) to a custom element.

When loading the page, I'm not getting any CSS error in the Console.
In the Inspector, I'm getting the result in the attached screenshot, where most of this code is cut in several declarations:

css-doodle {
  --rule: (
    :doodle {
      @grid: 30x1 / 18vmin;
    }
    :container {
      perspective: 30vmin;
    }

    @place-cell: center;
    @size: 100%;
  
    :after, :before {
      content: '';
      @size: 100%;
      position: absolute;
      border: 2.4vmin solid var(--color);
      border-image: radial-gradient(
        var(--color), transparent 60%
      );
      border-image-width: 4;
      transform: rotate(@pn(0, 5deg));
    }

    will-change: transform, opacity;
    animation: scale-up 15s linear infinite;
    animation-delay: calc(-15s / @size() * @i());
    box-shadow: inset 0 0 1em var(--color);
    border-radius: 50%;
    filter: var(--filter); 
  
    @keyframes scale-up {
      0%, 100% {
        transform: translateZ(0) scale(0) rotate(0);
        opacity: 0;
      }
      50% {
        opacity: 1;
      }
      99% {
        transform: 
          translateZ(30vmin) 
          scale(1) 
          rotate(-270deg);
      }
    }
  )
}

For the record, this code does return the complete value for the --rule property:

getComputedStyle(document.querySelector('css-doodle'))
  .getPropertyValue('--rule')

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: CSS declaration with complex value in Chrome Canary

This is spectacular! :) Most likely our CSS parsing never expected such complexity (an entire stylesheet) to be the value of a CSS declaration.

Chrome Canary handles it better (see attached screenshot)

Comment 2

[Patrick Brosset <:pbro>]

There are 2 problems here:

• When attempting to display existing styles in the rule-view with a property value containing a semicolon ;, the rule-view stops at that semicolon and assumes what comes after is the start of a new declaration.
• When attempting to write a new css value in the rule-view, pressing ; automatically ends the valuer and inserts a new property below, no matter where the character is typed.

The first problem comes from parseDeclarationsInternal in /devtools/shared/css/parsing-utils.js I assume. The 2nd might also come from here actually.

In the CSS Syntax Module Level 3, the following is mentioned:

[] blocks, () blocks and functions can now contain {} blocks, <at-keyword-token>s or <semicolon-token>s

So, it seems to me that our parser should consider (, { and [ as the start of blocks, and wait until those are closed to consider ; as the end of a declaration.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug 1544223 - Don't advance to the next property or value when parsing ; or : inside a CSS block; r?gl!

Comment 4

[Patrick Brosset <:pbro>]

Hi Tom. If by any chance you had some time, I'd really appreciate your feedback on this approach. It seems straight-forward enough to me, but you're, by far, more experienced than I am with those CSS parser helpers.
No sweat if you don't have time though.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug 1544223 - Disable a few high complexity functions and lower the limit to prevent more complexity; r?jdescottes!

Depends on D31973

Comment 6

[Patrick Brosset <:pbro>]

ESLint complains that the function I changed in this patch is now too complex.
DevTools' limit for cyclomatic complexity is 53 (see .eslintrc.js). That's really high already. I believe this was set to be that high to cover all devtools without errors.

The default eslint value is 20, so I looked at all of the functions that are higher than that and pasted the result here:
https://docs.google.com/spreadsheets/d/1w3jl9lbDsG7EHazAYkZ0s7id5uo20sGKjZ5MeiK0hx0/edit#gid=1748770914

From what I can see, the higher you go, the less functions there are, obviously. So it might be good to make a function exceptions to the few highly complexity ones, and lower our threshold in .eslintrc.
I'm tempted to go for 40 and disable this rule for the 8 functions that are higher than that.

It would be great to re-work these 8 functions and make them less complex, but unfortunately some are really hard to re-work.
The one I changed here is a state machine that does parsing, and moving bits of its logic out of the function and into smaller functions isn't easily feasible without redoing the entire thing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/011f651a8518
Don't advance to the next property or value when parsing ; or : inside a CSS block; r=gl
https://hg.mozilla.org/integration/autoland/rev/bcabad82b6c4
Disable a few high complexity functions and lower the limit to prevent more complexity; r=jdescottes

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/011f651a8518
https://hg.mozilla.org/mozilla-central/rev/bcabad82b6c4

Comment 9

[Razvan Caliman [:rcaliman]]

(In reply to Patrick Brosset <:pbro> from comment #6)

I'm tempted to go for 40 and disable this rule for the 8 functions that are higher than that.

It would be great to re-work these 8 functions and make them less complex, but unfortunately some are really hard to re-work.
The one I changed here is a state machine that does parsing, and moving bits of its logic out of the function and into smaller functions isn't easily feasible without redoing the entire thing.

Agreed. One of those functions is my doing, convertUnits() in fonts.js. It's been bugging me for a while and I have a few ideas to write it better. Regardless, when converting units, the count for the cyclomatic complexity will increase at least linearly with each unit type (and we already support 12 for fonts).

This message is a good reminder for me to reconsider a generalized CSS unit conversion utility that we can use across DevTools Inspector.

Comment 10

[Tom Tromey :tromey]

Hi Patrick. Sorry about the delay on this; also I can't seem to comment on phabricator any more.

The patch looks reasonable to me. One bit seemed maybe a little weird:

    if (token.tokenType === "symbol" &&
        currentBlocks[currentBlocks.length - 1] === token.text) {

Maybe it's safe enough but this seems odd when currentBlocks is empty.

Also I guess here:

    } else if (token.tokenType === "symbol" && token.text === ":") {

should this be checking !currentBlocks.length? Or else won't a ":" terminate mid-block? (It seems like the patch must be correct, since your test case checks this, but I don't understand why it works.)

I didn't check the context of the patch to see if there are other branches of the if that might need to be updated.

Comment 11

[Patrick Brosset <:pbro>]

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

Hi Patrick. Sorry about the delay on this; also I can't seem to comment on phabricator any more.

No need to be sorry, you were actually really quick at replying! I landed it because the tests gave me confidence + we are at the start of a nightly cycle, so there's time to uncover problems and fix them.

    if (token.tokenType === "symbol" &&
        currentBlocks[currentBlocks.length - 1] === token.text) {

Maybe it's safe enough but this seems odd when currentBlocks is empty.

currentBlocks[currentBlocks.length - 1] will return undefined then, and that won't be equal to token.text, so we should be safe. It might be better to be explicit about this though. It seems a bit unsafe at first sight.

Also I guess here:

    } else if (token.tokenType === "symbol" && token.text === ":") {

should this be checking !currentBlocks.length? Or else won't a ":" terminate mid-block? (It seems like the patch must be correct, since your test case checks this, but I don't understand why it works.)

That's exactly what I originally did, but then realized I didn't need to, otherwise the parser wouldn't be treating property names correctly. Blocks don't exist in property names, so a : character needs to advance to the value there. And if we're inside a value then we'll deal with this by checking the content of lastProp later anyway.
So this is safe.