861938 - JS demo does not run. http://js1k.com/2010-xmas/demo/856

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130415 Firefox/23.0
Build ID: 20130415030812

Steps to reproduce:

1. Create fresh profile
2. Go to http://js1k.com/2010-xmas/demo/856



Actual results:

Nothing happened.


Expected results:

A christmas tree should have appeared, with snow and some red balls.

Works in FF19. So a regression.

Comment 1

[Mayank Bansal]

In fact, there are some other demos on this site that dont run on the Nightly, but run on older versions.

Comment 2

[mjh563]

Attachment: Reduced testcase (displays GOOD or BAD)

Comment 3

[mjh563]

Regression range is

Last good nightly: 2013-01-16
First bad nightly: 2013-01-17

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d8be4bc4fba8&tochange=712eca11a04e

Comment 4

[mjh563]

The problem is caused by the page script calling this function:

CanvasRenderingContext2D::Fill()

with |undefined| as a parameter. That now throws an exception, whereas previously it didn't.

The function is documented on MDN as having no parameters[1], but looking at the code it does take one parameter of type const CanvasWindingRule&.

[1] https://developer.mozilla.org/en-US/docs/DOM/CanvasRenderingContext2D#fill%28%29

Comment 5

[Josh Matthews [:jdm]]

Boris, this looks like a change in how undefined values are treated when passed to methods expecting enums. The only possibility I see in the regression range is bug 824864; do you have a better suspect?

Comment 6

[mjh563]

I suspect it's this one...

changeset:   119115:64450d6fee96
user:        Rik Cabanier <[e-mail]>
date:        Wed Jan 16 21:55:43 2013 -0500
summary:     Bug 827053 - Add support for winding in fill + clip + isPointInPath + tests the feature. r=bas

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yep, comment 6 is spot on.  The way the IDL (and the spec so far) is written passing undefined is NOT the same as not passing the argument: the latter uses the default value while the latter tries to stringify to "undefined" and then that's not a valid enum value so it throws an exception.

This demo does not run in a current Chrome dev build, for presumably the same reason.

I suppose we could change the IDL to [TreatUndefinedAs=Missing], since that's what the ES folks want all WebIDL to move to by default anyway...  Rik?

Comment 8

[Maria Grazia Alastra [:mga]]

Probably there was no need to confirm this one, but:
Confirmed on Windows 7, 64 bits, on Firefox 21.0, on Aurora 23.0 and on latest Nightly 24.0:

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0, Build ID: 20130511120803

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130523 Firefox/23.0, Build ID: 20130523004551

- User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:24.0) Gecko/20130523 Firefox/24.0, Build ID: 20130523030935

Reporter, you have done an excellent work with this bug, thank you for your contribution!

Comment 9

[Boris Zbarsky [:bzbarsky]]

But not a layout bug at all.  ;)

Comment 10

[Rik Cabanier (don't use this account)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> 
> I suppose we could change the IDL to [TreatUndefinedAs=Missing], since
> that's what the ES folks want all WebIDL to move to by default anyway... 
> Rik?

Yes, this change should have been backward compatible. I will try to submit a fix for this tomorrow.

Comment 11

[Rik Cabanier (don't use this account)]

(In reply to mayankleoboy1 from comment #1)
> In fact, there are some other demos on this site that dont run on the
> Nightly, but run on older versions.

Could you also file a bug against WebKit and Chrome?

Comment 12

[Rik Cabanier]

(In reply to Rik Cabanier from comment #10)
> (In reply to Boris Zbarsky (:bz) from comment #7)
> > 
> > I suppose we could change the IDL to [TreatUndefinedAs=Missing], since
> > that's what the ES folks want all WebIDL to move to by default anyway... 
> > Rik?
> 
> Yes, this change should have been backward compatible. I will try to submit
> a fix for this tomorrow.

I can't submit a fix because [TreatUndefinedAs=Missing] is not implemented yet.
(https://bugzilla.mozilla.org/show_bug.cgi?id=829248)

I could work around this by having 2 signatures for 'fill'.
Does that sound OK?

Comment 13

[Boris Zbarsky [:bzbarsky]]

How would two signatures help?

Comment 14

[Rik Cabanier]

(In reply to Boris Zbarsky (:bz) from comment #13)
> How would two signatures help?

yes. I just tried it and it makes no difference. It still takes the 'enum' route which then fails.
I'm unsure how I can cleanly fix this.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the CanvasWindingRule arguments be treated as missing if undefined is passed in.

The clean way to fix this is to fix bug 829248.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 755018 [details] [diff] [review]
Make the CanvasWindingRule arguments be treated as missing if undefined is passed in.

Don't we need to change the spec too?

Comment 17

[Rik Cabanier]

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 755018 [details] [diff] [review]
> Make the CanvasWindingRule arguments be treated as missing if undefined is
> passed in.
> 
> Don't we need to change the spec too?

It seems this would apply to every function that currently doesn't take arguments.
"I suppose we could change the IDL to [TreatUndefinedAs=Missing], since that's what the ES folks want all WebIDL to move to by default anyway"

If that's the case, why don't we change the code that generates the C++ binding so undefined parameters for optional arguments are allowed?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> If that's the case, why don't we change the code that generates the C++ binding so
> undefined parameters for optional arguments are allowed?

Because I haven't been able to get a clear commitment out of the other WebIDL implementors that they plan to do that behavior.

So for now, the spec needs [TreatUndefinedAs=Missing] to work like we want it to.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=22218 on the spec.

Comment 20

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a25c0e2489f2

Comment 21

[Rik Cabanier (don't use this account)]

(In reply to Boris Zbarsky (:bz) from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a25c0e2489f2

Does this mean that "[TreatUndefinedAs=Missing]" is implemented?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yep.  See bug 829248.

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a25c0e2489f2