338110 - Remove operation on JavaXPTCStub map is called with bad parameters

Status: RESOLVED FIXED

(Core Graveyard :: Java to XPCOM Bridge, defect)

Description

[Tudor TEUSAN]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: 

The JavaToXPTCStubMap::Remove (nsJavaXPCOMBindingsUtils.cpp) operation is called from the nsJavaXPTCStub::Destroy() method of any contained stub. At the time of the call, we suppose that the mJavaStrongReference member holds a valid reference to the java object, which is wrong (see the scenario hereunder).

This bug is (remotely) related to bugs 337675 and 338014. I've marked it as critical for the same reasons as those outlined in bug 338014 - calling a jni method with a wrong (global) reference is unspecified and can actually cause a vm crash.

Reproducible: Always

Steps to Reproduce:
1. Create a nsJavaXPTCStub and put it in the map
2. Obtain a weak reference to the stub. stub.mWeakRefCnt is 1.
3. Obtain a reference to the stub. stub.mRefCont is 1 and mJavaStrongRef holds a valid global reference.

4. Release the reference stub.mRefCnt is 0 and the global ref is deleted (see  nsJavaXPTCStub::ReleaseInternal()). As mWeakRefCnt is still 1, the stub's Destroy() is not called.
5. The weak reference is released. Both mRefCnt and mWeakRefCnt are 0, the stub's Destroy() method gets called. (see nsJavaXPTCStub::ReleaseWeakRef()). The 
mJavaStrongRef is no longer valid. It is, however, passed to the Remove method of the global stub map (see nsJavaXPTCStub.Destroy)



Proposed solution: 

Keep the hashCode of the java object (which is also the hash key) with the XPTC stub. The hash code can be intialized in the constructor. Call the Remove method with the hashCode as unique parameter (signature change required).

Note that a straightforward but, imho, wrong solution would be to use the mJavaWeakReference instead of the mJavaStrongReference for the gJavaToXPTCStubMap->Remove(env, mJavaStrongRef) call in the nsJavXPTCStub::Destroy(). Once there are no more jni global references to a java object, the object can be garbage collected at any time. There is no guarantee about the validity of the mJavaWeakReference if mJavaStrongReference has been deleted.

Comment 1

[Tudor TEUSAN]

Attachment: Store and use hashCode in the XPTC stub

Patch for the above mentioned solution.

Comment 2

[jhp (no longer active)]

Attachment: patch

Actually, the solution is much simpler.  We just need to delete the strong ref in ReleaseInternal(), rather than waiting to do it in Destroy().

Try this patch.  Note that I also needed the patch from bug 337675 in order to avoid crashes.

Comment 3

[Tudor TEUSAN]

Works for me (with patch from bug 337675).
I think that having to places where we delete the global reference (ReleaseInternal and DeleteStrongReference) should be avoided. 

In ReleaseInternal(), I would call DeleteStrongReference() instead of simply deleting it through the jni env. It's just a (not so important) matter of style. 

Bug 338111 gets impossible to hit with these two patches applied. Nevertheless systematically checking mJavaStrongReference before deleting it and nulling it out afterwards is still a good thing to do.

Comment 4

[Tudor TEUSAN]

Attachment: call DeleteStrongRef()

Comment 5

[jhp (no longer active)]

Comment on attachment 222322 [details] [diff] [review]
call DeleteStrongRef()

bsmedberg, can you review this simple patch?  It fixes a crash by making sure to remove Java ref from hash table before it gets released.

Comment 6

[Tudor TEUSAN]

After having played with the above patch for a while it seems that it introduces another bug.

Scenario : 

Context : We have a listener "aJavaListener" (let's say aJavaListener is a  nsIWebProgressListener) implemented in Java. This listener has to be attached and detached from a webBrowser.

Step 1. We create the STUB for aJavaListener and we addref it. (This happens when we invoke the method webBrowser.addBrowserListener()). mRefCount is 1 and mWeakRefCount is 0. STUB is put in the global map.

Step 2. A weak reference (WEAK_REF_STUB) stub is created for STUB. Most of the listeners in mozilla (if not all) are kept through weak references. mWeakReferenceCnt is now 1.

Step 3. STUB reference is released. mRefCnt is 0, mJavaStrongRef is deleted and STUB IS REMOVED FROM THE GLOBAL MAP. At this moment WEAK_REF_STUB is still kept somewhere in webBrowser.

... We invoke webBrowser.removeWebBrowserListener(aJavaObject) 

Step 4. aJavaObject has no corresponding stub in the global map (we've removed it in STEP 3). So a NEW_STUB is created and addRefed.

Step 5. A weak reference NEW_WEAK_REF_STUB gets created by the implementation of  RemoveWebBrowserListener. This refernce is not the same as the stored WEAK_REF_STUB, so nothing is actually removed. The old listener is still there and will still be notified (unless it has been garbage collected) ...

Morality : We need to keep a stub in the global stub map unless both mRefCnt and mWeakRefCnt are 0. The stub should be removed from the global map only upon stub destruction.

Proposed solution: Put the global map removal code at its old place (in nsJavaXPTCStub::Destroy()). As the weak jni reference we have in the Destroy() method can be invalid, use the nsJavaXPTCStub::mHashCode solution outlined in the first comment. With this solution bug 338111 becomes important as we can actually call nsJavaXPTCStub::DeleteStrongRef() (through global stub map destruction) with a null/invalid mJavaStrongRef.

 

Comment 7

[Tudor TEUSAN]

Attachment: patch

Patch for the

Comment 8

[Tudor TEUSAN]

... proposed solution. This patch incorporates the patch for bug 338111.

Comment 9

[jhp (no longer active)]

(In reply to comment #6)
> ... We invoke webBrowser.removeWebBrowserListener(aJavaObject) 
> 
> Step 4. aJavaObject has no corresponding stub in the global map (we've removed
> it in STEP 3). So a NEW_STUB is created and addRefed.

Yes, you are absolutely right.  This was the reason why I wrote the code as it is.  But I forgot this scenario when creating my patch.  I'll take a look at your patch today.

Comment 10

[jhp (no longer active)]

Attachment: more complete patch

I like your patch, but I wanted to be consistent in the JavaToXPTCStubMap class.  So I changed all of its methods to take a hash code.  How's this work for you?

Comment 11

[Tudor TEUSAN]

It works. 

There's just a small problem when applied together with patch for bug 337675 (hunk 4 fails on nsJavaXPCOMBindingUtils.cpp).

A fuzz factor of at least 3 ("patch --fuzz=3 <patch") solves the problem. 

Comment 12

[Benjamin Smedberg]

Comment on attachment 222549 [details] [diff] [review]
more complete patch

Is this safe?... I mean, how do you ensure that the "hashcodes" are unique (especially on 64-bit systems, where you're guaranteed to lose precision).

Comment 13

[jhp (no longer active)]

Thanks for bringing that up, Benjamin.  There is a problem with this code.

The Object.hashCode() API docs have this to say:
"As much as is reasonably practical, the hashCode method defined by class Object does return distinct integers for distinct objects. (This is typically implemented by converting the internal address of the object into an integer, but this implementation technique is not required by the JavaTM programming language.)"

So in practice, this works.  But in theory, it is not guaranteed.

The other issue that this code doesn't take into account is that a Java class could override the hasCode() method, which makes this even worse.

Comment 14

[Benjamin Smedberg]

Comment on attachment 222549 [details] [diff] [review]
more complete patch

And many important objects do. You would at least need to call the Object::HashCode method (not sure of the Java syntax).

Comment 15

[jhp (no longer active)]

Attachment: use identityHashCode()

This patch still uses hash codes for the JavaToXPTCStubMap.  In addition, I changed to using the System.identityHashCode() static method, which always returns the hash code using the base Object.hashCode() method, even if the Java object overrode this method.

Comment 16

[jhp (no longer active)]

Checked in to trunk and 1.8 branch. ->FIXED

Comment 17

[jhp (no longer active)]

*** Bug 348020 has been marked as a duplicate of this bug. ***