Last Comment Bug 649618 - If the canvas has a horizontal dimension of zero, then toDataURL() must return the string "data:,".
: If the canvas has a horizontal dimension of zero, then toDataURL() must retur...
Status: VERIFIED FIXED
ietestcenter
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: :Benjamin Peterson
:
Mentors:
http://samples.msdn.microsoft.com/iet...
Depends on:
Blocks: 622842
  Show dependency treegraph
 
Reported: 2011-04-13 06:49 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2011-07-20 05:16 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to fix the issue (2.58 KB, patch)
2011-04-21 19:21 PDT, :Benjamin Peterson
joe: review-
Details | Diff | Splinter Review
fix the issue (2.45 KB, patch)
2011-05-05 16:21 PDT, :Benjamin Peterson
joe: review+
Details | Diff | Splinter Review
fix based on 421865 (1.11 KB, patch)
2011-05-10 17:25 PDT, :Benjamin Peterson
joe: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-13 06:49:31 PDT

    
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-13 07:09:43 PDT
At least one problem here: when canvas.width=0 is called we don't actually set the width to 0 ... see http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#121
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-13 23:26:05 PDT
If I change the clamp from <= to < we crash in Cairo while trying to get an inputstream from the context.
Comment 3 Masatoshi Kimura [:emk] 2011-04-13 23:37:28 PDT
The check was added by bug 291216.
Comment 4 :Benjamin Peterson 2011-04-21 19:21:58 PDT
Created attachment 527714 [details] [diff] [review]
patch to fix the issue

Here is a patch and test.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-21 19:29:55 PDT
Comment on attachment 527714 [details] [diff] [review]
patch to fix the issue

Thanks for the patch Benjamin.

Joe, can you review this?  Not sure who reviews canvas stuff these days ...
Comment 6 Joe Drew (not getting mail) 2011-05-05 12:35:19 PDT
Comment on attachment 527714 [details] [diff] [review]
patch to fix the issue

Review of attachment 527714 [details] [diff] [review]:

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +124,3 @@
     size.width = DEFAULT_CANVAS_WIDTH;
+  if (size.height < 0)
+    size.height = DEFAULT_CANVAS_HEIGHT;

I think I would rather GetWidthHeightInternal didn't do any frobbing of the returned data; if users want to get the raw data, they should be ready to handle silly results.

(This would also imply changing GetWidthHeight to contain <= rather than ==.)

@@ +360,5 @@
+    nsIntSize size = GetWidthHeightInternal();
+    if (size.height == 0 || size.width == 0) {
+      aDataURL = NS_LITERAL_STRING("data:,");
+      return NS_OK;
+    }

Similarly, here we'd change this to <= 0.
Comment 7 :Benjamin Peterson 2011-05-05 16:21:50 PDT
Created attachment 530456 [details] [diff] [review]
fix the issue

This addresses review comments. However, it does not change "width == 0" to "width <= 0" because I don't think a negative value can get past ParseNonNegativeIntValue in ParseAttribute(). Correct me if I'm wrong.
Comment 8 Joe Drew (not getting mail) 2011-05-10 13:08:52 PDT
Comment on attachment 530456 [details] [diff] [review]
fix the issue

's fine with me! :)
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-05-10 13:12:33 PDT
I don't think we should do this until we fix bug 421865.
Comment 11 :Benjamin Peterson 2011-05-10 17:25:20 PDT
Created attachment 531507 [details] [diff] [review]
fix based on 421865

This simple patch (requiring the the patch to 421865) fixes the issue.
Comment 12 Joe Drew (not getting mail) 2011-05-18 10:16:21 PDT
Comment on attachment 531507 [details] [diff] [review]
fix based on 421865

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

Love it.
Comment 13 Mounir Lamouri (:mounir) 2011-05-20 07:01:20 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/951de6cc1fdb
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-21 13:05:12 PDT
Thanks for fixing this Benjamin.
Comment 15 AndreiD[QA] 2011-07-20 05:06:50 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

The test toDataURL on w3c-test.org (Comment 10) is PASS, as well as the test at the link:
http://samples.msdn.microsoft.com/ietestcenter/html5/canvas_harness.htm?url=canvas-canvasElement-001
Also the depending bug 421865 has been fixed
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2011-07-20 05:16:10 PDT
Thanks for the verification, Andrei.

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