Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 329518 - Need to add some try/exception in some SWT JNI call
Summary: Need to add some try/exception in some SWT JNI call
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.4.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M7   Edit
Assignee: Silenio Quarti CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-05 04:19 EDT by wuzhe CLA
Modified: 2019-12-20 05:18 EST (History)
8 users (show)

See Also:


Attachments
add try/exception for two JNI call (1.98 KB, patch)
2010-11-05 04:49 EDT, wuzhe CLA
no flags Details | Diff
JNI sample (To run as SWT application in Eclipse) (2.18 KB, text/plain)
2010-11-08 02:29 EST, Jun Yue Liu CLA
no flags Details
Sample code snipet to be added into OS.java (431 bytes, text/plain)
2010-11-08 02:30 EST, Jun Yue Liu CLA
no flags Details
JNI sample DLL (14.52 KB, application/octet-stream)
2010-11-08 02:32 EST, Jun Yue Liu CLA
no flags Details
patch (5.97 KB, patch)
2011-03-17 16:19 EDT, Silenio Quarti CLA
no flags Details | Diff
patch (6.02 KB, patch)
2011-03-17 17:03 EDT, Silenio Quarti CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wuzhe CLA 2010-11-05 04:19:51 EDT
Build Identifier: 3.4.2

While working for our customer, we identified that a JVM crash occur in JNI call COM.OleSetMenuDescriptor(), and another crash in JNI call OS.VtblCall(int, int).  we developed a fix, which add try/exception to wrap these 2 JNI call. Patch please refer to attachment.

The crashes are very difficult to reproduce. Fortunately, customer verified on 100+ machines for 3 weeks, and confirmed that our fixing is good to avoid those 2 kinds of crashes.

This fix patch would require SWT team to make a few changes on the SWT native code, i.e. org.eclipse.swt.win32.win32.x86/com.c  and org.eclipse.swt.win32.win32.x86/os.c and then rebuild the org.eclipse.swt.win32.win32.x86/swt-win32-xxxx.dll.

This fix would need to be applied to SWT 3.4.2, 3.5.2, and 3.6.2.

Is it possible for SWT team to help to apply the patch?

Reproducible: Sometimes
Comment 1 wuzhe CLA 2010-11-05 04:49:35 EDT
Created attachment 182458 [details]
add try/exception for two JNI call
Comment 2 Felipe Heidrich CLA 2010-11-05 10:16:17 EDT
That c code you changed is auto-generated, in order to release your patch in the product we would at least have to move it to a custom c code file.
That said, using __try __catch in C and throwing a java exception seems wrong to me (in 10 years we never had to do something like that). 

Also, I am afraid your patch can change the behaviour and break other places that where using regular error code handling and catching other types of exception (like SWTException), this patch would break this other clients because no one expects to get a java.lang.UnknownError from SWT.

Passing over to Silenio.
Comment 3 Jun Yue Liu CLA 2010-11-08 02:29:39 EST
Created attachment 182588 [details]
JNI sample (To run as SWT application in Eclipse)
Comment 4 Jun Yue Liu CLA 2010-11-08 02:30:58 EST
Created attachment 182589 [details]
Sample code snipet to be added into OS.java
Comment 5 Jun Yue Liu CLA 2010-11-08 02:32:13 EST
Created attachment 182590 [details]
JNI sample DLL
Comment 6 Jun Yue Liu CLA 2010-11-08 02:34:55 EST
I just attach a sample to demo the problem and solution.

With this sample, we saw that, when an GPF exception occur inside JNI
a) if we don't catch it, JVM will surely crash
b) if we catch and eat it siliently, JVM will not crash. This is not bad.
c) if we catch it and throw a Java exception (e.g. java.lang.UnknownError), JVM will not crash, and Java code will be aware of the native exception. 

Steps to use the sample are,
1. Launch Eclipse IDE
2. Import org.eclipse.swt and org.eclipse.swt.win32.win32.x86 as source project
3. Unzip JNIsampleDLL.zip and copy JNIsample-win32.dll to %workspace%\org.eclipse.swt.win32.win32.x86\.  (JNIsample-win32.dll can be rebult from source code JNIsample1.zip if needed.)
3. Open OS.java, and add content from file OS.added.java.
5. Create a SWT project with JniSample1.java
6. Launch JniSample1 as SWT application, and click buttone #1, #2, #3
7. Watch the IDE console
Comment 7 Felipe Heidrich CLA 2010-11-08 10:05:52 EST
The problem is that you are using try-catch in native code to fix the problem. 

For the real fix you need to understand _why_ exception happens in the native code. For example, is the code calling OleSetMenuDescriptor() with a bad paremeter, in what situation does it happen, can this situation be detected and the call to OleSetMenuDescriptor() avoided ?

What information do you have about the crash ?
Comment 8 Jun Yue Liu CLA 2010-11-09 02:51:31 EST
We saw 2 crashes. One is in JNI call to OleSetMenuDescriptor(), another is JNI call to IUnknow.release(), i.e. COM.VtblCall(2, address). Both calls happen in disposing an IE WebBrowser control instances.

With a number of months, we saw NO difference in calling sequence and parameters between the success and failure. They are Microsoft code. It make no sense to ask Microsoft to investigate and fix.  Because we can not reproduce the crash at will, the crash frequency is around 1 out of 3000 attempts. 

People are OK for Microsoft IE to crash now and then. For example, if you use IE for 30 days, you may get one crash out of 100*30 opening and closing. You may not complain Microsoft.  So far, in 10 years, we can not expect Microsoft to improve their code and fix that crash.

However, IBM customer surely complain IBM for this kind of crash. Given a call center with 500 seats, where the agents will open and close the browser control with much higher frequency, we will get more than 500 crash per months.  One crash mean one failure in call services.  The customer will take this as strong reason to challenge IBM product quality. 

In my understanding, an exception from JNI will surly crash the JVM. I see no drawback in catching the exception before return to Java code. Do you have any sample to demo that JVM could be still safe at such situation?  Or sample on handling the exception in 'regular' way?
Comment 9 Jun Yue Liu CLA 2010-11-16 00:40:38 EST
Felipe and Silenio, any update on this?
Comment 10 Silenio Quarti CLA 2011-03-17 16:19:13 EDT
Created attachment 191466 [details]
patch

This patch adds the try/catch block to those natives taking into consideration that the C code is auto-generated. It also:

- throws SWTError instead of UnknownError
- adds the native exception code to the java exception message
- changes the __except expression from "1,1" to EXCEPTION_EXECUTE_HANDLER. "1,1" did not make much sense to me reading the MSDN docs.

Patch applies to both 3.6.x and 3.7 branches.
Comment 11 Silenio Quarti CLA 2011-03-17 16:39:35 EDT
I released the above patch to HEAD (>20110317).

Note that we strongly believe that this is not the right approach to handle this problem. The segmentation fault should be tracked down and an appropriate fix should be provided by the appropriate part (either SWT or third part code).  I understand that given the circumstances, this is not a trivial task.   I consider this a temporary work around to avoid the hard crash.  You should still try to isolate the problem with your customer and provide us more information so that we can investigate it further.
Comment 12 Silenio Quarti CLA 2011-03-17 17:03:42 EDT
Created attachment 191474 [details]
patch

Previous patch did not compile on WIN64.