197682 - Memory leak via static filed in generated bytecode

Status: VERIFIED FIXED

(Rhino Graveyard :: Compiler, defect)

Description

[Igor Bukanov]

Currently Rhino optimizer with optimization level set to 1 or higher adds a
static fields to classes representing compiled script functions to perform
function call optimization.

These static fields holds references to script runtime objects and as long as
the loaded classes together with their static fields are not garbage collected,
the runtime structures will remain in JVM memory. It is not a problem in typical
situations where script is created and immediately executed since in such cases
generated classes can be GCed after script execution allowing to GC the runtime
as well.

But in the case of JavaScript compiler this is no longer true since then
generated classes representing a compiled script are stored as class files and
can be loaded via system class loader which means that object stored in static
fields will never be garbage collected. It leaks references to script runtime
structures and if script is loaded more then once, it will disable the
optimization altogether since the static fields will hold the reference for
wrong runtime structures.

P.S. From now I will use [e-mail] for mozilla.org and other
communications outside my work since due to various reorganization at work
[e-mail] and [e-mail] may or may not work, a provider that I used
for private mail (igor at mir2 org) made an "improvement" that stopped mail
working for days and [e-mail] that I sometime used for various mail
lists to prevent spam is no longer necessary since I finally got good enough
spam filters.

Comment 1

[Igor Bukanov]

Attachment: Script test case where optimizer uses optimization leading to creation of static field

Comment 2

[Igor Bukanov]

Attachment: Java program to demonstrate a memory leak with classes created by JavaScript compiler for the the previous attachment

Comment 3

[Igor Bukanov]

Steps to reproduce the problem:

1. Compile the script from the first attachment by JavaScript compiler with an
optimization level set to 1 or higher:

java -classpath js.jar  org.mozilla.javascript.tools.jsc.Main -opt 1 script.js

2. Compile a Java program form the second attachment:

javac -classpath .:js.jar StaticFieldProblem.java

where . is included to the classpath so javac finds classes generated by
JavaScript compiler. 

The program instantiate and execute an instance of the class script created by
JavaScript compiler and then calls the garbage collector explicitly to see if an
instance of CustomScope which the program as a scope object is finalized.

3. Run StaticFieldProblem:

javac -classpath .:js.jar StaticFieldProblem

It currently prints 
finalizeCount=0
indicating that a reference to CustomScope is not GCed. 

Comment 4

[Igor Bukanov]

Attachment: Making references to direct call targets an instance fields of the main script class.

The static fields generated by the compiler store references to objects
representing JavaScript functions that are targets of direct call optimization.

They are used to make sure during optimized calls that a function reference
still points to the original function object.  For example, in the script from
the first attachment:

function a(x)
{
	b(x);
}

function b(x)
{
	return x == 0;
}

the code generated for a() to call b() checks if the property b still holds the
original function object and if it does, a() will use an optimized calling
conversion to call b(). 

The patch replaces static fields by an instance fields in the class
representing the main script object. This is quite significant change since it
adds code to store/query a field that refers to the main class to each function
class and changes the way the direct fields are initialized. Still the patch
itself is not that big since during the work on the patch I found that some of
refactoring that are necessary for the patch is make sense to apply on its own
and that part is already in CVS.

Comment 5

[Igor Bukanov]

I commited the patch and now the test case prints what it should:

finalizeCount=1

Comment 6

[Phil Schwartau]

Marking Verified.

I also get 'finalizeCount=1' as the output of the steps to reproduce
given in Comment #3. 

However, that's what I got before the patch was committed, as well!
Is it possible the results are machine-dependent?

Comment 7

[Igor Bukanov]

Phil, did you re-run the JavaScript compiler with optimization set to 1 against
the scripts when you run the test with and without patch? It is strange that you
get that finalizeCount=1 in both cases. Without the patch the script compiler
has to produce the code with static fields that are never cleared and AFAIK
objects reachable through such fields should not be subject of garbage
collection since JDK 1.1 in this case.

Comment 8

[Phil Schwartau]

Igor: you're right! I must have made some mistake the other day. When
I try the testcase today against rhino1_5R4pre (i.e. before the fix),
I get finalizeCount=0 just like you did.

Sorry for the trouble. I will attach my results below, verifying
that the bug is fixed -

Comment 9

[Phil Schwartau]

Attachment: Verification results: rhino1_5R4pre vs. rhino1_5R4

Comment 10

[Igor Bukanov]

Thanks Phil!

Comment 11

[Igor Bukanov]

Targeting as resolved against 1.5R5