154693 - Interpreted mode doesn't grok different functions on different objects

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Erik Strelzoff]

I have a script where I define a function on an object, I construct the object 
twice and then Rhino thinks that the the functions in the two different objects 
are identical.  This only occurs in interpretted mode 
(Context.setOptimizationLevel(-1)).  I'll attach a test case to demonstrate the 
problem.

Comment 1

[Erik Strelzoff]

Attachment: Problem script & test case.

Comment 2

[Erik Strelzoff]

I forgot to mention that I also tried this against the latest release version 
(1.5 R3) and it demonstrates the same problem.

Comment 3

[Igor Bukanov]

Here is a simpler example to expose the problem in Rhino shell:

function f() { 
	function nested() { }  
	return nested; 
}

var f1 = f(); 
var f2 = f(); 
 
var ok = (f1 != f2);

print(ok ? "GOOD" : "BAD");
 

Comment 4

[Phil Schwartau]

Igor: thank you for this test! I have added it to the JS testsuite:

      mozilla/js/tests/js1_5/Scope/regress-154693.js

It is currently passing in SpiderMonkey and Rhino compiled mode,
but failing in Rhino interpreted mode, as Erik reported -

Comment 5

[Igor Bukanov]

Attachment: Fix for omj/Interpreter.java

The reason for the bug is the current code in Interpreter.interpret that
instead of creating new InterpretedFunction for each nested function statement
on function entry, put to the activation scope a function currently stored in
InterpretedData.itsNestedFunction. 

I believe the patch fixes that (Rhino passes all tests with it) but as it
removes redundant generation of CLOSURE statements for each nested function
statement, as it seems they are only needed for function expression and
function expression statements, it would be nice to here something from someone
who wrote initial code.

Comment 6

[Igor Bukanov]

Attachment: Reduced version of the previous patch as I commited part of it

I commited essentially cosmetics part of the previous patch as it reduces
codesize and makes a real bug fix smaller which still needs some considerations
before the commit.

Comment 7

[Igor Bukanov]

Attachment: Another patch reduction due to a partial commit

I commited another part of the patch where it removes closure icode generation
for function statements. It seems for me it was a bug on its own.

 diff -u -r1.115 Interpreter.java
--- Interpreter.java	4 Dec 2002 09:49:07 -0000	1.115
+++ Interpreter.java	5 Dec 2002 20:57:58 -0000
@@ -271,13 +271,20 @@
	 switch (type) {
 
	     case TokenStream.FUNCTION : {
-		 Node fn = (Node) node.getProp(Node.FUNCTION_PROP);
-		 int index = fn.getExistingIntProp(Node.FUNCTION_PROP);
-		 iCodeTop = addByte(TokenStream.CLOSURE, iCodeTop);
-		 iCodeTop = addIndex(index, iCodeTop);
-		 itsStackDepth++;
-		 if (itsStackDepth > itsData.itsMaxStack)
-		     itsData.itsMaxStack = itsStackDepth;
+		 FunctionNode fn
+		     = (FunctionNode) node.getProp(Node.FUNCTION_PROP);
+		 if (fn.itsFunctionType != FunctionNode.FUNCTION_STATEMENT) {
+		     // Only function expressions or function expression
+		     // statements needs closure code creating new function
+		     // object on stack as function statements are initialized
+		     // at script/function start
+		     int index = fn.getExistingIntProp(Node.FUNCTION_PROP);
+		     iCodeTop = addByte(TokenStream.CLOSURE, iCodeTop);
+		     iCodeTop = addIndex(index, iCodeTop);
+		     itsStackDepth++;
+		     if (itsStackDepth > itsData.itsMaxStack)
+			 itsData.itsMaxStack = itsStackDepth;
+		 }
		 break;
	     }

Comment 8

[Igor Bukanov]

I commited the fix, so the bug can be marked as fixed.

Comment 9

[Phil Schwartau]

Marking FIXED -

Comment 10

[Phil Schwartau]

Marking Verified Fixed.

The above testcase used to pass in Rhino compiled mode,
but fail in interpreted mode. Now it passes in both -

Comment 11

[Igor Bukanov]

Targeting as resolved against 1.5R4