Instance variable is treated as class variable

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
19 years ago
19 years ago

People

(Reporter: gerrit.riessen, Assigned: rogerl)

Tracking

Details

(Reporter)

Description

19 years ago
----------- file: nesting_test.js -----------------
function Nest() 
{
  this.toString_return_value = "";
  this.contents = new Array();

  this.addElement = 
      new Function( "elem", "this.contents[this.contents.length] = elem; return 
(this);" );
  this.toString = NEST_toString_m;
}

function NEST_toString_m()
{
  this.toString_return_value = "";

  for ( var i = 0; i < this.contents.length; i++ ) 
    {
      this.toString_return_value += this.contents[ i ].toString() ;
    }

  return ( this.toString_return_value );
}

net = new Nest();
net.addElement( 'foobar' );
net.addElement( new Nest().addElement( 'hello world' ) );
net.addElement( 'snafu' );
---------------------------------------------------

when doing a net.toString() in the Rhino shell i get:
js> load( 'nesting_test.js' )
js> net.toString();
hello world
js>

but when i use IE(5.X) to test the code i get, what i think assume
to be correct: 'foobarhello worldsnafu' [same result using Netscape
4.X]

------------ file: nesting_test.html ---------------
<html>
<head>
<title></title>
<script language="JavaScript" src="nesting_test.js"></script>
</head>

<body onLoad="alert( net.toString() )">

</body>

</html>
----------------------------------------------------

I don't know what the ECMAScript Standard says, but i would assume
that the IE/Netscape result is the correct result?

Hope it helps,

Gerrit

Comment 1

19 years ago
Proposed fix:

Index: Interpreter.java
===================================================================
RCS file: /m/pub/mozilla/js/rhino/org/mozilla/javascript/Interpreter.java,v
retrieving revision 1.30
diff -u -r1.30 Interpreter.java
--- Interpreter.java    2000/03/13 18:27:25     1.30
+++ Interpreter.java    2000/05/07 05:45:50
@@ -1314,6 +1314,7 @@
         byte[] iCode = theData.itsICode;        
         int pc = 0;
         int iCodeLength = theData.itsICodeTop;
+        Scriptable itsThisObj = theData.itsThisObj;
         
         Object[] local = null;        // used for newtemp/usetemp etc.
         if (theData.itsMaxLocals > 0)
@@ -1659,8 +1660,7 @@
                         lhs = stack[stackTop];
                         stack[stackTop] = ScriptRuntime.callSpecial(
                                             cx, lhs, rhs, outArgs, 
-                                            theData.itsThisObj,
-                                            scope, name, i);
+                                            itsThisObj, scope, name, i);
                         pc += 6;
                         break;
                     case TokenStream.CALL :
@@ -1765,7 +1765,7 @@
                         stack[++stackTop] = null;
                         break;
                     case TokenStream.THIS :
-                        stack[++stackTop] = theData.itsThisObj;
+                        stack[++stackTop] = itsThisObj;
                         break;
                     case TokenStream.FALSE :
                         stack[++stackTop] = Boolean.FALSE;

The problem is that the InterpreterData object is referenced for the current 
'this' object, but recursive calls can overwrite that value. Copying it into a 
local prevents the problem.

This seems a bit inefficient. Roger, do you remember why it was done this way? 
Should we just add another parameter to the interpret method? 

If the change looks okay to you, Roger, could you check it in? I'll wait for my 
new email address to come before I request checkin privileges.
Assignee: norrisboyd → rogerl
(Assignee)

Comment 2

19 years ago
I think the only reason I didn't want it passed around was that it uses up a 
slot and a low-index Java register. The testing I did with various JITs 
indicated that they were favoring the parameters and earlier-declared variables 
(for register allocation? - seems kind of hard to believe on x86, but who 
knows). I checked in your fix, but moved the local 'itsThisObj' further down in 
the declaration order as it doesn't get used that freqeuntly.
Status: NEW → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.