82517 - Performing QI on async xpcom event proxy causes crash

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

If the following operations are performed, xpcom dies with a NULL pointer 
reference:

* Create an _async_ proxy around some object.
* Call QI on that object for some new interface (ie, an interface other than 
the one used to create the proxy.

The problem is that nsProxyEventClass::CallQueryInterfaceOnProxy() attempts to 
delegate the QI to the underlying object.  However, if the proxy is async, the 
QI call is simply posted to the event queue, and returns immediately.

In this case, the QI reports success, but the result remains NULL. 
nsProxyEventClass::CallQueryInterfaceOnProxy() simply de-references the 
returned pointer.

Attaching a JavaScript test case that demonstrates.  Simply run the js code 
under xpcshell to see the crash.  We create the proxy with nsISupports, but 
attempting to QI for the "real" interface then dies.

I am not attaching a patch as I am unsure of the correct semantics - either the 
QI should fail, or it should be immediately delegated ignoring the async flag.  
Neither seems particularly appropriate.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Test code that can be run under xpcshell to get the crash.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Taking the liberty of adding jband to this bug, being the xpcom czar that he 
is.  Note this is _not_ a blocker for Komodo, but worth knowing about anyway.

Comment 3

[John Bandhauer]

Adding dougt to cc list. He's the author of this fine proxy code. Maybe he'd 
like to take the bug? Upholding QI success/fail semantics is *very* important.

Comment 4

[Doug Turner (:dougt)]

sure

Comment 5

[Doug Turner (:dougt)]

Attachment: proposed patch v1

Comment 6

[Doug Turner (:dougt)]

Attachment: proposed patch v1

Comment 7

[Doug Turner (:dougt)]

With the last patch, all nsISupports calls will be synchronous.  

Comment 8

[Doug Turner (:dougt)]

MarkH, jband, can you review my patch (40889).  

Comment 9

[Mark Hammond [:markh] [:mhammond]]

I'm a little worried that this may lead to dead-locks.

Consider:
* main thread calls SomeFunction(my_proxy)
  my_proxy is an async proxy back to some other object on the main thread.

* SomeFunction() starts a new thread, then waits for thread to signal a 
notification.

* The new thread attempts to make some use of the passed proxy object - eg, 
perform a QI.

At this stage we will be dead-locked.  The QI will attempt to perform a 
synchronous call to the proxy object via the even queue.  However, the event 
queue is waiting for the initial call to SomeFunction() to return.

In effect, if there is ever a situation where using an ASYNC proxy is 
_necessary_ to avoid dead-locks, any attempt to QI this proxy will still 
deadlock.

Am I missing something?

Comment 10

[Doug Turner (:dougt)]

Exactly.  Ignore that last patch.

AddRef, Release*, and QI should be done on ANY thread.  The base class should 
implement these threadsafe.  Release is the special case where the delete will 
need to be proxied IFF it is the last reference.  

Comment 11

[Doug Turner (:dougt)]

Attachment: Proposed Patch v.2

Comment 12

[Mark Hammond [:markh] [:mhammond]]

This looks OK to me.  My only question is regarding PROXY_ALWAYS.

+    if ( (methodIndex < 2) ||
+         (mProxyType & PROXY_SYNC && 
+          NS_SUCCEEDED(mDestQueue->IsQueueOnCurrentThread(&callDirectly)) &&
+          callDirectly))

It _appears_ to me that this condition should handle this value - ie, if we 
have a proxy with PROXY_SYNC | PROXY_ALWAYS, then I believe we never want to 
call direct (except for the methodIndex < 2 case, where we always do).

However, as it seems the original code did not make this distinction I am far 
from sure about this!

Comment 13

[Doug Turner (:dougt)]

I think that you are right; It was busted.  This might cause some deadlock 
regressions as the code might not be expecting the calling thread to block 
waiting on another thread.  

The line should be:

(mProxyType & PROXY_SYNC && !(mProxyType & PROXY_ALWAYS)

Comment 14

[rpotts (gone)]

I'm a bit concerned about making *all* calls to nsISupports multithreaded.

The original purpose of the proxy code was to protect code that was inheritly
not-threadsafe.  But with this change, that assumption is invalidated!

I think that we need to decide what we need to use ASYNC proxies for...  I would
argue that the scenario that Mark described - leading to deadlock - is simply an
illustration of when *not* to use an ASYNC proxy :-)

I think that we should keep the requirement that *all* methods to the proxy MUST
be called on the correct (ie. originating) thread and detail the situations
where this will lead to deadlocks...

Otherwise, we will end up sweeping through the codebase to make more and more
objects implement THREADSAFE_ISUPPORTS....

I guess, that really, i'm just arguing for making the "smaller" change :-)

Comment 15

[Doug Turner (:dougt)]

rick, currently any client of this proxy system must have threadsafe isupports 
impelmentation.  

Comment 16

[rpotts (gone)]

Is this really true?  Because it certainly was NOT the intent!

There is no reason why the underlying 'real' object should have a threadsafe
ISupports implementation.  That was part of the reason for having proxy objects
in the first place...

I agree that the 'proxy object' must be treadsafe, but that's what its designed
for :-)

I think that QI is just a degenerate situation where the proxy object MUST
perform a synchronous proxy call to the original thread to perform the QI on the
'real object'...  Then, if the iface which is returned must be proxied too...

Comment 17

[Doug Turner (:dougt)]

someone has got to own a lock when we do addref's and release's on the REAL
object.  I probably made a mistake and should have had the proxy object own this
addref/release lock.  The only problem with that is that if the real object was
threadsafe, you would have to entry two locks. :-/

If the underlying real object was threadsafe, QI can just be a synchronous
freethreaded call.

Comment 18

[rpotts (gone)]

I guess, i'm still not seeing why the AddRef/Release on the real object need to
be locked (ie. threadsafe)...

I haven't looked at the code, but i'm assuming that there is a *single* addref
(on the caller's thread) of the "real object" when the proxy object is created.
 This "owning reference" is all that's needed to keep the real object alive...

after that all AddRef/Release calls apply *only* to the proxy object.  Finally
when the proxy object is deleted, it proxies a call back to the "caller's
thread" for the Release() of the real object...

In this situation, all AddRef/Release calls are made on the correct thread so no
locking is required...

I believe that the issue with requiring QI to be synchronous is just an explicit
example of an unstated assumption that "only interfaces which have no [out]
parameters can have async proxies"...

I think that in the case of ISupports we *must* implement it synchronously for
the very reason that QI (and even AddRef/Release) have [out] parameters...

Comment 19

[Doug Turner (:dougt)]

bulk move to 0.9.4

Comment 20

[Doug Turner (:dougt)]

Attachment: Proposed Patch v.3

Comment 21

[rpotts (gone)]

doug,

i think that i'm finally beginning to see why the underlying object needs to
implement a thread-safe version of nsISupports :-)  The main reason being that
since QI automatically does an AddRef(...) we can't call it on another thread
*unless* the ISupports implementation is threadsafe...

So, if we state (in VERY BIG PRINT) that all real objects *must* have a
threadsafe ISupports implementation then I think your patch is fine.

I've only got one question... what is the difference between PROXY_ASYNC and
PROXY_ALWAYS?  It looks like both are treated the same in your patch. 
Specifically, it looks like a proxy event will be created for PROXY_ASYNC *even*
if the call is made on the same thread...

-- rick

Comment 22

[Doug Turner (:dougt)]

PROXY_ASYNC will not wait for any response.  It is a kind of "fire and forget"
function call.

There is a check during object creation which determines if the destination
event queue is on the caller's thread.  If it is, the proxy code returns the
same object (no proxy is created).  This was some funky requirement back in the
days of t ravis.  PROXY_ALWAYS will bypass this check.

Comment 23

[rpotts (gone)]

r=rpotts

Comment 24

[Doug Turner (:dougt)]

John, how about a sr=?

Comment 25

[John Bandhauer]

sr=jband

Comment 26

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.4.

Comment 27

[Doug Turner (:dougt)]

I am seeing problems with this current patch.  I need more time to look at it. 
--> 0.9.5

Comment 28

[Doug Turner (:dougt)]

i am a loser.  I applied this patch by hand to a new tree and was seeing
problems.  The problem was that I messed up apply the patch and forgot a return
rv.  This caused a nice deadlock!!!  

I am going to check this fix in to 0.9.4.

Comment 29

[Doug Turner (:dougt)]

Checking in src/nsProxyEvent.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEvent.cpp,v  <--  nsProxyEvent.cpp
new revision: 1.64; previous revision: 1.63
done

Marking fixed.

Comment 30

[Doug Turner (:dougt)]

crap.  linux does not exit during the page cycler anymore.   Had to backout the
last patch. 

Comment 31

[Doug Turner (:dougt)]

i fumbled this one pretty bad.  Too many different patches on different
machines.  Patch v.3 above does not apply and is missing logic to handle zero
parameters.  If I rediff my windows box and apply it to my linux box, I can
successfully complete pagecycler.

New patch coming up.

Comment 32

[Doug Turner (:dougt)]

Attachment: Proposed Patch v.4 (and hopefully the last!!)

Comment 33

[rpotts (gone)]

hey doug,

is it too early in the morning, or does that new 'if (NS_FAILED(rv)) return rv;'
code at the bottom of the patch cause 'proxyInfo' to leak if 'PostAndWait()' fails?

I think it might be better to say something like:

    rv = PostAndWait(proxyInfo);
    if (NS_SUCCEEDED(rv)) {
        rv = proxyInfo->GetResult();
    }
    delete proxyInfo;
    return rv;


-- rick

Comment 34

[Doug Turner (:dougt)]

yup. and the leak was in the older patches too.  I will fix it now.

Comment 35

[Doug Turner (:dougt)]

Attachment: "I won't say that this is the last patch" patch

Comment 36

[rpotts (gone)]

Comment on attachment 48176 [details] [diff] [review]
"I won't say that this is the last patch" patch

At some point we should probably fix up 'convertMiniVariantToVariant' so it uses nsMemory instead of malloc/free...

Just for consistancy ;-)

Comment 37

[Chris Waterson]

rs=waterson, somebody else better understand these changes better than I do.

Comment 38

[Doug Turner (:dougt)]

fix checked in.