326991 - JS_SetDebugErrorHook and ReportError aren't properly threadsafe

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[timeless]

consider the unhappy event that there are two competing debuggers floating around and one of them is currently installed.

An error starts processing on a thread (#2).
ReportError reaches

 714         JSDebugErrorHook hook = cx->runtime->debugErrorHook;
 715         /* test local in case debugErrorHook changed on another thread */
 716         if (hook)

hook (for debugger "A") is now cached.

On thread #3, for kicks, the other debugger ("B") calls:

1189 JS_SetDebugErrorHook(JSRuntime *rt, JSDebugErrorHook hook, void *closure)
1190 {
1191     rt->debugErrorHook = hook;
1192     rt->debugErrorHookData = closure;
1193     return JS_TRUE;
1194 }

And reaches the end of the function. To make this bug interesting, the two debuggers do *not* have the same closure structure. If they did, life just wouldn't be interesting.

Thread #2 continues:
 717             hook(cx, message, reportp, cx->runtime->debugErrorHookData);

And passes a closure for debugger "B" to the hook for debugger "A".

Debugger A crashes and someone complains.

Now this isn't currently a likely scenario certainly, but as i'm trying to deal w/ basic hook problems, it's the first one I encountered.

Comment 1

[Brendan Eich [:brendan]]

Timeless, the debugger API was intentionally designed to be single-threaded.  There is some interest in making it thread-safe, but it's a big job.  Cc'ing other folks who might care.  We shouldn't just file bugs and patch piecemeal.  Need some coordinated thinking recorded on wiki.mozilla.org.

/be

Comment 2

[timeless]

i'm actually almost done making jsd_xpc deal with threads.

once i'm done someone can refactor it back to jsd or jsapi.

my proposal for this specific problem is to switch from independent pointers to structures. so when you swap out the callback and the closure you are simply swapping out the structure containing the pair. the makes retrieval and setting of these things atomic. the only drawback is that someone or something has to allocate the space for that struct. (ie it could fail.)

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> i'm actually almost done making jsd_xpc deal with threads.

How?  Need a reviewable design doc on the wiki.

> once i'm done someone can refactor it back to jsd or jsapi.

"someone"?  By jsapi do you mean jsdbgapi?

> my proposal for this specific problem is to switch from independent pointers to
> structures. so when you swap out the callback and the closure you are simply
> swapping out the structure containing the pair. the makes retrieval and setting
> of these things atomic. the only drawback is that someone or something has to
> allocate the space for that struct. (ie it could fail.)

Design review before patching, please.  What you propose above is broken, in several ways:

(1) pointer store and load are not atomic or coherent on all architectures.
(2) using compare-and-swap can fix (1), but is not the right tool for this job: if two threads are fighting to own a per-runtime hook, perhaps that hook really belongs in the context.

I'm not going to wade through a patch analyzing the design assumptions and intent and doing post-hoc design review.  Propose your complete design in detail.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > allocate the space for that struct. (ie it could fail.)
> 
> Design review before patching, please.  What you propose above is broken, in
> several ways:
> 
> (1) pointer store and load are not atomic or coherent on all architectures.
> (2) using compare-and-swap can fix (1), but is not the right tool for this job:
> if two threads are fighting to own a per-runtime hook, perhaps that hook really
> belongs in the context.

Not to mention (0) we should not dynamically allocate a pair of pointers that are equivalent to two members of their owning struct for no reason.  Dynamic range in population and/or lifetime is what motivates dynamic allocation.  The hook and its data are always a pair, one-for-one with their parent struct.

It really sounds as if you want per-context hooks-with-data.

/be

Comment 5

[Luke Wagner [:luke]]

Single-threaded runtime.