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).

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.

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

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

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

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!

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

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?

mass reassigning to nobody.

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