557933 - Hashtable String-to-number conversion is different in Argo vs. Astro

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Steven Johnson]

When you use a string as an object key, the language quietly translate strings to integers where appropriate, so that o[3] == o["3"]. It appears that changes to the String code have changed the semantics when there are leading zeroes in the string: we used to disallow leading zeroes (so "03" was not considered equivalent to "3" or 3) but now we appear to allow exactly one leading zero. 

This is flamingly wrong: consider

    var o:Object = {};
    o[3] = 1;
    o["3"] = 2;
    o["03"] = 3;
    o["003"] = 4;

    for (var n:* in o) {
        print(n, typeof(n), o[n])
    }

in 10.0 we'd get 

003 string 4
03 string 3
3 number 2

but now we get 

003 string 4
3 number 3

Comment 1

[Lars T Hansen]

Flame on!

Comment 2

[Steven Johnson]

Attachment: Patch

Problem seems to be in String::getIntAtom and String::parseIndex... it attempts to do the right thing but it appears its calculation is simply misplaced.

Comment 3

[Steven Johnson]

FWIW, this was only found because of a performance regression; apparently our acceptance tests aren't finding this. I'll add one. The performance regression was from code of the form

    var loops:uint = 100000;
    var hash2:Object;

    function init():void {
        hash2 = {};
        for (var i=0; i<loops; i++) {
            var s:String = "0"+i; // also: +"000", and +"00000000"
            hash2[s] = i/3;
        }
    }

    function objHash2():void {
        for (var n:String in hash2) {
            hash2[n] += 0.2;
        }
    }

    init();
    objHash2();

Formerly, the keys were always kept as String (since they had leading zero); now, they are erroneously converted to number, then reconverted to String in the iteration loop.

Comment 4

[Steven Johnson]

Attachment: Acceptance Test

Comment 5

[Trevor Baker]

Steven, where was the perf regression observed?

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Trevor: see http://watsonexp.corp.adobe.com/#bug=2597170

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #2)
> Created an attachment (id=437693) [details]
> Patch

You might revise the comment "// bad character, or more than one leading zero?"; 
as it currently stands the comment implies that exactly one leading zero might be
okay (precisely the bug you are fixing).  E.g. "bad character, or has leading zeroes?"

Comment 8

[Trevor Baker]

Should this be targetted for 10.1?

Comment 9

[Edwin Smith]

yes!

Comment 10

[Edwin Smith]

Comment on attachment 437699 [details] [diff] [review]
Acceptance Test

as long as the test fails before and passes after the fix, its good.  no idea how well it covers the area of interest tho.  (negative numbers? decimals? hex literals?)

Comment 11

[Edwin Smith]

Comment on attachment 437693 [details] [diff] [review]
Patch

Looks okay.

One more thought on the test: does it exersize both parseIndex() and getIntAtom()?

if not, it probably should.

Comment 12

[Steven Johnson]

(In reply to comment #7)
> You might revise the comment "// bad character, or more than one leading

yep, will do

(In reply to comment #10)
> how well it covers the area of interest tho.  (negative numbers? decimals? hex
> literals?) does it exersize both parseIndex() and getIntAtom()?


Will augment the test before pushing.

Comment 13

[Steven Johnson]

Pushed code change: 
http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/508cc7b9bd0a
http://hg.mozilla.org/tamarin-redux/rev/508cc7b9bd0a

(Testcase will be augmented prior to push)

Comment 14

[Steven Johnson]

Attachment: Updated acceptance test

Test per Edwin's comments.

Comment 15

[Lars T Hansen]

Comment on attachment 438194 [details] [diff] [review]
Updated acceptance test

Obviously we are more permissive around leading zeroes than the ECMAScript spec :-)

Comment 16

[Chris Peyer]

Acceptance test (attachment 438194 [details] [diff] [review]) pushed redux changeset: 4438:b7ee29a3e532