Bug 37176

Summary: Canvas: radialGradient with negative radius should throw exception
Product: WebKit Reporter: George Staikos <staikos>
Component: Layout and RenderingAssignee: George Staikos <staikos>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, dbates, eric, jamesr, kling, krit, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to fix the problem, with tests.
none
Self Contained Test
none
Patch
none
Patch darin: review+

Description George Staikos 2010-04-06 16:08:01 PDT
Created attachment 52677 [details]
Patch to fix the problem, with tests.

Canvas specification says that negative radius for radial gradients should trigger INDEX_SIZE_ERR DOM exception.  Credit to Dan Bates for the test.
Comment 1 Eric Seidel (no email) 2010-04-06 20:59:49 PDT
Comment on attachment 52677 [details]
Patch to fix the problem, with tests.

OK.  Sounds sane.  Lets hope this doesn't break the web.
Comment 2 Oliver Hunt 2010-04-06 22:30:08 PDT
Test firefox -- i'm fairly sure someone tried this before and it caused site breakage (although one would expect a test to have been added if we changed that behaviour, right?)
Comment 3 Eric Seidel (no email) 2010-04-06 23:46:05 PDT
Attachment 52677 [details] was posted by a committer and has review+, assigning to George Staikos for commit.
Comment 4 Daniel Bates 2010-04-06 23:54:48 PDT
Created attachment 52707 [details]
Self Contained Test

For convenience, a self-contained version of the layout test included in the patch.
Comment 5 George Staikos 2010-04-07 17:45:32 PDT
(In reply to comment #2)
> Test firefox -- i'm fairly sure someone tried this before and it caused site
> breakage (although one would expect a test to have been added if we changed
> that behaviour, right?)

Yes, and also I kind of wonder how this could break a site, nevermind an important one.  Negative radius on a gradient??
Comment 6 Eric Seidel (no email) 2010-05-17 00:40:53 PDT
Unsure of the status of this patch.  It's been in pending-commit for over a month.  Updates?
Comment 7 Dirk Schulze 2010-05-24 11:38:38 PDT
I agree to olliver. I think it's not good to apply this patch. Some sites use negative radius, and this would break this sites. I remember a site which draws meolcules that were dragable.
I also opened a bug report on mozilla https://bugzilla.mozilla.org/show_bug.cgi?id=491150 . Someone added a patch to this br and it is still not accepted.
I would wait for mozilla before we take the patch.
Comment 8 Dirk Schulze 2010-05-24 12:06:47 PDT
*** Bug 24668 has been marked as a duplicate of this bug. ***
Comment 9 Andreas Kling 2010-06-10 01:27:34 PDT
*** Bug 40392 has been marked as a duplicate of this bug. ***
Comment 10 Eric Seidel (no email) 2010-09-02 13:51:09 PDT
Comment on attachment 52677 [details]
Patch to fix the problem, with tests.

r- until firefox agreement.  What does IE9 do?
Comment 11 James Robinson 2010-09-22 18:14:29 PDT
*** Bug 46323 has been marked as a duplicate of this bug. ***
Comment 12 James Robinson 2010-09-23 17:49:35 PDT
Created attachment 68633 [details]
Patch
Comment 13 James Robinson 2010-09-23 17:50:40 PDT
The spec, Opera, and IE9 beta all agree that this should throw.  It's also consistent with our behavior for arc() and arcTo() with a negative radius.  The canvas molecule demo cited in the mozilla bug isn't available at that URL any more, so I doubt there is any valid concern about legacy content.
Comment 14 Andreas Kling 2010-09-28 13:07:44 PDT
(In reply to comment #13)
> The spec, Opera, and IE9 beta all agree that this should throw.  It's also consistent with our behavior for arc() and arcTo() with a negative radius.  The canvas molecule demo cited in the mozilla bug isn't available at that URL any more, so I doubt there is any valid concern about legacy content.

LGTM, but I'd like someone who's more familiar with this kind of change to give the kiss of death.
Comment 15 Dirk Schulze 2010-10-13 02:00:38 PDT
Comment on attachment 68633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review

r=me

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1458
> +    if (r0 < 0 || r1 < 0) {
> +        ec = INDEX_SIZE_ERR;
> +        return 0;
> +    }

Add a newline before and after the condition.
Comment 16 Darin Adler 2010-10-13 09:40:07 PDT
Comment on attachment 68633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461
>      PassRefPtr<CanvasGradient> gradient =  CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1);
>      prepareGradientForDashboard(gradient.get());
>      return gradient;

This is a misuse of PassRefPtr.
Comment 17 Dirk Schulze 2010-10-13 09:45:15 PDT
(In reply to comment #16)
> (From update of attachment 68633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review
> 
> > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461
> >      PassRefPtr<CanvasGradient> gradient =  CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1);
> >      prepareGradientForDashboard(gradient.get());
> >      return gradient;
> 
> This is a misuse of PassRefPtr.

Oh, thats right! James can you fix this as well?
Comment 18 Dirk Schulze 2010-10-13 09:48:26 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 68633 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review
> > 
> > > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461
> > >      PassRefPtr<CanvasGradient> gradient =  CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1);
> > >      prepareGradientForDashboard(gradient.get());
> > >      return gradient;
> > 
> > This is a misuse of PassRefPtr.
> 
> Oh, thats right! James can you fix this as well?

The same for linerGradient, should be RefPtr<CanvasGradient>
Comment 19 James Robinson 2010-10-13 12:30:44 PDT
Sure thing.  I'll add the extra newlines, fix the PassRefPtr<> violations in this file and post a new patch.  Thanks for looking.
Comment 20 James Robinson 2010-10-13 18:11:58 PDT
Created attachment 70697 [details]
Patch
Comment 21 James Robinson 2010-10-13 18:12:29 PDT
Patch adds some whitespace and fixes all the misuses of PassRefPtr / RefPtr I could find in this .cpp.  Mind taking another look?
Comment 22 James Robinson 2010-10-13 19:13:23 PDT
Committed r69727: <http://trac.webkit.org/changeset/69727>
Comment 23 WebKit Review Bot 2010-10-13 21:32:48 PDT
http://trac.webkit.org/changeset/69727 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
canvas/philip/tests/2d.gradient.radial.negative.html
editing/selection/context-menu-on-text.html