Closed
Bug 712460
Opened 12 years ago
Closed 12 years ago
GC: missing barriers in jsexn
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
4.74 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
When we create an exception object, we copy args out to a custom exception privates struct that we pass around. If any of the arg values copied to this struct are gcthings in the nursery, this external array needs to root them.
Attachment #583306 -
Flags: review?(wmccloskey)
Comment on attachment 583306 [details] [diff] [review] v1 Review of attachment 583306 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: js/src/jsexn.cpp @@ +310,5 @@ > JSSecurityCallbacks *callbacks = JS_GetSecurityCallbacks(cx); > JSCheckAccessOp checkAccess = callbacks ? callbacks->checkObjectAccess : NULL; > > Vector<JSStackTraceElem> frames(cx); > + Vector<HeapValue> values(cx); I don't think this is necessary. The vector is only stored on the stack. So the AppendArg change can also be removed. @@ +395,5 @@ > JS_ASSERT(valuesDest == GetStackTraceValueBuffer(priv)); > > PodCopy(framesDest, frames.begin(), frames.length()); > + for (size_t i = 0; i < values.length(); ++i) > + valuesDest[i].set(cx->compartment, values[i]); Please use init instead of set here, since there's no need for an incremental barrier. @@ +414,5 @@ > { > JSExnPrivate *priv; > JSStackTraceElem *elem; > size_t vcount, i; > + HeapValue *vp, v; I think v is dead now. @@ +433,5 @@ > vcount += elem->argc; > } > vp = GetStackTraceValueBuffer(priv); > for (i = 0; i != vcount; ++i, ++vp) { > + MarkValueUnbarriered(trc, *vp, "stack trace argument"); Now that *vp is a HeapValue, we can use MarkValue instead of MarkValueUnbarriered. @@ +664,5 @@ > APPEND_CHAR_TO_STACK('('); > for (i = 0; i != elem->argc; i++, values++) { > if (i > 0) > APPEND_CHAR_TO_STACK(','); > + str = ValueToShortSource(cx, values->get()); I don't think the get() call is necessary. The conversion to Value should be automatic.
Attachment #583306 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #1) > ::: js/src/jsexn.cpp > @@ +310,5 @@ > > JSSecurityCallbacks *callbacks = JS_GetSecurityCallbacks(cx); > > JSCheckAccessOp checkAccess = callbacks ? callbacks->checkObjectAccess : NULL; > > > > Vector<JSStackTraceElem> frames(cx); > > + Vector<HeapValue> values(cx); > > I don't think this is necessary. The vector is only stored on the stack. So > the AppendArg change can also be removed. I don't follow. If we are keeping these values on the stack and they reference a GCThing in the nursery, then we will want to root those GCThings if we GC the nursery, right? Is it because we know, for other reason(s), that we cannot GC during this Vector's lifetime?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #2) > (In reply to Bill McCloskey (:billm) from comment #1) > > ::: js/src/jsexn.cpp > > @@ +310,5 @@ > > > JSSecurityCallbacks *callbacks = JS_GetSecurityCallbacks(cx); > > > JSCheckAccessOp checkAccess = callbacks ? callbacks->checkObjectAccess : NULL; > > > > > > Vector<JSStackTraceElem> frames(cx); > > > + Vector<HeapValue> values(cx); > > > > I don't think this is necessary. The vector is only stored on the stack. So > > the AppendArg change can also be removed. > > I don't follow. If we are keeping these values on the stack and they > reference a GCThing in the nursery, then we will want to root those GCThings > if we GC the nursery, right? Is it because we know, for other reason(s), > that we cannot GC during this Vector's lifetime? Bill answered my question IRL: it's quite obvious in retrospect. If we add a remembered set pointer to a stack location, we will overwrite a random spot in the stack at the next GC.
Assignee | ||
Comment 4•12 years ago
|
||
For posterity, just in-case mozilla-inbound never opens again.
Attachment #583306 -
Attachment is obsolete: true
Attachment #583890 -
Flags: review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05d47054f2b
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c05d47054f2b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•