Bug 37176 – Canvas: radialGradient with negative radius should throw exception

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[George Staikos]

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.

[Eric Seidel (no email)]

Comment on attachment 52677 [details]
Patch to fix the problem, with tests.

OK.  Sounds sane.  Lets hope this doesn't break the web.

[Oliver Hunt]

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?)

[Eric Seidel (no email)]

Attachment 52677 [details] was posted by a committer and has review+, assigning to George Staikos for commit.

[Daniel Bates]

Created attachment 52707 [details]
Self Contained Test

For convenience, a self-contained version of the layout test included in the patch.

[George Staikos]

(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??

[Eric Seidel (no email)]

Unsure of the status of this patch.  It's been in pending-commit for over a month.  Updates?

[Dirk Schulze]

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.

[Dirk Schulze]

*** Bug 24668 has been marked as a duplicate of this bug. ***

[Andreas Kling]

*** Bug 40392 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 52677 [details]
Patch to fix the problem, with tests.

r- until firefox agreement.  What does IE9 do?

[James Robinson]

*** Bug 46323 has been marked as a duplicate of this bug. ***

[James Robinson]

Created attachment 68633 [details]
Patch

[James Robinson]

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.

[Andreas Kling]

(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.

[Dirk Schulze]

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.

[Darin Adler]

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.

[Dirk Schulze]

(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?

[Dirk Schulze]

(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>

[James Robinson]

Sure thing.  I'll add the extra newlines, fix the PassRefPtr<> violations in this file and post a new patch.  Thanks for looking.

[James Robinson]

Created attachment 70697 [details]
Patch

[James Robinson]

Patch adds some whitespace and fixes all the misuses of PassRefPtr / RefPtr I could find in this .cpp.  Mind taking another look?

[James Robinson]

Committed r69727: <http://trac.webkit.org/changeset/69727>

[WebKit Review Bot]

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