JS ABBA deadlock between rt->setSlotLock and a claimed scope

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla0.9.1
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

17 years ago
Thread t has rt->setSlotLock (because it's first into js_SetProtoOrParent) and
wants to lock an object's scope (say, to get that object's proto, while walking
the chain checking for cycles, jsobj.c:258).  But that scope is claimed, i.e.,
exclusively owned until end of request, by another thread, u.  Thread u is still
in its request and now wants rt->setSlotLock (because it is setting a parent or
proto slot -- it's entering js_SetProtoOrParent).  Oops.

While fixing bug 54743, my brain melted and I overlooked this now-obvious AB-BA
deadlock.  In fact, it seems pretty bad to me that rt->setSlotLock, a PRLock, is
held while the thread in js_SetProtoOrParent in the *non-deadlock* case is then
waiting to claim a scope or two.  I can't detect cycles here without nesting
locks, but I surely can do better than to abuse a mutex to make threads wait
indefinitely (if not infinitely!).

Patch coming up.

/be
(Assignee)

Comment 1

17 years ago
This is critical to my good friends who embed JS in their big, multi-threaded
VXML engine.

/be
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9.1
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1
(Assignee)

Comment 2

17 years ago
Created attachment 33312 [details] [diff] [review]
proposed fix: use a condvar to make other proto or parent setters suspend their requests and wait
(Assignee)

Comment 3

17 years ago
Ok, shaver and jband: I need your r/sr='s you are the only two who might claim
to understand JS thread safety.

Cc'ing jcondit@tellme.com, please nag Mike Epstein to get an epstein@tellme.com
bugzilla login.

/be
(Assignee)

Comment 4

17 years ago
Created attachment 33319 [details] [diff] [review]
better fix, ignore the first patch and try this one!
(Assignee)

Comment 5

17 years ago
In my haste, I copy-and-pasted rt->setSlotDone's condvar creation code from
rt->scopeSharingTodo's, copying rt->gcLock as the condvar's lock arg instead of
rt->setSlotLock.  That second patch fixes JS_NewRuntime.  Here's a third patch
to tidy up the local macro in js_SetProtoOrParent, by #undef'ing it at the end
of the function's body.

/be
(Assignee)

Comment 6

17 years ago
Created attachment 33336 [details] [diff] [review]
best fix, #undef local macro at end of js_SetProtoOrParent
r=shaver

Comment 8

17 years ago
sr=jband
(Assignee)

Comment 9

17 years ago
Fix checked in.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 10

17 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.