320080 - Enforce nsIClassInfo threadsafety markers in xpconnect

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[timeless]

Currently xpconnect is rather loose about what you can handle from threads. the unfortunate result is that bad things can happen, deadlock/crashes, when you use objects that didn't indicate they were willing to cooperate with threading. this is a violation of their poor contractual rights.

Comment 1

[timeless]

Attachment: enforce thread restriction (ifdef XPC_CHECK_WRAPPER_THREADSAFETY)

the restriction will have to apply to everyone eventually. and the value of the errors is subject to negotiation.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 205700 [details] [diff] [review]
enforce thread restriction (ifdef XPC_CHECK_WRAPPER_THREADSAFETY)

>Index: xpcwrappednativejsops.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,v
>retrieving revision 1.48
>diff -u -r1.48 xpcwrappednativejsops.cpp
>--- xpcwrappednativejsops.cpp
>+++ xpcwrappednativejsops.cpp
>@@ -41,7 +41,16 @@
> /* JavaScript JSClasses and JSOps for our Wrapped Native JS Objects. */
> 
> #include "xpcprivate.h"
>-
>+/* Winerror.h(21313):// MessageId: RPC_E_ATTEMPTED_MULTITHREAD
>+ * Winerror.h(21317)://  Attempted to make calls on more than one thread in single threaded mode.
>+ * Winerror.h(21319):#define RPC_E_ATTEMPTED_MULTITHREAD      _HRESULT_TYPEDEF_(0x80010102L)
>+ */
>+#define NS_ERROR_ATTEMPTED_MULTITHREAD ((nsresult) 0x80010102)
>+/* Winerror.h(21421):// MessageId: RPC_E_WRONG_THREAD
>+ * Winerror.h(21425)://  The application called an interface that was marshalled for a different thread.
>+ * Winerror.h(21427):#define RPC_E_WRONG_THREAD               _HRESULT_TYPEDEF_(0x8001010EL)
>+ */
>+#define NS_ERROR_WRONG_THREAD          ((nsresult) 0x8001010E)

First, let's put these in the right place.  They don't belong in an XPConnect .cpp file, they belong in a base XPCOM .h file.  Think PyXPCOM or other bridges.

Second, must we use the same HRESULT/nsresult as MS did?  If so, why?  If not, there are nsresult-building macros to use in xpcom/base, IIRC.

>+#ifdef XPC_CHECK_WRAPPER_THREADSAFETY
>+#define THROW_AND_RETURN_IF_THREADUNSAFE(cx, wrapper)                        \
>+    PR_BEGIN_MACRO                                                           \
>+        XPCWrappedNativeProto* proto = wrapper->GetProto();                  \
>+        if(proto &&                                                          \
>+           !proto->ClassIsThreadSafe() &&                                    \
>+            wrapper->mThread != PR_GetCurrentThread())                       \

Nit: wrapper->... is overindented by one space.

>+            return Throw(NS_ERROR_ATTEMPTED_MULTITHREAD, cx);                \
>+    PR_END_MACRO
>+#else
>+#define THROW_AND_RETURN_IF_THREADUNSAFE(cx, wrapper)                        \
>+    PR_BEGIN_MACRO                                                           \
>+    PR_END_MACRO
>+#endif
> // Handy macro used in many callback stub below.
> 
> #define THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper)                         \
>@@ -61,6 +84,7 @@
>         return Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx);                   \
>     if(!wrapper->IsValid())                                                  \
>         return Throw(NS_ERROR_XPC_HAS_BEEN_SHUTDOWN, cx);                    \
>+    THROW_AND_RETURN_IF_THREADUNSAFE(cx, wrapper);                           \
>     PR_END_MACRO

What effect does this added code have on Tp, Tdhtml, Txul, Ts, etc.?  It looks like it might hurt.  Instead of checking all the time, we might want to add gatekeepers to paths that could pass XPCOM objects among threads.  That minimizes the cost.  Are there few enough gates that we can guard them all?

/be

Comment 3

[timeless]

We don't have to use the Microsoft result values, I just offer that as a nice way to be consistent w/ ms. I'm not wedded to using them and will use whatever values people like (and I'm certainly glad to stick them in nsError.h) -- yes, please just pick any string and I'll use it, don't ask me to pick one, I've picked one.

Unfortunately, xpconnect and js conspire to not really understand the concept of passing objects across threads.

I create a js object and give it lots of js objects and xpcom objects.
I create an xpcom thread and give it my jsobject as an nsIRunnable.

From the perspective of xpconnect and js, nothing interesting has happened.

This is the 'simple' case. The more fun cases are when I create a callback for necko (necko then eventually has some path by which something in necko decides to call through to that callback object from some other thread) or jsd (js from xpconnect on some other thread calls the debugger service which finds its debugger callback and calls it). Note that in many of these cases, the objects are just assigned to things which already straddle thread boundaries. I think I actually might have a patch floating around to make jsd stop handling things from other threads if it won't work, But that's some other patch I'll have to dig up so it can rot publicly in Bugzilla.

Back to our simple case. I now ask the xpcom thread to run. It spins up an nspr thread and passes it the xpcom thread entrypoint and a data pointer.

nspr spins up an os thread. On the os thread, nspr's thread entry is called, which then chains to the xpcom thread entry which finds its nsIRunnable and invokes run.

Some QIs may happen in this process, but ideally they already happened when the xpcom thread was given the nsirunnable on the initiating thread.

run becomes xptcall which is translated to xpconnect which doesn't see how this xptcall is different from any other xptcall. So it talks to js about getting a run property as a function and calling it. js now runs around and starts poking properties through either members of the object, or for kicks its global object. Eventually js pokes an xpcom native object which necessitates an xpconnect transition. At this point, xpconnect still has no idea why this transition is any different from any other transition. And if the object happens to be the dom global window, we shortly die.

If you can point to useful places where things can go and recognize that objects are transitioning, then I can see about doing something (although probably not for 6+ months unless I start working for Mozilla Corporation).

As for performance. Someone else is welcome to run with these changes and get numbers. I don't have any resources in my present position to do that. All I can say is that Chatzilla, Venkman, and a couple of modules internally are all dying very unhappy deaths because xpconnect is not enforcing information it has been told about and is instead blatantly violating the contract and passing things to the DOM global window which cheerfully crash gecko.

Basically, there's no such thing as a path for moving an object across threads. objects are just storage (unless they're TLS) and suddenly the objects are being accessed from an unexpected thread.

Now we could try to keep a per thread table of all objects that have been accessed on a given thread, and only do this work if an object is not listed in that table. such a hashtable /might/ be more efficient than the code I have above. I suppose if xpconnect stored that hashtable via TLS it might be moderately inexpensive and it wouldn't require PR_GetCurrentThread() calls since it would always just look in its current hashtable. We'd of course need provisions for clearing out that table, and doing that work would very much balloon this patch far beyond the scope I want to deal with for this bug.

I already have the beginnings of code (or at least an outline) somewhere to start using per thread hashtables to fix the other side of this coin. But it's really the subject of a different bug. That other side of the coin deals with destroying objects at the end of a GC() from the wrong thread. At present if the last reference to an xpcom object is from js and it was created on some other thread (and even only used on that other thread), then it will be destroyed on the main thread because xpconnect doesn't do work to properly arrange for its death on some other thread.

But back to your original question. There's *no* perf cost in the standard case with this patch because in the standard case the ifdef isn't applied. This just lets debug builds not get screwed. It also gets it out of my tree, so people could start using it or measuring the perf, or optimizing it or whatever.

The sample case can of course be expanded. I could instead create an xpcom object, give it a js object, and the xpcom object could go off and get itself assigned to some other xpcom object which decides to land in an xpcom thread and eventually they could call back to the js object. As with your lament about a lack of reachability for xpcom objects to enable you to avoid reference counting loops, the same problem happens here to me to be able to recognize that some object has managed to transition badly. Note that technically each object that is threadsafe should enforce threadsafety (and they don't), so it's possible for a pure c++ xpcom mess to screw up, it's just a bit easier to screw up when you're js and starting w/ a globalwindow in your scope chain, because of the way the fireworks trigger and how spectacular they are.

Comment 4

[Brendan Eich [:brendan]]

Timeless: you still looking for review? I'm trying to trim my queue of old stuff. It would be great to have a short status report on this bug -- maybe you don't care any longer, but others might?

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 205700 [details] [diff] [review]
enforce thread restriction (ifdef XPC_CHECK_WRAPPER_THREADSAFETY)

We do gate-keep between threads, in the JS engine at least (see jslock.c). As with security checks, we should factor costly tests and compensation code into wrappers or gatekeepers that do not penalize the same-(origin/thread) case.

/be

Comment 6

[timeless]

this patch only applies to DEBUG builds, so it wouldn't affect any time tests.

the goal of this patch was to enable people writing extensions to be able to find out when their code was technically wrong and fix it, without hurting release builds. release builds would be just as crashy as ever, except if people did development with debug builds, they'd notice their code was broken (it wouldn't work at all) and they'd fix it before delivering it to end users, and the delivered code wouldn't crash.

i think that from js's perspective, xpconnect probably cheats by using per thread contexts and swapping objects between them. in order to behave "correctly", it'd probably need to support per object contexts.

I suppose at wrap time, it could assign the object the current thread context.  It could probably respond to a magic error from js anytime an object fails to be on the right context (assuming js doesn't simply abort()) and then check classinfo, if the object succeeds, it could create a new context for that object, and try again, and if it fails, it could propagate a threadsafety exception.

This would handle the case of an xpconnect component, which is itself uninteresting, because such components are written in js and the engine is technically threadsafe.

The next half of such a process would be for xp*wrapped*native (there used to only be one such object, but I think there are now a couple XOW?) to somehow keep track of each object it gets, it could either QI for classinfo up front, or only do it on the first access, but unfortunately in order to do it, it needs to keep track of each object's threadorigin except for the threadsafe case.

I think actually that solving this was planned by me in some other bug or work, I'm also fairly certain that i alluded to it in some other comment here or in some other bug. I certainly don't want this bug to focus on that. This bug is just a way of enabling developers to be forced to write code that's correct.

brendan:
i'm still looking for a review. but the review is mostly: please pick a proper name/place for the errors.

Comment 7

[Christian Haider]

Attachment: The same as the patch above just updated

Comment 8

[Christian Haider]

Attachment: same as above works for debug build as well

Comment 9

[timeless]

Comment on attachment 308647 [details] [diff] [review]
same as above works for debug build as well

please just suggest error values/names or accept these.

Comment 10

[Andrew McCreight [:mccr8]]

XPConnect is single threaded now.