Closed Bug 38384 Opened 24 years ago Closed 24 years ago

Instance variable is treated as class variable

Categories

(Rhino Graveyard :: Core, defect, P3)

Product:
Rhino Graveyard
Component:
Core
Platform:
x86
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gerrit.riessen, Assigned: rogerl)

Details

----------- 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
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
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
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.