Last Comment Bug 451165 - CanvasRenderingContext2D.fillStyle and strokeStyle accept invalid colours
: CanvasRenderingContext2D.fillStyle and strokeStyle accept invalid colours
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Yury Delendik (:yury)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 622842
  Show dependency treegraph
 
Reported: 2008-08-19 01:49 PDT by Oliver Hunt
Modified: 2011-07-20 04:18 PDT (History)
11 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test that checks different invalid colour strings being assigned to fill and storke style (1004 bytes, text/html)
2008-08-19 01:50 PDT, Oliver Hunt
no flags Details
Fixes ParseColorString to check if while buffer was parsed plus w3c tests (4.92 KB, patch)
2011-05-01 19:58 PDT, Yury Delendik (:yury)
dbaron: review+
roc: feedback+
Details | Diff | Splinter Review
Fixes ParseColorString to check if whole buffer was parsed (plus w3c tests) (v2) (4.94 KB, patch)
2011-05-03 19:22 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review

Description Oliver Hunt 2008-08-19 01:49:00 PDT
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.
Comment 1 Oliver Hunt 2008-08-19 01:50:19 PDT
Created attachment 334435 [details]
Test that checks different invalid colour strings being assigned to fill and storke style
Comment 2 Oliver Hunt 2008-08-19 01:51:21 PDT
Whoops, obviously you should see a big green square in the test case when everything is correct.
Comment 3 Oliver Hunt 2008-08-19 01:53:54 PDT
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.
Comment 4 Steven Michaud [:smichaud] (Retired) 2008-09-05 12:00:04 PDT
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).
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-01-29 06:28:09 PST
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.
Comment 6 Yury Delendik (:yury) 2011-05-01 19:58:38 PDT
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.
Comment 7 David Baron :dbaron: ⌚️UTC-8 2011-05-02 14:34:51 PDT
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.
Comment 8 Yury Delendik (:yury) 2011-05-02 17:43:16 PDT
(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?
Comment 9 David Baron :dbaron: ⌚️UTC-8 2011-05-02 20:31:09 PDT
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.
Comment 10 Yury Delendik (:yury) 2011-05-03 19:22:37 PDT
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.
Comment 11 Philip Taylor 2011-05-04 08:38:12 PDT
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.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-05-05 20:00:52 PDT
http://hg.mozilla.org/projects/cedar/rev/17754546028b
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-05-06 21:20:19 PDT
http://hg.mozilla.org/mozilla-central/rev/17754546028b
Comment 14 Eric Shepherd [:sheppy] 2011-05-10 06:09:55 PDT
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".
Comment 15 AndreiD[QA] 2011-07-20 02:03:46 PDT
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
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2011-07-20 03:15:58 PDT
Expected result for the attached test case is four green squares on a black background.
Comment 17 AndreiD[QA] 2011-07-20 04:18:06 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.