Bug 55763

Summary: JSC and V8 versions of Java bridge should share JobjectWrapper
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Steve Block <steveblock>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, dglazkov, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 55384    
Bug Blocks:    
Attachments:
Description Flags
Patch
steveblock: commit-queue-
Patch 2
none
Patch
none
Patch 4 jorlow: review+

Steve Block
Reported 2011-03-04 03:43:05 PST
JSC and V8 versions of Java bridge should share JobjectWrapper
Attachments
Patch (13.15 KB, patch)
2011-03-04 03:50 PST, Steve Block 		steveblock: commit-queue-
DetailsFormatted DiffDiff
Patch 2 (21.84 KB, patch)
2011-03-04 03:59 PST, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (14.28 KB, patch)
2011-03-04 08:25 PST, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch 4 (22.83 KB, patch)
2011-03-04 08:27 PST, Steve Block 		jorlow: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Steve Block
Comment 1 2011-03-04 03:50:46 PST
Created attachment 84722 [details] Patch
WebKit Review Bot
Comment 2 2011-03-04 03:54:30 PST
Attachment 84722 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8084626
Steve Block
Comment 3 2011-03-04 03:56:22 PST
Comment on attachment 84722 [details] Patch webkit-patch seems to have screwed up my patch, fixing ...
Steve Block
Comment 4 2011-03-04 03:59:13 PST
Created attachment 84723 [details] Patch 2
Andrei Popescu
Comment 5 2011-03-04 07:48:51 PST
I think you forgot the Android makefiles :) >53: m_env->DeleteGlobalRef(m_instance); I know this is what the old code looked like, but should you not check if m_instance is non-null before calling DeleteGlobalRef()?
Steve Block
Comment 6 2011-03-04 08:23:54 PST
Comment on attachment 84723 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > + m_env->DeleteGlobalRef(m_instance); I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null.
Steve Block
Comment 7 2011-03-04 08:25:17 PST
Created attachment 84753 [details] Patch
Andrei Popescu
Comment 8 2011-03-04 08:26:46 PST
(In reply to comment #6) > (From update of attachment 84723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > > > Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > > + m_env->DeleteGlobalRef(m_instance); > > I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes.
Steve Block
Comment 9 2011-03-04 08:27:16 PST
Created attachment 84754 [details] Patch 4
WebKit Review Bot
Comment 10 2011-03-04 08:28:48 PST
Attachment 84753 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8083730
Steve Block
Comment 11 2011-03-04 08:43:20 PST
Comment on attachment 84723 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review >>> Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 >>> + m_env->DeleteGlobalRef(m_instance); >> >> I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. > > But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes. Sure, but in that case we'll have crashed long ago because we don't check that at every call site that m_instance is non-null, so adding a check here doesn't add much.
Andrei Popescu
Comment 12 2011-03-04 08:46:06 PST
(In reply to comment #11) > (From update of attachment 84723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > > >>> Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > >>> + m_env->DeleteGlobalRef(m_instance); > >> > >> I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. > > > > But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes. > > Sure, but in that case we'll have crashed long ago because we don't check that at every call site that m_instance is non-null, so adding a check here doesn't add much. Ah yeah, you're right...well ok. LGTM.
Jeremy Orlow
Comment 13 2011-03-04 10:31:11 PST
Comment on attachment 84754 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84754&action=review > Source/WebCore/bridge/jni/JobjectWrapper.h:34 > +namespace JSC { Why in the JSC namespace if this is for V8 as well? > Source/WebCore/bridge/jni/JobjectWrapper.h:47 > + void ref() { m_refCount++; } There's no way to just use RefCounted? > Source/WebCore/bridge/jni/JobjectWrapper.h:50 > + if (!(--m_refCount)) Are the parenthesis really necessary?
Steve Block
Comment 14 2011-03-04 10:35:06 PST
Comment on attachment 84754 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84754&action=review >> Source/WebCore/bridge/jni/JobjectWrapper.h:34 >> +namespace JSC { > > Why in the JSC namespace if this is for V8 as well? All of the bridge code is in JSC::Bindings, I guess for legacy reasons >> Source/WebCore/bridge/jni/JobjectWrapper.h:47 >> + void ref() { m_refCount++; } > > There's no way to just use RefCounted? Hmm, I can look into this, but would prefer to keep this patch as just code movement. >> Source/WebCore/bridge/jni/JobjectWrapper.h:50 >> + if (!(--m_refCount)) > > Are the parenthesis really necessary? Will fix.
Jeremy Orlow
Comment 15 2011-03-04 10:40:48 PST
Comment on attachment 84754 [details] Patch 4 r=me please file a bug to keep a log of things that should be looked at after refactoring. I think the namespace issue should be one of them.
Steve Block
Comment 16 2011-03-04 10:46:59 PST
Filed Bug 55785 to look at the namespace issue and Bug 55786 to use RefCounted for JobjectWrapper. Will fix the parentheses when landing.
Steve Block
Comment 17 2011-03-04 11:18:06 PST
Committed r80367: <http://trac.webkit.org/changeset/80367>
Note You need to log in before you can comment on or make changes to this bug.