101252 - proxied objects require threadsafe impl of nsISupports

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

When I apply the patches in 79509, every time an autocomplete finishes
successfully, I get 

##!!! ASSERTION: nsLDAPAutoCompleteSession not thread-safe: 'owningThread ==
N\S_CurrentThread()', file
/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.\cpp, line 528 
###!!! Break: at file
/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp\, line 528 

I'll attach the stack.  

After some poking around, I think that the problem is the code below this
comment the destructor of nsProxyEventObject:

        // 
        // This proxy is for the root interface.  Each proxy in the chain 
        // has a strong reference to the root... So, when its refcount goes 
        // to zero, it safe to remove it because no proxies are in its chain. 
        // 

Specifically, this line:

nsCOMPtr<nsISupports> rootObject =
                                do_QueryInterface(mProxyObject->mRealObject);

is calling QI (which AddRefs) on whatever thread the destructor is being called
on (which is not guarenteed to be the owning thread of mRealObject).

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: stack trace from the assertion

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I just talked to dougt, and he informs me that this is not a bug, per-se; merely
an undocumented requirement: objects that are going to be proxied need to
implement nsISupports method in a threadsafe way (eg use NS_IMPL_THREADSAFE_*).  

In the longer-term, it would be nice to get rid of this requirement, since it
would save locking overhead and make it possible to use the assertions in the
non-threadsafe macros to see when a non-threadsafe component really is being
called from the wrong thread.

In the short term, let's at least document this, probably both in the
nsISupports proxies doc and the relevant IDL.

Comment 3

[rpotts (gone)]

hey dan,

take a look at the evolution of bug #82517 for an explanation of why we need any
object which is proxied to implement ISupports threadsafe...

-- rick

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 5

[rpotts (gone)]

after reading all of the comments for this bug (again), i realize that there is
no 'concise' explanation of why we cannot easily fix the 'bug' :-)  so, i'm
going to try to explain the issues (as i see them)...

the crux of the problem is that our current async proxy implementation uses the
QI implementation of the real object to 'look up' interface information.  since
the proxy object needs this information when called from an arbitrary thread, it
force the QI implementation for the real object to be threadsafe.
as far as i can see there are several ways to fix this problem:

1. Limit the semantics of Async Proxies to apply to a single interface only!  By
this i mean that calling QI on an async proxy should only return the 'proxied'
interface -- not any others that the real object may support.  therefore, if you
need multiple interfaces from an object, you must explicitly construct multiple
async proxies (rather than relying on QI to implicitly construct them for you).

This solution is the simplest, but it severly limits the ways that async proxies
can be used.  (personally i like this solution :-))

2. Force the 'proxiably interfaces' to be explicitly passed in when the async
proxy is first constructed.  

This option is basically just a slight relaxation of  the first solution.  but
it still precludes any kind of dynamic interface determination to be made by the
proxy code...

3. Have the proxy code rely on external 'interface information' about the
component (rather than the component's own QI).  I believe that this type of
interface information is *beginning* to become available.  However, the majority
of the components in our codebase do not currently support this notion.

This solution fixes the problem because the proxy code has access to a
threadsafe information repository about the component (ie. its typelib etc)
which can be accessed whenever the proxy object needs to construct a proxy for
another interface...


Personally, i think that we should try to limit the functionality of our async
proxies rather than enhance them :-)

Also as a side issue, i think that we should *not* allow an async proxy to be
constructed for an interface that contains [out] parameters.  because [out] the
values of parameters are undefined in the asynchronous case !!

-- rick

Comment 6

[John Bandhauer]

There was discussion. I like option 3. This is nsIClassInfo. It works. We just
haven't added support to many components. But adding support is simple and
cheap. We can add a flag to nsIClassInfo that talks about whether or not to QI
for all 'listed' interfaces when building the proxy (while still on the same
thread). We can *require* nsIClassInfo of objects that will be proxied. Let's
use nsIClassInfo to tell the proxy system what to do. Let's go there!

Comment 7

[Doug Turner (:dougt)]

sounds like the right thing to do.  This could also be used to indicate the
threading model of an object.

Comment 8

[Darin Fisher]

the problem with option #3 is that classes must be designed to work with
proxies.  this is a problem because a class may not know that it might be
proxied.  consider some interface:

 interface nsIFoo : nsISupports
 {
   void setEventSink(in nsIFooEventSink sink);
 };

suppose that today, nsIFoo is implemented using only the main thread.  but, it
uses async proxies of nsIFooEventSink to make the events happen asynchronously.
 tomorrow, someone modifies the nsIFoo implementation to use a background
thread.  suddenly, all of the callers of nsIFoo::setEventSink must be fixed to
either support threadsafe addref/release or implement nsIClassInfo.  this just
doesn't scale well in an embedding world.  or are we saying that all classes
(including classes written by embedders) must support nsIClassInfo from this day
forward?  i really doubt it.

i like rpotts' suggestion.  or maybe we can exploit nsIClassInfo if available??
 if nsIClassInfo is not available, can we just fallback on limiting the proxy to
the one interface IID passed to the getProxyForObject call?

Comment 9

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 10

[Benjamin Smedberg]

We're not going to touch this for 1.9 and the world is changing for moz2.