Closed Bug 341728 Opened 15 years ago Closed 9 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: mike.lei, Assigned: mike.lei)

Details

(Keywords: fixed1.8.0.9, fixed1.8.1)

Attachments

(3 files)

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
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 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.
Attachment #225853 - Flags: review?(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 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
Attachment #225853 - Flags: review?(brendan) → review+
(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?
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?
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #225853 - Flags: approval1.8.1?
Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

a=schrep for drivers.
Attachment #225853 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

This has been checked in to MOZILLA_1_8_BRANCH
fixed1.8.1 keyworld and --> RESOLVED FIXED?
$SUBJECT is fixed but comment #2 is still unresolved. I'm not sure if that can be closed as resolved because of this.
Keywords: fixed1.8.1
Probably should open a new bug on that.
Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

Low risk leak fix, wanted for distros.
Attachment #225853 - Flags: approval1.8.0.9?
Comment on attachment 225853 [details] [diff] [review]
patch to fix local reference

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #225853 - Flags: approval1.8.0.9? → approval1.8.0.9+
Assignee: live-connect → mike.lei
Whiteboard: [checkin needed (1.8.0 branch)]
Checked in on 1.8.0 branch.
Keywords: fixed1.8.0.9
Whiteboard: [checkin needed (1.8.0 branch)]
Product: Core → Core Graveyard
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".
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.