337675 - improper automatic nsISupportsWeakReference handling for java-based xpcom objects

Status: RESOLVED FIXED

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

Description

[Alexander Kireyev]

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: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

Passing a java-based implementation of an xpcom into a method that expects nsISupportsWeakReference results in creating an instantly invalid weak reference if the passed java object has no other references to its xpcom stub. The problematic code is in the SetupParams native function. In the case of native xpcom object the weak reference conversion is handled properly as follows:

            nsCOMPtr<nsISupportsWeakReference> supportsweak =
                                                   do_QueryInterface(xpcom_obj);
            if (supportsweak) {
              nsWeakPtr weakref;
              supportsweak->GetWeakReference(getter_AddRefs(weakref));
              NS_RELEASE(xpcom_obj);
              xpcom_obj = weakref;
              NS_ADDREF(xpcom_obj);
            } else {
              xpcom_obj = nsnull;
            } 

in the case of java-based implementation the code is a bit strange:

            nsJavaXPTCStub* stub = NS_STATIC_CAST(nsJavaXPTCStub*,
                                                 NS_STATIC_CAST(nsXPTCStubBase*,
                                                                xpcom_obj));
            nsJavaXPTCStubWeakRef* weakref;
            weakref = new nsJavaXPTCStubWeakRef(java_obj, stub);
            if (!weakref) {
              rv = NS_ERROR_OUT_OF_MEMORY;
              break;
            }
            NS_RELEASE(xpcom_obj);
            xpcom_obj = weakref;
            NS_ADDREF(xpcom_obj); 

Here a weak reference stub is created, but the underlying stub is not getting its weak reference count updated. Therefore the following NS_RELEASE(xpcom_obj) destroys that object without regard to the weak reference. The net result here is that weak reference refers to a deleted stub, but does not know this. Further attemps to dereference or even free this weak reference can result in a system crash.

I have managed to work around this problem by changing the code for java-based xpcom case to be similar to the native case. This may cause memory leaks or some other problems, but at least it fixes this bug.

Reproducible: Always

Comment 1

[jhp (no longer active)]

Attachment: patch

Good catch.  I think at one time I was doing something special for JavaXPTCStubs, but that's no longer true.  So it can all go down the common path.

Let me know if this patch works for you.

Comment 2

[jhp (no longer active)]

Attachment: more complete patch

Comment 3

[jhp (no longer active)]

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

Comment 4

[jhp (no longer active)]

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

bsmedberg, can you take a look at this patch?  It makes it so we always acquire a new nsJavaXPTCStubWeakRef by calling nsJavaXPTCStub::GetWeakReference(), which properly updates the weak ref count.  The rest just cleans up the calls to GetNewOrUsedXPCOMObject(), since the last parameter is no longer necessary.

Comment 5

[Alexander Kireyev]

(In reply to comment #2)
> Created an attachment (id=222242) [edit]
> more complete patch
> 

Tried the patch. My test applications work now. 

Comment 6

[Alexander Kireyev]

Any progress on integrating these patches into the source tree?

Comment 7

[Benjamin Smedberg]

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

We really need to find some new peers for this code: I only vaguely understand what's going on here ;-)

Comment 8

[jhp (no longer active)]

Checked in to trunk and 1.8 branch.  ->FIXED