341728 - Local reference leak in nsCLiveconnect::Call when Java call javascript

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Java: Live Connect, defect)

Description

[Mike Lei]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

When Java call Javascript, nsCLiveConnect::Call() is called. Local references are leaked when converting java args to js args, see below

    for (arg_num = 0; arg_num < argc; arg_num++) {
        jobject arg = jEnv->GetObjectArrayElement(java_args, arg_num);

        if (!jsj_ConvertJavaObjectToJSValue(cx, jEnv, arg, &argv[arg_num]))
            goto cleanup_argv;
        JS_AddRoot(cx, &argv[arg_num]);
    }

the array element arg is local reference and is not released during for loop. This can cause JS to hold the local reference to Java Object. If Java Object is very big, serveral calls will make JVM out of memory.  see testcase in attachment. click button about 8 times , JVM will be out of memory in firefox 1.5.0.4


Reproducible: Always

Steps to Reproduce:
1. run testcase, open java console to watch
2. click "Test" button serveral times
3. OutOfMemoryError seen in java console

Actual Results:  
OutOfMemoryError on Java Console

Expected Results:  
No OutOfMemoryError on Java Console

I tested on Sun JRE 5.0U7  and mustang beta

Comment 1

[Mike Lei]

Attachment: use this file to load the applet

Comment 2

[Mike Lei]

Attachment: Java Applet source code

compile this file to use with index-js.html.  This testcase demostrate the local reference leak problem. However, even the local reference issue is fixed, liveconnect still has cache to contain the global reference to java object. So may  be JS GC should run even JS memory is not low because the reference in JS memory prevents JVM from free Java Object

Comment 3

[Mike Lei]

Attachment: patch to fix local reference

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

That's not my code but it makes sense to get it in if it's correct.
Module owner seems to be Kyle Yuan but I couldn't find his bugmail.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (From update of attachment 225853 [details] [diff] [review] [edit])
> That's not my code but it makes sense to get it in if it's correct.
> Module owner seems to be Kyle Yuan but I couldn't find his bugmail.

Kyle is long-gone.  Cc'ing Xiao Bin, who can remind us of the latest owner.  The despot database behind http://www.mozilla.org/owners.html must be out of date, yet again.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

Absent a module owner or peer, this is good enough.

Nit: the canonical name for |ret| is |ok|, so change that to match prevailing style.

Note that none of the failures in this function except for the last one (exiting java) result in a non-NS_OK return.  But that's a separate bug.

/be

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> compile this file to use with index-js.html.  This testcase demostrate the
> local reference leak problem. However, even the local reference issue is
> fixed, liveconnect still has cache to contain the global reference to java
> object. So may  be JS GC should run even JS memory is not low because the
> reference in JS memory prevents JVM from free Java Object

Perhaps the JVM could signal us when it's low on memory, so we can run JS GC. I think triggering JS GC on our side would be easy; how can the JVM tell us when it's low on memory?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Suppose that while LiveConnect is running, we have a thread that checks every second whether Runtime.getRuntime().freeMemory() is less than 1MB, and if so, triggers a JS GC. Would that work?

Comment 9

[Mike Lei]

(In reply to comment #8)
> Suppose that while LiveConnect is running, we have a thread that checks every
> second whether Runtime.getRuntime().freeMemory() is less than 1MB, and if so,
> triggers a JS GC. Would that work?
> 
The better way is to use a new feature in j2se 5.0 to detect low memory. JS can add a listener to receive the low memory notification , then release java object
.
see http://java.sun.com/j2se/1.5.0/docs/guide/management/mxbeans.html#low_memory

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

This has been checked in to trunk yesterday and I would propose to take it also for 1.8 branch.

Comment 11

[Mike Schroepfer]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

a=schrep for drivers.

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

This has been checked in to MOZILLA_1_8_BRANCH

Comment 13

[Steve England [:stevee]]

fixed1.8.1 keyworld and --> RESOLVED FIXED?

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

$SUBJECT is fixed but comment #2 is still unresolved. I'm not sure if that can be closed as resolved because of this.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably should open a new bug on that.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

Low risk leak fix, wanted for distros.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in on 1.8.0 branch.

Comment 19

[Benjamin Smedberg]

Firefox code moved from custom Liveconnect code to the NPAPI/NPRuntime bridge a while back. Mass-closing the bugs in the liveconnect component which are likely invalid. If you believe that this bug is still relevant to modern versions of Firefox, please reopen it and move it the "Core" product, component "Plug-Ins".