Bug 12636 - Incorrect JNI handling of arrays causes browser crash
Summary: Incorrect JNI handling of arrays causes browser crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: David Carson
URL:
Keywords: InRadar
Depends on:
Blocks: 12731
  Show dependency treegraph
 
Reported: 2007-02-06 12:24 PST by David Carson
Modified: 2007-02-19 20:57 PST (History)
2 users (show)

See Also:


Attachments
Test case. (2.76 KB, application/octet-stream)
2007-02-06 12:27 PST, David Carson
no flags Details
Patch (12.95 KB, patch)
2007-02-07 14:15 PST, David Carson
mjs: review+
Details | Formatted Diff | Diff
updated test case (3.45 KB, application/octet-stream)
2007-02-07 14:19 PST, David Carson
no flags Details
Now includes manual test (21.77 KB, patch)
2007-02-09 07:31 PST, David Carson
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Carson 2007-02-06 12:24:57 PST
When I try to call a Java function that takes an array, the method signature is not created corrected, resulting in a crash when invoking the JVM.

Java function:
public void arrayFunction(String [] array)

Correct JNI signature:
([Ljava/lang/String;)V

WebKit's generated signature:
(L[Ljava/lang/String;;)V

presently, jni_utility is handling an array like any other generic java object, and thus wrapping it in a L<generic_object>;

GDB trace below:
Invalid memory access of location 00000000 eip=9b6833a9
Program received signal:  "EXC_BAD_ACCESS".
(gdb) where
#0  0x9b6833a9 in JVM_MonitorWait ()
#1  0x9b7072f9 in JVM_IsConstructorIx ()
#2  0x17434a46 in MethodSwizzle ()
#3  0x00521120 in KJS::Bindings::dispatchJNICall (targetAppletView=0x2116a80, obj=0x1751043c, isStatic=false, returnType=void_type, methodID=0x0, args=0x1712d7f0, result=@0xbfffedf8, exceptionDescription=@0xbfffedf4) at /Users/dacarson/WebKit/JavaScriptCore/bindings/jni/jni_objc.mm:54
#4  0x00516737 in KJS::Bindings::JavaInstance::invokeMethod (this=0x21497f0, exec=0xbffff130, methodList=@0x1712d7b0, args=@0xbfffef38) at /Users/dacarson/WebKit/JavaScriptCore/bindings/jni/jni_instance.cpp:154
#5  0x0051729b in KJS::RuntimeMethod::callAsFunction (this=0x1712d780, exec=0xbffff130, thisObj=0x170a8160, args=@0xbfffef38) at /Users/dacarson/WebKit/JavaScriptCore/bindings/runtime_method.cpp:89
#6  0x004f8160 in KJS::JSObject::call (this=0x1712d780, exec=0xbffff130, thisObj=0x170a8160, args=@0xbfffef38) at /Users/dacarson/WebKit/JavaScriptCore/kjs/object.cpp:97
#7  0x004edfe7 in KJS::FunctionCallDotNode::evaluate (this=0x1713ad00, exec=0xbffff130) at /Users/dacarson/WebKit/JavaScriptCore/kjs/nodes.cpp:780
Comment 1 David Carson 2007-02-06 12:27:36 PST
Created attachment 12981 [details]
Test case.

Attachment is a zip file containing HTML and Java class file (with source) that shows the crash.
Comment 2 Geoffrey Garen 2007-02-06 15:13:02 PST
Repro crash => P1.
Comment 3 Maciej Stachowiak 2007-02-06 23:26:42 PST
<rdar://problem/4981001>
Comment 4 David Carson 2007-02-07 07:11:32 PST
The fix is not as easy as I initially thought. I thought that if I could just separate recognition of Array parameters from generic object parameters, all would be good.
There is more to it. Currently the JNI bindings don't know how to generate a Java Array class from a JS Array. Though, the reverse is true, the bindings can generate a JS Array from a Java Array. This is used when the return type is a Java Array.
Comment 5 David Carson 2007-02-07 14:15:52 PST
Created attachment 13016 [details]
Patch

This patch creates a new internal type, array_type.
This patch also handles the creation of Java arrays for the given Java parameter array type and fills it with the Javascript Array content.
Comment 6 David Carson 2007-02-07 14:19:57 PST
Created attachment 13017 [details]
updated test case

This test case also tests all other Java array types, eg byte[], char[], etc. The previous test case only tested String[].
Comment 7 Maciej Stachowiak 2007-02-07 18:14:52 PST
Comment on attachment 13016 [details]
Patch

r-
Comment 8 Maciej Stachowiak 2007-02-07 18:15:26 PST
Comment on attachment 13016 [details]
Patch

Er, I meant, r=me

Is there a way to make a test case for this (even a manual test)?
Comment 9 David Carson 2007-02-07 19:08:54 PST
(In reply to comment #8)
> (From update of attachment 13016 [details] [edit])
> Er, I meant, r=me
> 
> Is there a way to make a test case for this (even a manual test)?
> 

There is a manual test case attached. I will move it to the WebCore\manual-tests and make it part of the patch.
Comment 10 David Carson 2007-02-09 07:31:24 PST
Created attachment 13083 [details]
Now includes manual test

Nothing has changed with respect to the code. I have added the test case to the manual-tests and included that as part of the patch, as mjs suggested.
Comment 11 Maciej Stachowiak 2007-02-09 16:12:02 PST
Comment on attachment 13083 [details]
Now includes manual test

r=me
Comment 12 Sam Weinig 2007-02-10 11:29:33 PST
I am having trouble applying the patch as provided.  I am getting an error "svn: 'WebCore/manual-tests/resources/ArrayParameterTestApplet.class' does not exist". 
Comment 13 David Kilzer (:ddkilzer) 2007-02-10 17:57:51 PST
(In reply to comment #12)
> I am having trouble applying the patch as provided.  I am getting an error
> "svn: 'WebCore/manual-tests/resources/ArrayParameterTestApplet.class' does not
> exist".

Sam, did you use svn-apply to apply the patch?

I just noticed a bug in svn-apply that won't apply the binary patch on Attachment 13083 [details] because it doesn't have a blank line after the end of the binary segment.

Comment 14 Sam Weinig 2007-02-10 18:02:14 PST
Dave, yeah, I used svn-apply.  Is there easy way for me (or you or anyone) to work around this in the meantime.  Like say, just adding the blank line?
Comment 15 David Kilzer (:ddkilzer) 2007-02-10 18:29:04 PST
Committed revision 19559.