623437 - Making an argument optional changes the order in which the method is enumerated

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110105
Firefox/4.0b9pre ID:20110105030550

Drawing shape is wrong.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open ( http://js1k.com/2010-xmas/demo/865 )
3. 

Actual Results:
 Drawing shape is wrong.
 Red circle of player missing.
 Shape of obstacles is wrong.

Expected Results:
 Drawing shape should be as same as Nomoroka/GoogleChrome.

Comment 1

[Alice0775 White]

Regression window(cached hourly:
Works:
http://hg.mozilla.org/mozilla-central/rev/d3d71ad68624
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208154432
Fails:
http://hg.mozilla.org/mozilla-central/rev/4fccdd4bc023
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208180049
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d3d71ad68624&tochange=4fccdd4bc023

Comment 2

[Alice0775 White]

In local build(windows7),
build from 78ff8d5339c3 : fail
build from d3d71ad68624 : works
Candidate:
78ff8d5339c3	Benoit Jacob — Bug 617319 - HTML5 canvas 2d context arc() method requires a parameter that should be optional - r=vladimir, a=joe

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's ... really odd.  The patch there was very safe (in the sense that all it does is make calls that used to throw no longer throw), and the testcase doesn't have any arc() calls that I can see.  Can you please double-check that bisection result?

Comment 4

[Alice0775 White]

(In reply to comment #3)
> Can you please double-check that
> bisection result?
I checked , the regression changeset is sure.

Comment 5

[Alice0775 White]

I confirmed
build backed out 78ff8d5339c3 from tip 9be0e81cb86e ; works

Comment 6

[Alice0775 White]

This is the site issue.
The site's script uses method name which extracted 2 character from native method name,

The reason is because the order of the method name to appear are different.
>var c = document.getElementsByTagName('canvas')[0];
>var a = c.getContext('2d');
>for ($ in a) {
>  a[$[0] + ($[6] || $[2])] = a[$];

Prior to 9be0e81cb86e,  ac = arcTo  and over write with ac = arc
after  9be0e81cb86e, ac = arcTo and over write with ac = arcTo

Comment 7

[Alice0775 White]

Correct:
Prior to 9be0e81cb86e,  ac = arcTo  and over write with ac = arc
after  9be0e81cb86e, ac = arc and over write with ac = arcTo

Comment 8

[Brendan Eich [:brendan]]

Evangelize?

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

Why does changing to having an optional argument change the enumeration order?  That's the underlying issue here, right?  That seems ... fishy.  We can evangelize if that's hard to fix or desired for some reason, but at first glance it seems odd.

I'll attach a testcase that shows that the enumeration order did in fact change.  But I can't figure out what the enumeration order _is_ in this case; it doesn't seem to match idl order.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And to be clear, I would somewhat expect enumeration order and idl order to match, maybe.  Again, not sure how big a deal it is that it doesn't.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 12

[Sheila Mooney]

This hasn't been commented on in a while. Sounds like it should block until we understand more what's going on.

Comment 13

[Brendan Eich [:brendan]]

Unowned bug is bad. Johnny, who can dig into this?

It smells like a softblocker at best.

/be

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> Why does changing to having an optional argument change the enumeration order? 

Isn't this because by making the argument optional we lose the traceable quickstub? I think the enumeration order for quickstubs is the order in which they are installed: properties, non-traceable functions, traceable functions.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  I didn't realize quickstubs were installed in batches like that.  That certainly explains the behavior.

At the same time it makes our enumeration order for this stuff completely unpredictable to web developers.  But I'm not sure messing with this code for 2.0 is worth it...

Comment 16

[Brendan Eich [:brendan]]

We have treated other enumeration order breaks (in JS engine) as blockers. The web does tend to depend on order, at least of direct (AKA "own") non-index names.

Peter, is there a way to restore the old order without losing the essential fix that changed order?

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

I think the amount of surgery involved in that, for code that we're sort of working on removing, and given the schedule at this point, isn't worth it.  Personally.

Comment 18

[Brendan Eich [:brendan]]

Could we reorder IDL methods (and regen IID)?

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

What would be the goal?

Comment 20

[Brendan Eich [:brendan]]

You mentioned the order is unpredictable but I'm wondering if we haven't made it fixed again, and just different. If there's no static IDL member reordering we could do that would restore the lost for-in order, then never mind.

/be

Comment 21

[Boris Zbarsky [:bzbarsky]]

Right.  The current for-in order is "all idl properties in order, all non-traceable natives in order, all traceable natives in order".  This bug is a result of us changing a method from traceable to non-traceable, so it moved from bucket 3 to bucket 2.  The only way to change the order of those two methods back is to either stop the bucketing or swap those two buckets in their entirety.

Comment 22

[Peter Van der Beken [:peterv]]

The enumeration order hasn't been following the idl order for at least since quickstubs landed (FF 3.5?). I personally don't remember seeing any bugs filed about enumeration order changing when quickstubs landed. It might even have changed after that, I think we made more quickstubs traceable and we certainly quickstubbed more. I also think that given the number of DOM properties/methods and the number of interfaces that we have and the fact that we regularly do add more it's hard to guarantee enumeration order for a DOM object 'forever'.

We probably should remove the bucketing eventually, but it seems late to do that kind of change (and it will change the order again). Making this specific quickstub traceable again without making the argument non-optional also seems non-trivial.

Comment 23

[Johnny Stenback (:jst)]

I don't think we can block on this, given the last set of comments.

Comment 24

[Boris Zbarsky [:bzbarsky]]

This seems to be a somewhat common code pattern in js1k demos.  What do people think of forcing arcTo to not generate a traceable native, so the ordering of arc and arcTo changes back to what it used to be, as a temporary hack?

Comment 25

[Andreas Gal :gal]

arcTo is just one concrete problem, there are likely many more hidden ones, we should either fix the problem, or live with the symptoms

Comment 26

[Boris Zbarsky [:bzbarsky]]

Andreas, arcTo is a specific case that breaks because of specific hacky-code that's popular in the js1k demos.

The "hidden" ones were there in 3.6, for the most part.

If we care about fixing these demos, then we should consider doing that if it's simple, but "fix the problem" is a non-starter for 2.0, imo.

Comment 27

[Johnny Stenback (:jst)]

I'd be ok with a hack here to get these demos going working again, as long as we don't try to fool ourselves into thinking this is a solved problem after that...

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 29

[Boris Zbarsky [:bzbarsky]]

I will definitely file a followup on the real issue here, I promise!

I did verify that the attached patch, if I can call it that, fixes the demos.

Comment 30

[Andreas Gal :gal]

Comment on attachment 509334 [details] [diff] [review]
Like so

We should land it while peterv looks at it. And ... ugh.

Comment 31

[Brendan Eich [:brendan]]

Glad this is getting fixed (it's a fix, even if a hack -- js1k demos are hacks, too -- some quite clever and beautiful).

/be

Comment 32

[Peter Van der Beken [:peterv]]

Comment on attachment 509334 [details] [diff] [review]
Like so

Bah.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0399ff8f614c

Filed bug 632539 on the underlying problem.

Comment 35

[Brendan Eich [:brendan]]

http://hg.mozilla.org/mozilla-central/rev/db3e06f7018e

(js/src/xpconnect/tests/mochitest/Makefile.in was missing from bz's push.)

/be