The default bug view has changed. See this FAQ.

CanvasRenderingContext2D.fillStyle and strokeStyle accept invalid colours

VERIFIED FIXED in mozilla6

Status

()

Core
Canvas: 2D
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Oliver Hunt, Assigned: yury)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_3; en-us) AppleWebKit/527.3+ (KHTML, like Gecko) Version/4.0 Safari/527.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/4.0.1

When assigning to the fillStyle and strokeStyle properties of a CanvasRenderingContext2D instance colours with garbage data following the colour string are incorrectly accepted.

Reproducible: Always

Steps to Reproduce:
1. Open the soon to be attached test case

Actual Results:  
You will see a big red square.

Expected Results:  
You should see a big red square.
(Reporter)

Comment 1

9 years ago
Created attachment 334435 [details]
Test that checks different invalid colour strings being assigned to fill and storke style
(Reporter)

Comment 2

9 years ago
Whoops, obviously you should see a big green square in the test case when everything is correct.
(Reporter)

Comment 3

9 years ago
I would guess (without looking at the code) that gecko already does what i was dconsidering doing in webkit, and special casing numeric colour assignment rather than instantiating an instance of the css colour parser.  Based on that possibly wildly incorrect assumption I imagine all that is necessary is to verify that you are at the end of the string when you think you've parsed a colour.
This problem also happens on Windows and Linux (and also in both Firefox 2.X and 3.X).

You get a green square in Safari on OS X.  You get an error in IE7 (on Windows XP) and Konqueror (on Linux).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: Macintosh → All
Oliver, you are wildly incorrect :)

The main problem here is that CSSParserImpl::ParseColor doesn't seem to check that there's no tokens left.

http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.fillStyle.parse.invalid.name-3.htm

RRGGBB style colors also work in quirks mode.
Blocks: 622842
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Assignee: nobody → async.processingjs
(Assignee)

Comment 6

6 years ago
Created attachment 529408 [details] [diff] [review]
Fixes ParseColorString to check if while buffer was parsed plus w3c tests

The patch touches the reftest layout/reftests/bugs/579349-1.html (see bug 579349 attachment 461138 [details] by :roc). The reftest's code has |ctx.fillStyle = "blue;";|. The whatwg allows that the fillStyle and strokeStyle string must be parsed as CSS <color> value; and it looks like "blue;" is not it.

Also, the patch adds three related w3c tests to the test_canvas.html to check invalid color name assignments.
Attachment #529408 - Flags: review?(dbaron)
Attachment #529408 - Flags: feedback?(roc)
Attachment #529408 - Flags: feedback?(roc) → feedback+
Comment on attachment 529408 [details] [diff] [review]
Fixes ParseColorString to check if while buffer was parsed plus w3c tests

>@@ -1236,19 +1236,20 @@ CSSParserImpl::ParseColorString(const ns
> {
>   AssertInitialState();
>   InitScanner(aBuffer, aURI, aLineNumber, aURI, nsnull);
> 
>   nsCSSValue value;
>   PRBool colorParsed = ParseColor(value);
>   nsresult rv = mScanner.GetLowLevelError();
>   OUTPUT_ERROR();
>+  PRBool allBufferTokensWereParsed = !GetToken(PR_TRUE);

Rather than adding a second boolean, just change the earlier line to:

// parse a color, and check that there's nothing else after it
PRBool colorParsed = ParseColor(value) && !GetToken(PR_TRUE);


> Also, the patch adds three related w3c tests to the test_canvas.html to check
> invalid color name assignments.

Where did these tests come from, and how are they licensed?


r=dbaron with the change above, and assuming the tests are licensed appropriately and any licensing metadata that needs to be copied to our copy is.
Attachment #529408 - Flags: review?(dbaron) → review+
(Assignee)

Comment 8

6 years ago
(In reply to comment #7)
> Comment on attachment 529408 [details] [diff] [review] [review]

> > Also, the patch adds three related w3c tests to the test_canvas.html to check
> > invalid color name assignments.
> 
> Where did these tests come from, and how are they licensed?
> 
> 
> r=dbaron with the change above, and assuming the tests are licensed
> appropriately and any licensing metadata that needs to be copied to our copy
> is.

The added tests are Philip Taylor's as the rest of the tests in the test_canvas.html (from http://philip.html5.org/tests/canvas/suite/tests/; also see bug 407049). The tests related to this bug were missing. 

David, shall I do something special about the tests that were added?
I don't see a license on those tests, but Philip was ok with us adding them the first time around (Bug 407049).  So I think it's fine, though it would be nice to get explicit confirmation.
Keywords: dev-doc-needed
(Assignee)

Comment 10

6 years ago
Created attachment 529915 [details] [diff] [review]
Fixes ParseColorString to check if whole buffer was parsed (plus w3c tests) (v2)

(In reply to comment #7)
> Comment on attachment 529408 [details] [diff] [review] [review]

> >   nsCSSValue value;
> >   PRBool colorParsed = ParseColor(value);
> >   nsresult rv = mScanner.GetLowLevelError();
> >   OUTPUT_ERROR();
> >+  PRBool allBufferTokensWereParsed = !GetToken(PR_TRUE);
> 
> Rather than adding a second boolean, just change the earlier line to:
> 
> // parse a color, and check that there's nothing else after it
> PRBool colorParsed = ParseColor(value) && !GetToken(PR_TRUE);

Done. Thanks.
Attachment #529408 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 11

6 years ago
In case it's relevant, http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/ is a slightly more up-to-date copy with some more colour parsing tests, though it's in a sometimes less convenient format so I haven't replaced the copy on philip.html5.org yet.

The HTML files are derived from source files which are marked as MIT-licensed (http://philip.html5.org/tests/canvas/suite/tests2d.yaml) or BSD / W3C Test licensed (http://w3c-test.org/html/tests/submission/PhilipTaylor/tools/canvas/LICENSE.txt to match the conventions there) which I assume should be fine, and in any case I'm happy for anyone to use any of my test stuff for anything.
http://hg.mozilla.org/projects/cedar/rev/17754546028b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/17754546028b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
The old behavior was not documented (since it was an error that it worked that way), so the only documentation here is a new note added to "Firefox 6 for developers".
Keywords: dev-doc-needed → dev-doc-complete

Comment 15

6 years ago
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

I tried to verify if the issue is fixed on FX6 but it's unclear for me which is the expected result. 
When loading the test webpage, I don't see a red square as described (in Description), but instead I see 4 green squares. Is this expected? Thanks
Expected result for the attached test case is four green squares on a black background.

Comment 17

6 years ago
Verified fixed on:

-> Linux: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
-> Mac: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0
-> Windows: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

If you open the test webpage four green squares on a black background are visible, as expected (comment 16)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.