Last Comment Bug 712460 - GC: missing barriers in jsexn
: GC: missing barriers in jsexn
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks: 673454
  Show dependency treegraph
 
Reported: 2011-12-20 14:59 PST by Terrence Cole [:terrence]
Modified: 2011-12-29 03:35 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.66 KB, patch)
2011-12-20 14:59 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Review
v2: With review feedback. (4.74 KB, patch)
2011-12-22 12:28 PST, Terrence Cole [:terrence]
terrence: review+
Details | Diff | Review

Description Terrence Cole [:terrence] 2011-12-20 14:59:53 PST
Created attachment 583306 [details] [diff] [review]
v1

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.
Comment 1 Bill McCloskey (:billm) 2011-12-20 15:48:18 PST
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.
Comment 2 Terrence Cole [:terrence] 2011-12-20 17:01:53 PST
(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?
Comment 3 Terrence Cole [:terrence] 2011-12-22 10:49:38 PST
(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.
Comment 4 Terrence Cole [:terrence] 2011-12-22 12:28:16 PST
Created attachment 583890 [details] [diff] [review]
v2: With review feedback.

For posterity, just in-case mozilla-inbound never opens again.
Comment 5 Terrence Cole [:terrence] 2011-12-28 15:05:24 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05d47054f2b
Comment 6 Marco Bonardo [::mak] 2011-12-29 03:35:10 PST
https://hg.mozilla.org/mozilla-central/rev/c05d47054f2b

Note You need to log in before you can comment on or make changes to this bug.