Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 316252 - [Mozilla][Browser]Wrong usage of C.free()
Summary: [Mozilla][Browser]Wrong usage of C.free()
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6.1   Edit
Assignee: Grant Gayed CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 349709 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-09 04:09 EDT by Zhou Xing CLA
Modified: 2011-06-29 11:31 EDT (History)
4 users (show)

See Also:
Silenio_Quarti: review+

Attachments
Sample project to reproduce the JVM crash (13.80 KB, application/octet-stream)
2010-06-10 04:07 EDT, Zhou Xing CLA 		no flags Details
JVM Crash stack info log file (19.17 KB, application/octet-stream)
2010-06-10 04:08 EDT, Zhou Xing CLA 		no flags Details
patch (14.18 KB, patch)
2010-06-14 12:34 EDT, Grant Gayed CLA 		no flags Details | Diff
patch #2 (12.97 KB, patch)
2010-07-12 15:29 EDT, Grant Gayed CLA 		no flags Details | Diff
patch #3 (15.45 KB, patch)
2010-07-27 12:29 EDT, Grant Gayed CLA 		no flags Details | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zhou Xing CLA 2010-06-09 04:09:25 EDT 
Build Identifier: M20090211-1700

In Eclipse SWT Mozilla Browser, Runnable "MozillaGetCookie" in Mozilla.java, uses "C.free()" method to release the "cookieString" which point to the memory space allocate by the XPCOM ToNewString()( reference to the source code of GetCookieString() ). See snippet:

MozillaGetCookie = new Runnable() {
			public void run() {
                                ...

				C.free (cookieString);
				
				...
			}
		};

Mozilla Developer Center : "https://developer.mozilla.org/En/Mozilla_internal_string_guide#Callee-allocated_Parameters" said that,  char *ToNewCString(nsACString&) return a buffer allocated using XPCOM's allocator instead of the traditional allocator (malloc, etc.). You should use NS_Free() to deallocate the result when you no longer need it.

So we should not use "C.free()" in this situation(Leads to crash sometimes) and use "XPCOM.nsfree()" instead. 


 

Reproducible: Always
Comment 1 Zhou Xing CLA 2010-06-10 04:07:53 EDT 
Created attachment 171612 [details]
Sample project to reproduce the JVM crash

This is a sample plug-in project to reproduce the JVM crash.
1. import the plug-in project
2. may change the "xulRuntimeBundleNameWin32" or "xulRuntimeBundleNameGtk" or "xulRuntimeBundleNameMacPPC" or "xulRuntimeBundleNameMacX86" parameter's value in "XULRunnerInitializer.java" according to your testing environment. 
3. "XULRunnerInitializer.isInstalledXulrunner()" is make sure the XULRunner Environment is correctly setup, and set the "org.eclipse.swt.browser.XULRunnerPath" with the correctly value;
4. run the plug-in project with "Eclipse Application", and Click the "Sample Meny" -> "Sample Action" to run the testing code
5. the JVM will soon crash due to the problem of wrong usage of "C.free"..
6. the crash stack info is in another attachment.
Comment 2 Zhou Xing CLA 2010-06-10 04:08:56 EDT 
Created attachment 171613 [details]
JVM Crash stack info log file
Comment 3 Grant Gayed CLA 2010-06-14 12:34:32 EDT 
Created attachment 171847 [details]
patch

test snippet:

public static void main(String[] args) {
	Device.DEBUG = true;
	//System.setProperty("org.eclipse.swt.browser.XULRunnerPath","e:\\xulrunner-1.9.2\\xulrunner");
    Display display = new Display();
    Shell shell = new Shell(display);
	new Browser(shell, SWT.MOZILLA);
	shell.open();
	final String cookieName = "cookie_1";
	final String TestPage = "file://test.html";
	Browser.setCookie(cookieName + "=cookie value", TestPage);
	for(int i = 0; i< 10000; i++) {
		System.out.println("i: " + i + " " + Browser.getCookie(cookieName, TestPage));
	}
    display.dispose();
}
Comment 4 Grant Gayed CLA 2010-07-12 15:29:17 EDT 
Created attachment 174090 [details]
patch #2

This revised patch does not assume that the Browser has a LocationProvider available to give the GRE location.
Comment 5 Grant Gayed CLA 2010-07-27 12:29:06 EDT 
Created attachment 175336 [details]
patch #3

This patch incorporates suggestions from SSQ's review.
Comment 6 Silenio Quarti CLA 2010-07-27 18:08:50 EDT 
+1 for patch #3
Comment 7 Grant Gayed CLA 2010-07-28 09:34:41 EDT 
fixed in the 3.6.1 and 3.7 streams > 2100728
Comment 8 Grant Gayed CLA 2011-06-29 11:31:17 EDT 
*** Bug 349709 has been marked as a duplicate of this bug. ***