182028 - Calling has() in get() of a ScriptableObject causes getter function to not be called

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Morten Moeller]

If has() is called within get() of a ScriptableObject, lastAccess is set to the
slot found by has. When my get() then calls super.get() into the
ScriptableObject, the lastAccess.value is returned to me and the getter method
is not called. The cahched slot object (lastAccess) does not check for a getter
method.

The following patch fixes the problem:

[root@mmoeller rhino]# cvs diff -u src/org/mozilla/javascript/ScriptableObject.java
Index: src/org/mozilla/javascript/ScriptableObject.java
===================================================================
RCS file:
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v
retrieving revision 1.55
diff -u -r1.55 ScriptableObject.java
--- src/org/mozilla/javascript/ScriptableObject.java    24 Sep 2002 14:10:53
-0000      1.55
+++ src/org/mozilla/javascript/ScriptableObject.java    26 Nov 2002 17:20:03 -0000
@@ -160,7 +160,12 @@
         Slot slot = lastAccess; // Get local copy
         if (name == slot.stringKey) {
             // Cache match, check if it was not deleted
-            if (slot.wasDeleted == 0) { return slot.value; }
+            if (slot.wasDeleted == 0) {
+                if ((slot.flags & Slot.HAS_GETTER) != 0) {
+                    return getByGetter((GetterSlot) slot, name, start);
+                }
+                return slot.value;
+            }
         }

         int hashCode = name.hashCode();

Comment 1

[Phil Schwartau]

cc'ing Igor to consider this patch -

Comment 2

[Igor Bukanov]

The patch is right. In think it also makes sence to update the get method to
cache last accessed slot even it has getter/setter, but that is something to
consider later.

I can commit the patch later next week as I will be away for few days, but if
Norris would like to freeze for 1.5R4 sooner, then it is a good idea to include
the patch before that.

Comment 3

[Igor Bukanov]

I commited a fix similar to the one outlined in the patch together with a bigger
set of changes to remove a potential race condition in ScriptableObject.setBySetter.

Comment 4

[Igor Bukanov]

It is possible to see the regression via simple a test case which uses the fact
that currently java object for LiveConnect is initialized via getter/setter:

function f() {
	// put java into cache via ScriptableObject.has
	this.hasOwnProperty("java");
	// use java from cache via ScriptableObject.get
	return this.java;
}

var ok = (f() === java);
print(ok);

The test case printed false before the commit and true now. As the test is very
implementation-specific I doubt it would have any sence to add it to the test suite.

Comment 5

[Phil Schwartau]

Marking FIXED. I tested this on WinNT 4.0 using JRE 1.4.1-b21.

As Igor stated, the testcase above returned 'false' before 
the patch was committed, and now returns 'true'

I tried the testcase in the interpreted and compiled modes of Rhino,
using both the -f option to the Rhino shell, and the load() function.

Comment 6

[Phil Schwartau]

Marking Verified -

Comment 7

[Igor Bukanov]

Targeting as resolved against 1.5R4