RESOLVED FIXED 62389
[V8] JavaMethodJobject::signature() incorrectly encodes arguments of type JavaTypeString 
https://bugs.webkit.org/show_bug.cgi?id=62389
Summary [V8] JavaMethodJobject::signature() incorrectly encodes arguments of type Jav...
Steve Block
Reported 2011-06-09 11:57:54 PDT
On V8, JavaTypeString is now distinct from JavaTypeObject, but both need to represented by a JNI class description when forming a JNI method signature.
Attachments
Patch (2.09 KB, patch)
2011-06-10 04:01 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (2.13 KB, patch)
2011-07-20 07:40 PDT, Steve Block 		tonyg: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Steve Block
Comment 1 2011-06-09 12:15:02 PDT
It looks like the culprit is http://trac.webkit.org/changeset/82194, which introduced JavaTypeString, but didn't update JavaMethodJobject::signature() to account for this.
Steve Block
Comment 2 2011-06-10 04:01:32 PDT
Created attachment 96728 [details] Patch
Tony Gentilcore
Comment 3 2011-07-20 07:01:02 PDT
Comment on attachment 96728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96728&action=review > Source/WebCore/ChangeLog:8 > + Covered by existing tests. I don't understand. Then, should this patch be changing expectations to pass an existing failing test? > Source/WebCore/bridge/jni/JavaMethodJobject.cpp:135 > + requiresClassName |= (type == JavaTypeString); Is this code perf sensitive? Unless the compiler is smarter than I think it is, it seems like we lost a shortcut for JavaTypeObject.
Steve Block
Comment 4 2011-07-20 07:40:52 PDT
Created attachment 101469 [details] Patch
Steve Block
Comment 5 2011-07-20 07:44:13 PDT
Committed r91362: <http://trac.webkit.org/changeset/91362>
Steve Block
Comment 7 2011-07-20 08:49:47 PDT
I think that actually the break was due to http://trac.webkit.org/changeset/91355
Ryosuke Niwa
Comment 8 2011-07-20 08:58:46 PDT
(In reply to comment #7) > I think that actually the break was due to http://trac.webkit.org/changeset/91355 You're right. Commented on the right bug :(
Note You need to log in before you can comment on or make changes to this bug.