47134 - nsProxy needs to proxy Release()

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Alec Flett]

as per a short discussion on n.p.m.xpcom, the XPCOM proxy system needs to
automatically proxy release, so that the destructor for a proxied object does
not happen on the wrong thread.

Per doug, this needs to happen much like the way QI works:

http://lxr.mozilla.org/seamonkey/source/xpcom/proxy/src/nsProxyEventObject.cpp#344

I looked into doing this myself, and it's kinda complicated.

Comment 1

[Alec Flett]

nominate for nsbeta3, add arch keyword

Comment 2

[Judson Valeski]

adding mlk keyword (remove it if I'm wrong) and plussing.

Comment 3

[Alec Flett]

not quite an mlk, but it's pretty bad (calling the destructor on the wrong
thread)

Comment 4

[Alec Flett]

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

Comment 5

[Alec Flett]

this duped bug is the bug that first brought my attention to this issue..

Comment 6

[Doug Turner (:dougt)]

AddRef'ing and Release'ing always can happen on different threads. I don't think
that this is a problem.  You should be using THREADSAFE nsISupports when
proxying.  The refcounting is a limition of the ownership model we have.  I need
to make sure that the object does not go away when we put it in an eventQ.  I am
not going change this.  However, the deletion of the object must happen on the
destination q - always, always, always.


I have added a special Release to the nsProxyObject which will post an event to
the destination eventQ when the refcount hits zero.  When handled, the event
will delete the object.

I have tested this, and the I can not reproduce the above bug.


Attaching the diff next.  I need someone to review it.  

Comment 7

[Doug Turner (:dougt)]

Attachment: Proxy - Delete - Diff

Comment 8

[Doug Turner (:dougt)]

fix is in.  

qa, verify against bug 41958

Comment 9

[Alec Flett]

thanks doug, this is awesome
(and as a bonus I'll bet this means less assertions if I ^C on windows)

Comment 10

[Doug Turner (:dougt)]

please let me know.  

But note, that your object that you are proxing should have a threadsafe
nsISupports.... maybe I should test for this in debug builds???

Comment 11

[Alec Flett]

I thought warren had a check for this already? 
I think that any proxied AddRef will assert, so it would be easy to catch this..

Comment 12

[Doug Turner (:dougt)]

yes, exactly.  lets just go with the generic assertion.  

Maybe I should fail to create a proxy in your isupport is not thread safe?

Comment 13

[Alec Flett]

I think the assertion is good enough, esp because it only affects debug builds..

It's already there too :)