1184452 - @font-face rules allow certain trailing garbage in descriptors, e.g. font-family: foo !cake; src: url("a.ttf") );

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Timothy Loh]

User Agent: Mozilla/5.0 (X11; CrOS x86_64 6946.63.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

@font-face rules allow certain trailing garbage in descriptors. For example, the following rule is happily accepted:

@font-face {
 font-family: foo !cake;
 src: url("a.ttf") ) bla;
}

JSFiddle repro: http://jsfiddle.net/8ncb4w8s/4/

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, instead of using ExpectEndProperty (which should go away), ParseFontDescriptor should pass the descID and value back to its caller, which should call aRule->SetDesc() once a correct end of the descriptor has been found.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's testcase

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, no need to move across functions, just get rid of ExpectEndProperty.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I have a patch with tests, but can't test it easily until I'm back home.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Correctly reject @font-face descriptors that have garbage after them

I confirmed that the patch fixes the original testcase (attachment
8634600).  I also confirmed that with the whole patch,
layout/style/test/test_descriptor_syntax_errors.html passes, but with
the new tests but not the code change, it reports 12 failures.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8636246 [details] [diff] [review]
Correctly reject @font-face descriptors that have garbage after them

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

Not sure if the text about confirming the patch works needs to go in the commit message.

::: layout/style/nsCSSParser.cpp
@@ +3735,5 @@
>      REPORT_UNEXPECTED_P(PEValueParsingError, descName);
>      return false;
>    }
>  
> +  // Expect termination by either ;, }, or EOF.

s/either // since you don't have two items in the list.

Comment 7

[Pulsebot]

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

Comment 8

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

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