Closed Bug 712460 Opened 12 years ago Closed 12 years ago

GC: missing barriers in jsexn

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — 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+
(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?
(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.
For posterity, just in-case mozilla-inbound never opens again.
Attachment #583306 - Attachment is obsolete: true
Attachment #583890 - Flags: review+
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.