If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla2.0b12

Status

()

Core
XPConnect
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Alice0775 White, Assigned: bz)

Tracking

Trunk
mozilla2.0b12
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Created attachment 501558 [details]
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.
(Reporter)

Comment 1

7 years ago
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
blocking2.0: --- → ?
Keywords: regression
(Reporter)

Comment 2

7 years ago
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
Blocks: 617319
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?
(Reporter)

Comment 4

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

Comment 5

7 years ago
I confirmed
build backed out 78ff8d5339c3 from tip 9be0e81cb86e ; works
(Reporter)

Comment 6

7 years ago
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
(Reporter)

Updated

7 years ago
No longer blocks: 617319
blocking2.0: ? → ---
Keywords: regression
(Reporter)

Comment 7

7 years ago
Correct:
Prior to 9be0e81cb86e,  ac = arcTo  and over write with ac = arc
after  9be0e81cb86e, ac = arc and over write with ac = arcTo
Evangelize?

/be
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.
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Component: Canvas: 2D → XPConnect
QA Contact: canvas.2d → xpconnect
Resolution: INVALID → ---
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.
Summary: Drawing shape is wrong → Making an argument optional changes the order in which the method is enumerated
Created attachment 501926 [details]
Testcase

Comment 12

7 years ago
This hasn't been commented on in a while. Sounds like it should block until we understand more what's going on.
Unowned bug is bad. Johnny, who can dig into this?

It smells like a softblocker at best.

/be
(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.
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...
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
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.
Could we reorder IDL methods (and regen IID)?

/be
What would be the goal?
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
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.
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.
I don't think we can block on this, given the last set of comments.
blocking2.0: ? → -
Blocks: 617319
Blocks: 631117
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

7 years ago
arcTo is just one concrete problem, there are likely many more hidden ones, we should either fix the problem, or live with the symptoms
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.
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...
Created attachment 509334 [details] [diff] [review]
Like so
Attachment #509334 - Flags: review?(peterv)
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.
Assignee: nobody → bzbarsky
Whiteboard: [need review]

Comment 30

7 years ago
Comment on attachment 509334 [details] [diff] [review]
Like so

We should land it while peterv looks at it. And ... ugh.
Attachment #509334 - Flags: review+
Whiteboard: [need review] → [need approval]
Attachment #509334 - Flags: approval2.0?
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 on attachment 509334 [details] [diff] [review]
Like so

Bah.
Attachment #509334 - Flags: review?(peterv) → review+
Duplicate of this bug: 631117

Updated

7 years ago
Attachment #509334 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/0399ff8f614c

Filed bug 632539 on the underlying problem.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Flags: in-testsuite+
Priority: -- → P1
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b12

Updated

7 years ago
blocking2.0: - → betaN+
http://hg.mozilla.org/mozilla-central/rev/db3e06f7018e

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

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