Memory leak when using Java reflection & shared scopes

RESOLVED FIXED

Status

Rhino
Core
--
major
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Attila Szegedi, Assigned: Attila Szegedi)

Tracking

Details

Attachments

(1 attachment)

2.26 KB, patch
Hannes Wallnoefer
: review+
Hannes Wallnoefer
: review+
Hannes Wallnoefer
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
From the e-mail Kieran Topping sent to the rhino mailing list: 

When using the default WrapFactory, ClassCache holds a reference to whatever the top level scope was at the time the class-in-question was cached.
Usually, this is truly the top-level scope, which doesn't cause any problems.
However, if you're using a shared "global" scope, as per:
https://developer.mozilla.org/En/Rhino_documentation/Scopes_and_Contexts#Sharing_Scopes
then this sharedScope isn't used by ClassCache; it's actually your "newScope", as follows:

Scriptable sharedScope = ... create shared scope.

create a scope that uses the shared scope
  Scriptable newScope = cx.newObject(sharedScope);
  newScope.setPrototype(sharedScope);
  newScope.setParentScope(null);
then run a script in newScope, in particular one that ends up wrapping a java class, either via Context.javaToJS() or actually in your javascript. Even if you don't do this explicitly, things like throwing a java exception will end in WrapFactory being used.

After the above, you can end up with the following:

sharedScope
-stores-reference-to-> classCacheInstance

classCacheInstance
-stores-reference-to-> newScope.

This means that newScope will /never /become eligible for garbage collection. If you created some large objects in newScope then these will never be GC-ed either. Hence....memory leak.
(Assignee)

Updated

9 years ago
Assignee: nobody → szegedia
(Assignee)

Comment 1

9 years ago
Created attachment 379424 [details] [diff] [review]
Patch that fixes the bug

Attached a patch to fix the bug. Basically, have the JavaMembers objects bind to the scope of the ClassCache instead of the current top-level scope (as the ClassCache scope can be the current top-level scope's prototype).
(Assignee)

Updated

9 years ago
Attachment #379424 - Flags: review?(hannes)
(Assignee)

Comment 2

9 years ago
Committed patch to CVS HEAD:

***
cvs ci -m "Fix for #494665: Memory leak when using Java reflection & shared scopes" -l "/Rhino/src/org/mozilla/javascript/JavaMembers.java" "/Rhino/src/org/mozilla/javascript/ClassCache.java"
    Checking in src/org/mozilla/javascript/ClassCache.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ClassCache.java,v  <--  ClassCache.java
    new revision: 1.20; previous revision: 1.19
    done
    Checking in src/org/mozilla/javascript/JavaMembers.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/JavaMembers.java,v  <--  JavaMembers.java
    new revision: 1.80; previous revision: 1.79
    done
ok (took 0:04.203)
***
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 3

6 years ago
Comment on attachment 379424 [details] [diff] [review]
Patch that fixes the bug

Review of attachment 379424 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't seen this at the time, and bugzilla has recently started sending me me reminder emails about it, so yes, this looks perfectly fine :)
Attachment #379424 - Flags: review?(hannesw)
Attachment #379424 - Flags: review+
Attachment #379424 - Flags: feedback+
You need to log in before you can comment on or make changes to this bug.