Bug 55763 – JSC and V8 versions of Java bridge should share JobjectWrapper

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

JSC and V8 versions of Java bridge should share JobjectWrapper

[Steve Block]

Created attachment 84722 [details]
Patch

[WebKit Review Bot]

Attachment 84722 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8084626

[Steve Block]

Comment on attachment 84722 [details]
Patch

webkit-patch seems to have screwed up my patch, fixing ...

[Steve Block]

Created attachment 84723 [details]
Patch 2

[Andrei Popescu]

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 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]

Created attachment 84753 [details]
Patch

[Andrei Popescu]

(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]

Created attachment 84754 [details]
Patch 4

[WebKit Review Bot]

Attachment 84753 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8083730

[Steve Block]

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]

(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 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 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 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]

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]

Committed r80367: <http://trac.webkit.org/changeset/80367>