585791 - String.localeCompare with a null String object returns 0

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Watson #2636183

Method:
1. var other:String = null;
2. var str:String = "test";
3. var res:int = str.localCompare(other); 

Result:
the int result is 0

Expected:
Throw exception or return a negative value.

Comment 1

[Steven Johnson]

Definitely a "needsversioning" fix. Both Firefox and Chrome return negative results, so that's my vote.

Comment 2

[Steven Johnson]

Attachment: Patch

Fix. The fix itself is straightforward, but brings up a potential deficiency in the BugCompatibility setup: if we are in a context where we don't have easy access to a Toplevel (eg, a native String method), we don't have access to the proper default BugCompatibility if we are called directly from native code (and thus the current CodeContext is null). In this particular case, it's fairly moot, as String.localeCompare isn't ever called in such a fashion, but potential future fixes might be.

Comment 3

[Lars T Hansen]

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

ECMAScript is explicit here:

The 'this' object is converted to string, call it a.
The argument is converted to string, call it b.
Those strings are compared and a string comparison value is returned.

The value null is converted to the string "null".

Thus if you change "test" to "foo" above you'll see a different result.

Comment 4

[Lars T Hansen]

Attachment: HTML test case

Comment 5

[Lars T Hansen]

Attachment: HTML test case, v2

Also shows toString conversion of the lhs when the lhs is not already a string.

Comment 6

[Steven Johnson]

(In reply to comment #3)
> Thus if you change "test" to "foo" above you'll see a different result.

Ah. And indeed, the same is true for "undefined".

Either way, though, the existing implementation is incorrect. New patch coming soon.

Comment 7

[Steven Johnson]

Attachment: Patch, v2

Revised per Lars comments, including greatly expanded test cases.

Comment 8

[Lars T Hansen]

Comment on attachment 464495 [details] [diff] [review]
Patch, v2

R+ provisional on changing AS3_localeCompare to use core()->currentBugCompatibility, now that that's been approved.

Comment 9

[Steven Johnson]

pushed to TR as 5015:2c2bee327e4f