GC: missing barriers in jsexn

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
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

6 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

6 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

6 years ago
Created attachment 583890 [details] [diff] [review]
v2: With review feedback.

For posterity, just in-case mozilla-inbound never opens again.
Attachment #583306 - Attachment is obsolete: true
Attachment #583890 - Flags: review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05d47054f2b
https://hg.mozilla.org/mozilla-central/rev/c05d47054f2b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.