305753 - NativeJavaMethod objects have incorrect parent when using parent scopes

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Attila Szegedi]

User-Agent:       Opera/8.02 (Windows NT 5.0; U; en)
Build Identifier: 

I'm using a shared scope -- all scopes have it as their prototype. The 
ClassCache is associated with the shared scope. Yet, when the Java binding 
machinery creates new instances of NativeJavaMethod in JavaMembers.lookupClass(
), the newly created NativeJavaMethod objects will have the NativeJavaObject's 
parent scope as their parent scope. However, this parent scope is temporary and 
should get garbage collected, while the NativeJavaMethod persists in the 
ClassCache associated with the shared scope, and holds a strong reference to the 
temporary scope.

As a solution, I propose that ClassCache stores the information about the scope 
it is associated with (i.e. the shared scope), and JavaMembers are created 
against that scope.

Reproducible: Always

Comment 1

[Attila Szegedi]

Attachment: Patch that fixes the bug

Attached a patch that fixes the bug. Also note I fixed the return value of
ClassCache.associate()

Comment 2

[Attila Szegedi]

Attachment: Same patch, but with corrected formatting of opening brace

Attached again - one opening brace was misplaced. I'll commit it early friday
if there are no objections to it.

Comment 3

[Igor Bukanov]

(In reply to comment #0)
> As a solution, I propose that ClassCache stores the information about the scope 
> it is associated with (i.e. the shared scope), and JavaMembers are created 
> against that scope.
> 


Here is some history behind ClassCache. Before its introduction Rhino stored
cached instances of various objects for LiveConnect support in static variables.
That was wrong leading not only to memory leaks but also potential security
breaches via accessing independent runtime objects through __parent__/__proto__
properties.

During the transition from static to ClassCache I hoped to separate caching of
Java reflection helper with their JavaScript wrappers. In this way it would be
possible to share ClassCache between independent scopes and save memory since
wrappers would be created only when necessary (it was actual for the project I
worked).

The idea was to create extremely lightweight version of NativeJavaMethod which
would be created each time when one need to accesses, for example, println, in
java.lang.System.out. Then the wrapper for System.out would store only reference
to small reflection helper object for println function without any scope refs
allowing to cache that between independent scopes. 

Moreover, with some code generation tuning that wrapper object would be
unnecessary in most cases since in
java.lang.System.out.println() the wrapper is not visible and can be avoided
completely.

The problem was with code like java.lang.System.out.println.foo = "bar" that
LiveConnect allowed. It requires to store properties in wrappers persistently
and I was never able to come up with simple solution without using weak
hashtables which were not available on the JVM for the project.

Given that it is only right to store scope in ClassCache and document that
sharing of ClassCache between scopes are not allowedand NEVER was.

Comment 4

[Attila Szegedi]

(In reply to comment #3)
> 
> Given that it is only right to store scope in ClassCache and document that
> sharing of ClassCache between scopes are not allowedand NEVER was.
> 

Sorry, I couldn't parse from what you wrote whether my patch is okay. :-) 

I'm not sharing ClassCache between scopes -- I'm having a "master" scope 
(scope_0) that is a prototype to multiple subscopes (scope_1...scope_n). The 
lookup of "associated values" in ScriptableObject is such that ClassCache.
get(scope_1) == ... == ClassCache.get(scope_n) == ClassCache.get(scope_0). 
Strictly speaking, that ClassCache belongs to scope_0. Actually, I find it OK 
this way, as the Function prototype (which is the prototype object of 
NativeJavaMethod objects) also lives in scope_0 (it is the one that has all 
standard objects initialized in it). 

It'd be rather inefficient if each scope_i (i > 0) would have its own 
ClassCache, as that's mean each would need to do reflective discovery on its 
own, and these scopes are continually created and destroyed on 50 threads as 
they process their work units.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> 
> Sorry, I couldn't parse from what you wrote whether my patch is okay. :-) 

The patch is OK and fixes the real bug! What I wanted to say is that sharing of
ClassCache instances between *unrelated* scopes never worked since ClassCache
does refer scope reference through NativeJavaMethod as you pointed out. In fact
for thos reason I would even sugest to add a new ClassCache(scope) constructor
and deprecate default constructor/associate method there.

Comment 6

[Attila Szegedi]

(In reply to comment #5)
> for thos reason I would even sugest to add a new ClassCache(scope) constructor
> and deprecate default constructor/associate method there.
> 

Will do -- for now I just committed the existing patch. As usual, there are some 
urgencies that need to be tended to in my paid work (you can tell that I 
discovered this bug while hunting for memory leaks with a profiler -- this was 
sort of a memory leak although fortunately not an unbounded one, but bound by 
the total number of Java methods the scripts invoke), as it retained lots of 
parent scopes that were supposed to get garbage collected), so I'll get around 
to beautifying the solution a bit later.

Comment 7

[Igor Bukanov]

Closing this: potential improvments can go to a separated bug.