261468 - SpiderMonkey LiveConnect calls free() instead of JS_smprintf_free(), problem with static linking

Status: RESOLVED FIXED

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

Description

[Joacim Zschimmer]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: 

LiveConnnect frees memory got with JS_smprintf() with free(), but it should 
use JS_smprintf_free().

This works fine when js32.dll and liveconnect.dll are linked with msvcr71.dll 
but fails when linked with static C runtime, because malloc() and free() are 
called in different runtime contexts. Yes, you know, since this is why 
JS_smprintf_free() exists.


Two Examples are

- report_java_initialization_error() in jsj.c, line 88
- JavaPackage_resolve() in jsj_JavaPackage.c, line 260

I think, all uses of free() in liveconnect should be examined.

Reproducible: Always
Steps to Reproduce:
1. build spidermonkey and liveconnect with /MT (instead of /MD)
2. call JSJ_ConnectToJavaVM(), JSJ_InitJSContext()
3. init_netscape_java_classes() warns by calling
4. report_java_initialization_error()
5. where the Debugger detects the wrong free().




report_java_initialization_error()
JavaPackage_resolve()

and possibly more functions calling free() after JS_smprintf()

Comment 1

[Brendan Eich [:brendan]]

Use the right component.

/be

Comment 2

[Pete Zha]

mass reassign to Alfred

Comment 3

[Alfred Peng]

Attachment: Patch v1 for Liveconnect

The xpconnect module also seems to be affected by this problem.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 250701 [details] [diff] [review]
Patch v1 for Liveconnect



>Index: js/src/liveconnect/jsj_JavaObject.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/liveconnect/jsj_JavaObject.c,v
>retrieving revision 1.43
>diff -u -p -8 -r1.43 jsj_JavaObject.c
>--- js/src/liveconnect/jsj_JavaObject.c	12 Dec 2006 10:45:06 -0000	1.43
>+++ js/src/liveconnect/jsj_JavaObject.c	6 Jan 2007 17:55:15 -0000
>@@ -347,18 +347,24 @@ void
> jsj_DiscardJavaObjReflections(JNIEnv *jEnv)
> {
>     JSJavaThreadState *jsj_env;
>     char *err_msg;
> 
>     /* Get the per-thread state corresponding to the current Java thread */
>     jsj_env = jsj_MapJavaThreadToJSJavaThreadState(jEnv, &err_msg);
>     JS_ASSERT(jsj_env);
>-    if (!jsj_env)
>+    if (!jsj_env) {
>+        if (err_msg) {
>+            jsj_LogError(err_msg);
>+            JS_smprintf_free(err_msg);
>+        }
>+
>         return;
>+    }
> 

Could JS_ASSERT(!err_msg); here.

Looks good, r=me in any case.  Sorry I missed this in my queue for so long.

/be

Comment 5

[Alfred Peng]

Comment on attachment 250701 [details] [diff] [review]
Patch v1 for Liveconnect

Brendan, I'll add the JS_ASSERT to the patch after jst's review.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 250701 [details] [diff] [review]
Patch v1 for Liveconnect

sr=jst

Comment 7

[Alfred Peng]

Attachment: The patch checked in

Add the JS_ASSERT line.

Comment 8

[Alfred Peng]

=>FIXED