Closed Bug 337675 Opened 15 years ago Closed 15 years ago

improper automatic nsISupportsWeakReference handling for java-based xpcom objects

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alexander.kireyev, Assigned: jhpedemonte)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #222238 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 338014 has been marked as a duplicate of this bug. ***
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.
Attachment #222242 - Flags: review?(benjamin)
(In reply to comment #2)
> Created an attachment (id=222242) [edit]
> more complete patch
> 

Tried the patch. My test applications work now. 
Any progress on integrating these patches into the source tree?
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 ;-)
Attachment #222242 - Flags: review?(benjamin) → review+
Checked in to trunk and 1.8 branch.  ->FIXED
Keywords: fixed1.8.1
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.