623746 - Incorrect treatment of some null-valued arguments by addition/concatenation

Status: RESOLVED FIXED

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

Description

[Christopher Thilgen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: AVMPlus Internal Build On Nov-2-2010

Behavior is different than previous build which would not prepend null.

NOTE: appending a string to an unitialized String (+=) would prepend a null (and continues today)

Reproducible: Always

Steps to Reproduce:
------------------------

trace("BEGIN-TEST")

var testString1:String;
testString1 += "5";
trace("testString1:" + testString1);

var testString2:String;
testString2 += 5;
trace("testString2:" + testString2);

trace("END-TEST")

------------------------


RESULTS: oct-31-2010 11:00 pm

BEGIN-TEST
  testString1:null5
  testString2:5
END-TEST

RESULTS: nov-2-2010 11:00 pm

BEGIN-TEST
  testString1:null5
  testString2:null5
END-TEST

------------------------




Please refer to Watson Bug #2783583 for additional information and injection details.

Comment 1

[Christopher Thilgen]

NOTE: We should add additional unit tests to cover this operation since we obviously don't have them today.

Comment 2

[Lars T Hansen]

This could be a result of jit policy changes in Spicy?

If I run your test case with the interpreter (-Dinterp) I get your "old" result, and if I run it with the JIT (-Ojit) I get the "new" result (tamarin-redux 5733).

Chris, when you're reporting results like these that are obtained with a plugin we need to know from which branch and changeset the plugin was built (and also whether it's a Release or Release-Debugger or other configuration).

Comment 3

[Lars T Hansen]

BTW my understanding is that your "old" result is correct according to specs - null added to number 5 yields number 5 which is then converted back to string - so we're possibly looking at a new JIT bug here where it prematurely converts number 5 to string because it "knows" the result is going to be a string.

For grins I tried with TR 4058, and here the JIT and interpreter agree and produce your "old" result.

Looks like a JIT injection.

Comment 4

[Werner Sharp]

Bill should investigate this ASAP for the Spicy release.  The problem is the VMCFG_FASTPATH_ADD code and all its helpers not correctly handling a null String ptr.  In the above test case, we get into op_add_a_ai and hit the concat_strings code at the bottom.  A fix would be to have a null check added: 

	if (ISNULL(lhs))
		return s->atom();
	else
		return core->concatStrings(core->string(lhs), s)->atom();

All the helper routines that perform concatStrings need to be investigated.

int + atom
atom + int
double + atom
atom + double
etc.

Comment 5

[Steven Johnson]

Taking the liberty of assigning to [e-mail] -- if you don't have the bandwidth to examine soon, let us know and we'll find someone to do so

Comment 6

[Steven Johnson]

(BTW, this bug is currently assigned to me in Watson, but Bill is far more familiar with this code than I -- for a late-breaking Spicy change I'd definitely be more comfortable with him investigating, if possible)

Comment 7

[William Maddox]

Attachment: Remove atomIsString() and use AvmCore::isString() in specialized add/concatenate helper functions

The helper functions are all specializations of op_add_a_aa(), which implements the general case of atom + atom addition/concatenation when the fastpath optimization is enabled.  It was intended to be a slightly optimized version of the existing op_add() used by the interpreter, with more streamlined handling of the numeric + numeric cases.  I also replaced calls to AvmCore::isString() with a new function atomIsString(), which tested only the atomKind tag, and therefore failed to distinguish actual string values from one of the multiple representations of the null.  This patch reverts this incorrect change, and adds a test case.

The patch is against tr-spicy, but should apply to TR as well.

Comment 8

[Steven Johnson]

Comment on attachment 502130 [details] [diff] [review]
Remove atomIsString() and use AvmCore::isString() in specialized add/concatenate helper functions

Nice. Subtle.

Comment 9

[Rick Reitmaier]

Is this a blocker for tracker 561368 (re comment 2) ?

Comment 10

[William Maddox]

Yes.

Comment 11

[Christopher Thilgen]

> Chris, when you're reporting results like these that are obtained with a plugin
> we need to know from which branch and changeset the plugin was built (and also
> whether it's a Release or Release-Debugger or other configuration).

Lars - I did not know what was appropriate to add to bugzilla - i added a note about the related Watson bug to look at - which has additional details - this was found in AIR - but it should also be reproducible in all variants of the player.

Comment 12

[William Maddox]

Pushed to tr-spicy:

http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/9989a55e791e

Comment 13

[Tamarin Bot]

changeset: 5825:56ee0c2cc62d
user:      William Maddox <[e-mail]>
summary:   Bug 623746 - Use correct test for String-valued atom in add/concatenate fastpath helper functions (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/56ee0c2cc62d