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

RESOLVED FIXED in Firefox 42

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Timothy Loh, Assigned: dbaron)

Tracking

41 Branch
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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/
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.
Created attachment 8634600 [details]
reporter's testcase
Assignee: nobody → dbaron
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Actually, no need to move across functions, just get rid of ExpectEndProperty.)
(I have a patch with tests, but can't test it easily until I'm back home.)
Created attachment 8636246 [details] [diff] [review]
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.
Attachment #8636246 - Flags: review?(cam)
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.
Attachment #8636246 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/dfa5748ff086
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.