String.localeCompare with a null String object returns 0

RESOLVED FIXED

Status

Tamarin
Virtual Machine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

unspecified
Bug Flags:
flashplayer-needsversioning +

Details

Attachments

(2 attachments, 2 obsolete attachments)

382 bytes, text/plain
Details
24.63 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Flags: flashplayer-needsversioning+
(Assignee)

Comment 1

8 years ago
Definitely a "needsversioning" fix. Both Firefox and Chrome return negative results, so that's my vote.
(Assignee)

Updated

8 years ago
Depends on: 535770
(Assignee)

Comment 2

8 years ago
Created attachment 464216 [details] [diff] [review]
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.
Assignee: nobody → stejohns
Attachment #464216 - Flags: review?(lhansen)

Comment 3

8 years ago
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.
Attachment #464216 - Flags: review?(lhansen) → review-

Comment 4

8 years ago
Created attachment 464333 [details]
HTML test case

Comment 5

8 years ago
Created attachment 464339 [details]
HTML test case, v2

Also shows toString conversion of the lhs when the lhs is not already a string.
Attachment #464333 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
Created attachment 464495 [details] [diff] [review]
Patch, v2

Revised per Lars comments, including greatly expanded test cases.
Attachment #464216 - Attachment is obsolete: true
Attachment #464495 - Flags: review?(lhansen)

Comment 8

8 years ago
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.
Attachment #464495 - Flags: review?(lhansen) → review+
(Assignee)

Comment 9

8 years ago
pushed to TR as 5015:2c2bee327e4f
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.