1471772 - U+0000 in a custom property value is not replaced with U+FFFD

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Simon Sapin (:SimonSapin)]

Reported in https://github.com/w3c/csswg-drafts/issues/2757#issuecomment-396525340

Test case: https://codepen.io/anon/pen/WyYeLv

    <div id="a"></div>
    <div id="b"></div>

    <script type="text/javascript">
    var css = document.createElement("style");
    css.innerHTML = '* { --a: hello\0world;  }';
    document.body.appendChild(css);

    a.innerHTML = JSON.stringify(css.sheet.cssRules[0].style.getPropertyValue("--a"));
    b.innerHTML = JSON.stringify(getComputedStyle(document.body).getPropertyValue('--a'));
    </script>

The tokenizer does replace correctly, but for custom properties we store a slice of the unmodified original input as a single String, since that is more compact than Vec<Token>.

We should probably do the replacement around here: 

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/servo/components/style/custom_properties.rs#305


There is a second issue where that string is null-truncated at some point between specified value and computed value, but fixing that first issue should stop NULLs from reaching far enough to trigger this.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Yeah, that sounds like the right place to do the replacement. Also I noticed that we can actually convert the string to owned in that function directly because every callsite invokes into_owned as well, so it doesn't really make much sense to keep Cow<str> there...

Comment 2

[Simon Sapin (:SimonSapin)]

Re owned String, sounds good.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Reading the spec issue, it sounds like there is no need to fix this bug actually... We can just discard the whole thing afterward directly in this case, right?

Comment 4

[Simon Sapin (:SimonSapin)]

Well that is one of the alternatives of a proposal under discussion, but this proposal doesn’t make consensus so far.