634783 - Add additional shape and property rooting

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We think we may have a problem with shape rooting, since they're quite common and they're not scanned by the stack scanner.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch adds a bunch of rooters. I audited all the code, and in any place that seemed like it might possibly need a rooter, I added one.

I also made an AutoPropertyRooter. Given a property and an object containing that property, it checks if the object isNative. If it is, it casts the property to a shape and roots it. I talked to Brendan, and it sounds like this should be sufficient to root properties in Firefox.

This is a short-term solution, but it might reduce crashes.

Comment 2

[Andreas Gal :gal]

Comment on attachment 512973 [details] [diff] [review]
patch

Flag in the constructor if obj is native and root the shape based on that (or not). Later the object might no longer be native (proxies!).

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 512973 [details] [diff] [review]
patch

>@@ -3468,16 +3468,17 @@ JS_AliasProperty(JSContext *cx, JSObject
>     if (!atom)
>         return JS_FALSE;
>     if (!LookupPropertyById(cx, obj, ATOM_TO_JSID(atom), JSRESOLVE_QUALIFIED, &obj2, &prop))
>         return JS_FALSE;
>     if (!prop) {
>         js_ReportIsNotDefined(cx, name);
>         return JS_FALSE;
>     }
>+    AutoPropertyRooter apr(cx, obj2, prop);

This is not needed, because js_Atomize might run a GC it will not delete shape from obj2.

> js_watch_set(JSContext *cx, JSObject *obj, jsid id, JSBool strict, Value *vp)
> {
>     JSRuntime *rt = cx->runtime;
>     DBG_LOCK(rt);
>     for (JSWatchPoint *wp = (JSWatchPoint *)rt->watchPointList.next;
>          &wp->links != &rt->watchPointList;
>          wp = (JSWatchPoint *)wp->links.next) {
>         const Shape *shape = wp->shape;
>+        AutoShapeRooter asr(cx, shape);

This is not necessary, because for all wp in the runtime, wp->shape is traced by js_TraceWatchPoints.

> UpdateWatchpointShape(JSContext *cx, JSWatchPoint *wp, const Shape *newShape)
> {
>     JS_ASSERT_IF(wp->shape, wp->shape->id == newShape->id);
>     JS_ASSERT(!IsWatchedProperty(cx, newShape));
> 
>+    AutoShapeRooter asr(cx, newShape);

This is not necessary, because newShape must be mapped by the watched object.

>@@ -1027,16 +1030,17 @@ JS_SetWatchPoint(JSContext *cx, JSObject
>                                          JSPROP_ENUMERATE, 0, 0, &prop)) {
>                 return JS_FALSE;
>             }
>             shape = (Shape *) prop;
>         }
>     } else if (pobj != obj) {
>         /* Clone the prototype property so we can watch the right object. */
>         AutoValueRooter valroot(cx);
>+        AutoShapeRooter asr(cx, shape);

This is not necessary because shape is in pobj while used by the immediately following code. It is not used after the conditional pobj->getProperty call.

>@@ -1061,16 +1065,18 @@ JS_SetWatchPoint(JSContext *cx, JSObject
>         if (!js_DefineNativeProperty(cx, obj, propid, valroot.value(),
>                                      getter, setter, attrs, flags,
>                                      shortid, &prop)) {
>             return JS_FALSE;
>         }
>         shape = (Shape *) prop;
>     }
> 
>+    AutoShapeRooter asr(cx, shape);
>+
>     /*
>      * At this point, prop/shape exists in obj, obj is locked, and we must
>      * unlock the object before returning.

The comments about object locking are obsolete, please remove, but the comment about shape existing in obj is correct and that roots shape. So the AutoShapeRooter here is not needed.

>@@ -1633,16 +1639,18 @@ JS_PropertyIterator(JSObject *obj, JSSco
> JS_PUBLIC_API(JSBool)
> JS_GetPropertyDesc(JSContext *cx, JSObject *obj, JSScopeProperty *sprop,
>                    JSPropertyDesc *pd)
> {
>     assertSameCompartment(cx, obj);
>     Shape *shape = (Shape *) sprop;
>     pd->id = IdToJsval(shape->id);
> 
>+    AutoShapeRooter asr(cx, shape);
>+

This isn't enough -- the API returns shape pointers via the ancient JSScopeProperty * opaque pointer type and a delete could leave such a pointer dangling after GC. Rooting here is not enough and it covers up for the fact that this API is untenable. Suggest we leave this alone now, deprecate in docs, and remove after Firefox 4.

>@@ -1712,16 +1720,17 @@ JS_GetPropertyDescArray(JSContext *cx, J
>         return JS_FALSE;
>     uint32 i = 0;
>     for (Shape::Range r = obj->lastProperty()->all(); !r.empty(); r.popFront()) {
>         if (!js_AddRoot(cx, Valueify(&pd[i].id), NULL))
>             goto bad;
>         if (!js_AddRoot(cx, Valueify(&pd[i].value), NULL))
>             goto bad;
>         Shape *shape = const_cast<Shape *>(&r.front());
>+        AutoShapeRooter asr(cx, shape);
>         if (!JS_GetPropertyDesc(cx, obj, reinterpret_cast<JSScopeProperty *>(shape), &pd[i]))

Nothing here could delete the property so there's no need to root. This internal use of the unsafe ancient API is ok.

>+++ b/js/src/jsfun.cpp
>@@ -1460,16 +1460,18 @@ JSStackFrame::getValidCalleeObject(JSCon
>                  * and typed arrays have native prototypes, so keep going.
>                  */
>                 if (!thisp->isNative())
>                     continue;
> 
>                 if (thisp->hasMethodBarrier()) {
>                     const Shape *shape = thisp->nativeLookup(ATOM_TO_JSID(fun->methodAtom()));
>                     if (shape) {
>+                        AutoShapeRooter asr(cx, shape);

Again this is unnecessary since the shape is in the object while the shape local is live here.

>+++ b/js/src/jsinterp.cpp
>@@ -2038,16 +2038,17 @@ AssertValidPropertyCacheHit(JSContext *c
>     if (!ok)
>         return false;
>     if (cx->runtime->gcNumber != sample || entry->vshape() != pobj->shape())
>         return true;
>     JS_ASSERT(prop);
>     JS_ASSERT(pobj == found);
> 
>     const Shape *shape = (Shape *) prop;
>+    AutoShapeRooter asr(cx, shape);

This is not necessary given the shape being rooted by being linked from pobj->lastProp.

>@@ -4355,16 +4356,17 @@ BEGIN_CASE(JSOP_SETMETHOD)
>              * know that the entry applies to regs.pc and that obj's shape
>              * matches.
>              *
>              * The entry predicts either a new property to be added directly to
>              * obj by this set, or on an existing "own" property, or on a
>              * prototype property that has a setter.
>              */
>             const Shape *shape = entry->vword.toShape();
>+            AutoShapeRooter asr(cx, shape);

This is a fast path and we should not add gratuitous rooting. It is not needed since the shape is in the object, objects are single-threaded, and there's no call-out that might delete the property.

>@@ -5424,16 +5426,17 @@ BEGIN_CASE(JSOP_DEFFUN)
>     JSObject *parent = &regs.fp->varobj(cx);
> 
>     /* ES5 10.5 (NB: with subsequent errata). */
>     jsid id = ATOM_TO_JSID(fun->atom);
>     JSProperty *prop = NULL;
>     JSObject *pobj;
>     if (!parent->lookupProperty(cx, id, &pobj, &prop))
>         goto error;
>+    AutoPropertyRooter apr(cx, pobj, prop);

Ditto. It is not time to add paranoid and potentially performance-regressing rooting, and by inspection it's not needed here.

>+++ b/js/src/jsobj.cpp
>@@ -234,16 +234,17 @@ MarkSharpObjects(JSContext *cx, JSObject
>             ok = obj->lookupProperty(cx, id, &obj2, &prop);
>             if (!ok)
>                 break;
>             if (!prop)
>                 continue;
>             bool hasGetter, hasSetter;
>             AutoValueRooter v(cx);
>             AutoValueRooter setter(cx);
>+            AutoPropertyRooter apr(cx, obj2, prop);

This is not necessary. Inspect the uses of prop and (below, the aliasing local) shape:

>             if (obj2->isNative()) {
>                 const Shape *shape = (Shape *) prop;
>                 hasGetter = shape->hasGetterValue();
>                 hasSetter = shape->hasSetterValue();
>                 if (hasGetter)
>                     v.set(shape->getterValue());
>                 if (hasSetter)
>                     setter.set(shape->setterValue());

There is no potential call-out that could delete the property.

>@@ -565,16 +566,18 @@ obj_toSource(JSContext *cx, uintN argc, 
>     for (jsint i = 0, length = ida->length; i < length; i++) {
>         /* Get strings for id and value and GC-root them via vp. */
>         jsid id = ida->vector[i];
> 
>         ok = obj->lookupProperty(cx, id, &obj2, &prop);
>         if (!ok)
>             goto error;
> 
>+        AutoPropertyRooter apr(cx, obj2, prop);

Ditto.

This is like old Unix run-to-completion-in-kernel analysis: you have to know where the sleep() calls nest. In this case you have to know where delete and gc calls nest, and they do not nest. Adding rooting now is a waste of effort and a potential perf hit (not on toSource but elsewhere).

>@@ -1472,16 +1475,17 @@ js_HasOwnProperty(JSContext *cx, LookupP
>     if (!(lookup ? lookup : js_LookupProperty)(cx, obj, id, objp, propp))
>         return false;
>     if (!*propp)
>         return true;
> 
>     if (*objp == obj)
>         return true;
> 
>+    AutoPropertyRooter apr(cx, *objp, *propp);
>     Class *clasp = (*objp)->getClass();
>     JSObject *outer = NULL;
>     if (JSObjectOp op = (*objp)->getClass()->ext.outerObject) {

The rooter is not needed provided outerObject implementations never delete properties -- and they do not.

>@@ -1788,16 +1792,17 @@ js_GetOwnPropertyDescriptor(JSContext *c
>         return false;
>     if (!prop) {
>         vp->setUndefined();
>         return true;
>     }
> 
>     Value roots[] = { UndefinedValue(), UndefinedValue(), UndefinedValue() };
>     AutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(roots), roots);
>+    AutoPropertyRooter apr(cx, pobj, prop);
>     unsigned attrs;
>     bool doGet = true;
>     if (pobj->isNative()) {
>         Shape *shape = (Shape *) prop;
>         attrs = shape->attributes();
>         if (attrs & (JSPROP_GETTER | JSPROP_SETTER)) {
>             doGet = false;
>             if (attrs & JSPROP_GETTER)

Again this rooter is unnecessary overhead. We do not call out to anything that might delete, and we collect members of shape in rooted storage.

>@@ -2070,16 +2075,18 @@ DefinePropertyOnObject(JSContext *cx, JS
>     JSProperty *current;
>     JSObject *obj2;
>     JS_ASSERT(!obj->getOps()->lookupProperty);
>     if (!js_HasOwnProperty(cx, NULL, obj, desc.id, &obj2, &current))
>         return JS_FALSE;
> 
>     JS_ASSERT(!obj->getOps()->defineProperty);
> 
>+    AutoPropertyRooter apr(cx, obj2, current);
>+

DefinePropertyOnObject does not call out to anything that might delete, so this too is deadwood.

>@@ -4610,16 +4617,17 @@ js_DefineProperty(JSContext *cx, JSObjec
>  * Backward compatibility requires allowing addProperty hooks to mutate the
>  * nominal initial value of a slotful property, while GC safety wants that
>  * value to be stored before the call-out through the hook.  Optimize to do
>  * both while saving cycles for classes that stub their addProperty hook.
>  */
> static inline bool
> CallAddPropertyHook(JSContext *cx, Class *clasp, JSObject *obj, const Shape *shape, Value *vp)
> {
>+    AutoShapeRooter asr(cx, shape);
>     if (clasp->addProperty != PropertyStub) {
>         Value nominal = *vp;
> 
>         if (!CallJSPropertyOp(cx, clasp->addProperty, obj, shape->id, vp))
>             return false;
>         if (*vp != nominal) {
>             if (obj->containsSlot(shape->slot))
>                 obj->nativeSetSlot(shape->slot, *vp);

As with outerObject, the rule is addProperty must not delete anything. This too is deadwood.

>@@ -4656,16 +4664,17 @@ js_DefineNativeProperty(JSContext *cx, J
>          * already in obj and obj has its own (mutable) scope.  So if we are
>          * defining a getter whose setter was already defined, or vice versa,
>          * finish the job via obj->changeProperty, and refresh the property
>          * cache line for (obj, id) to map shape.
>          */
>         if (!js_LookupProperty(cx, obj, id, &pobj, &prop))
>             return JS_FALSE;
>         shape = (Shape *) prop;
>+        AutoShapeRooter asr(cx, shape);
>         if (shape && pobj == obj && shape->isAccessorDescriptor()) {
>             shape = obj->changeProperty(cx, shape, attrs,
>                                         JSPROP_GETTER | JSPROP_SETTER,
>                                         (attrs & JSPROP_GETTER)
>                                         ? getter
>                                         : shape->getter(),
>                                         (attrs & JSPROP_SETTER)
>                                         ? setter

Again, useless rooting. Now how the r.v. of obj->changeProperty kills shape so there's no need to root it!

>@@ -4747,16 +4756,18 @@ js_DefineNativeProperty(JSContext *cx, J
> #ifdef DEBUG
>             const Shape *newshape =
> #endif
>                 obj->methodWriteBarrier(cx, *shape, value);
>             JS_ASSERT(newshape == shape);
>         }
>     }
> 
>+    AutoShapeRooter asr(cx, shape);
>+    
>     /* Store value before calling addProperty, in case the latter GC's. */
>     if (obj->containsSlot(shape->slot))
>         obj->nativeSetSlot(shape->slot, value);
> 
>     /* XXXbe called with lock held */
>     Value valueCopy = value;
>     if (!CallAddPropertyHook(cx, clasp, obj, shape, &valueCopy)) {
>         obj->removeProperty(cx, id);

Ditto earlier comments.

>@@ -5167,16 +5178,19 @@ js_FindIdentifierBase(JSContext *cx, JSO
>     } while (obj->getParent());
>     return obj;
> }
> 
> static JS_ALWAYS_INLINE JSBool
> js_NativeGetInline(JSContext *cx, JSObject *receiver, JSObject *obj, JSObject *pobj,
>                    const Shape *shape, uintN getHow, Value *vp)
> {
>+    AutoShapeRooter tvr(cx, shape);
>+    AutoObjectRooter tvr2(cx, pobj);

These were locally scoped on purpose. Why are they hoisted by this patch?

> js_NativeSet(JSContext *cx, JSObject *obj, const Shape *shape, bool added, bool strict, Value *vp)
> {
>+    AutoShapeRooter tvr(cx, shape);

Ditto.

>@@ -5363,16 +5376,18 @@ js_GetPropertyHelperWithShapeInline(JSCo
>                                           JSDVG_IGNORE_STACK, IdToValue(id),
>                                           NULL, NULL, NULL)) {
>                 return JS_FALSE;
>             }
>         }
>         return JS_TRUE;
>     }
> 
>+    AutoPropertyRooter apr(cx, obj2, prop);
>+
>     if (!obj2->isNative()) {
>         return obj2->isProxy()
>                ? JSProxy::get(cx, obj2, receiver, id, vp)
>                : obj2->getProperty(cx, id, vp);
>     }
> 
>     shape = (Shape *) prop;
>     *shapeOut = shape;

The rest of the function is:

    if (getHow & JSGET_CACHE_RESULT) {
        JS_ASSERT_NOT_ON_TRACE(cx);
        JS_PROPERTY_CACHE(cx).fill(cx, aobj, 0, protoIndex, obj2, shape);
    }

    /* This call site is hot -- use the always-inlined variant of js_NativeGet(). */
    if (!js_NativeGetInline(cx, receiver, obj, obj2, shape, getHow, vp))
        return JS_FALSE;

    return JS_TRUE;

and there's no place here that might delete, except for the getter called by js_NativeGetInline, but that function roots shape so it won't be GC'ed. This all works as designed as it does not need unnecessary extra rooting.

>@@ -5524,16 +5539,19 @@ js_SetPropertyHelper(JSContext *cx, JSOb
> 
>     /* Convert string indices to integers if appropriate. */
>     id = js_CheckForStringIndex(id);
> 
>     protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
>                                             &pobj, &prop);
>     if (protoIndex < 0)
>         return JS_FALSE;
>+
>+    AutoPropertyRooter apr(cx, pobj, prop);
>+
>     if (prop) {
>         if (!pobj->isNative()) {
>             if (pobj->isProxy()) {
>                 AutoPropertyDescriptorRooter pd(cx);
>                 if (!JSProxy::getPropertyDescriptor(cx, pobj, id, true, &pd))
>                     return false;
> 
>                 if (pd.attrs & JSPROP_SHARED)

Same deal here as in js_GetPropertyHelper*.

>@@ -5655,16 +5673,17 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * cache, as the interpreter has no fast path for these unusual
>              * cases.
>              */
>             bool identical = shape->isMethod() && &shape->methodObject() == &vp->toObject();
>             if (!identical) {
>                 shape = obj->methodShapeChange(cx, *shape);
>                 if (!shape)
>                     return false;
>+                AutoShapeRooter asr(cx, shape);

If the method shape changed, the new shape returned and assigned to the shape local is rooted by being mapped by obj.

>@@ -5710,16 +5729,17 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>                 getter = CastAsPropertyOp(funobj);
>             }
>         }
> 
>         shape = obj->putProperty(cx, id, getter, setter, SHAPE_INVALID_SLOT,
>                                  attrs, flags, shortid);
>         if (!shape)
>             return JS_FALSE;
>+        AutoShapeRooter asr(cx, shape);

Ditto.

>@@ -5800,16 +5820,17 @@ js_DeleteProperty(JSContext *cx, JSObjec
> 
>     rval->setBoolean(true);
> 
>     /* Convert string indices to integers if appropriate. */
>     id = js_CheckForStringIndex(id);
> 
>     if (!js_LookupProperty(cx, obj, id, &proto, &prop))
>         return false;
>+    AutoPropertyRooter apr(cx, proto, prop);
>     if (!prop || proto != obj) {
>         /*
>          * If the property was found in a native prototype, check whether it's
>          * shared and permanent.  Such a property stands for direct properties
>          * in all delegating objects, matching ECMA semantics without bloating
>          * each delegating object.
>          */
>         if (prop && proto->isNative()) {

This code does indeed delete a shape but it doesn't use prop or shape after doing so (not shown further below). Again, gratuitous!

> JSObject::methodReadBarrier(JSContext *cx, const js::Shape &shape, js::Value *vp)
> {
>+    js::AutoShapeRooter asr(cx, &shape);

No. shape is in the |this| object by definition. It's not used after the write-back via methodShapeChange->js_SetPropertyHelper -- the newshape is, of course.

This works as designed and I'm annoyed Andreas sent you on a Snipe hunt adding unnecessary rooting to this code!

Same goes for remaining few hunks. This was a waste of effort, I'm sorry to say. Do not land it.

/be

Comment 4

[Brendan Eich [:brendan]]

We have real bugs to try to fix, and real crash reports, including some that may be shape-related but are in no way addresed by the patch. We need to understand our ownership and execution models, enforce them efficiently with C++ RAII and type-system enforcement where we can, and otherwise (where we need to leave old code alone in order to ship), inspect and make proofs in comments.

Adding shape rooting where objects own shapes and this is provable (and indeed essential to the correctness of the code being patched!) is just wrong. Please stop.

/be

Comment 5

[Andreas Gal :gal]

> >     }
> >+    AutoPropertyRooter apr(cx, obj2, prop);
> 
> This is not needed, because js_Atomize might run a GC it will not delete shape
> from obj2.

I don't think this is entirely accurate. XPConnect hooks into the GC using a GCCallback and is notified when the GC is done (JSGC_END) and will do

            case JSGC_END:
            {
                // NOTE that this event happens outside of the gc lock in
                // the js engine. So this could be simultaneous with the
                // events above.

                // Do any deferred released of native objects.
                DoDeferredRelease(self->mNativesToReleaseArray);
                break;
	    }

template<class T> static void
DoDeferredRelease(nsTArray<T> &array)
{
    while(1)
    {
        PRUint32 count = array.Length();
        if(!count)
	{
            array.Compact();
            break;
	}
        T wrapper = array[count-1];
        array.RemoveElementAt(count-1);
        NS_RELEASE(wrapper);
    }
}

NS_RELEASE can trigger finalizers which can cause almost arbitrary code execution, including C++ JSAPI calls setting slots (and thereby unrooting the shape), or running JS code, which can modify obj.

> 
> > UpdateWatchpointShape(JSContext *cx, JSWatchPoint *wp, const Shape *newShape)
> > {
> >     JS_ASSERT_IF(wp->shape, wp->shape->id == newShape->id);
> >     JS_ASSERT(!IsWatchedProperty(cx, newShape));
> > 
> >+    AutoShapeRooter asr(cx, newShape);
> 
> This is not necessary, because newShape must be mapped by the watched object.

Right after the new rooter we call WrapWatchedSetter, which does js_NewFunction, which can GC and through that unroot the shape (see above), or it might also run a resolve hook for F.p. I don't think in this particular case any JS would run in the resolve hook since we check for standard classes very early, but in general resolve hooks are scripted in our embedding (i.e. console, CA_init), so many opportunities here to mess with obj.

> This is not necessary because shape is in pobj while used by the immediately
> following code. It is not used after the conditional pobj->getProperty call.
> 
> >@@ -1061,16 +1065,18 @@ JS_SetWatchPoint(JSContext *cx, JSObject
> >         if (!js_DefineNativeProperty(cx, obj, propid, valroot.value(),
> >                                      getter, setter, attrs, flags,
> >                                      shortid, &prop)) {
> >             return JS_FALSE;
> >         }
> >         shape = (Shape *) prop;
> >     }
> > 
> >+    AutoShapeRooter asr(cx, shape);
> >+
> >     /*
> >      * At this point, prop/shape exists in obj, obj is locked, and we must
> >      * unlock the object before returning.
> 
> The comments about object locking are obsolete, please remove, but the comment
> about shape existing in obj is correct and that roots shape. So the
> AutoShapeRooter here is not needed.

The code does UpdateWatchpointShape here as well, with can GC and run resolve hooks, so I don't think this rooter is moot either.

> >@@ -1633,16 +1639,18 @@ JS_PropertyIterator(JSObject *obj, JSSco
> > JS_PUBLIC_API(JSBool)
> > JS_GetPropertyDesc(JSContext *cx, JSObject *obj, JSScopeProperty *sprop,
> >                    JSPropertyDesc *pd)
> > {
> >     assertSameCompartment(cx, obj);
> >     Shape *shape = (Shape *) sprop;
> >     pd->id = IdToJsval(shape->id);
> > 
> >+    AutoShapeRooter asr(cx, shape);
> >+
> 
> This isn't enough -- the API returns shape pointers via the ancient
> JSScopeProperty * opaque pointer type and a delete could leave such a pointer
> dangling after GC. Rooting here is not enough and it covers up for the fact
> that this API is untenable. Suggest we leave this alone now, deprecate in docs,
> and remove after Firefox 4.

The stack scanner would make this safe actually. But the API is pretty bad. I am certainly not opposed to removing it.

> >@@ -1712,16 +1720,17 @@ JS_GetPropertyDescArray(JSContext *cx, J
> >         return JS_FALSE;
> >     uint32 i = 0;
> >     for (Shape::Range r = obj->lastProperty()->all(); !r.empty(); r.popFront()) {
> >         if (!js_AddRoot(cx, Valueify(&pd[i].id), NULL))
> >             goto bad;
> >         if (!js_AddRoot(cx, Valueify(&pd[i].value), NULL))
> >             goto bad;
> >         Shape *shape = const_cast<Shape *>(&r.front());
> >+        AutoShapeRooter asr(cx, shape);
> >         if (!JS_GetPropertyDesc(cx, obj, reinterpret_cast<JSScopeProperty *>(shape), &pd[i]))
> 
> Nothing here could delete the property so there's no need to root. This
> internal use of the unsafe ancient API is ok.

Why are we adding roots for everything except shape then? It looks to me that JS_GetPropertyDesc does a regular js_GetProperty, which will run getters, so this could very well be deleted on the fly and thats why we have the rooters there and adding a shape rooter makes sense. But I might be wrong here. I didn't fully track this down.

> >+++ b/js/src/jsfun.cpp
> >@@ -1460,16 +1460,18 @@ JSStackFrame::getValidCalleeObject(JSCon
> >                  * and typed arrays have native prototypes, so keep going.
> >                  */
> >                 if (!thisp->isNative())
> >                     continue;
> > 
> >                 if (thisp->hasMethodBarrier()) {
> >                     const Shape *shape = thisp->nativeLookup(ATOM_TO_JSID(fun->methodAtom()));
> >                     if (shape) {
> >+                        AutoShapeRooter asr(cx, shape);
> 
> Again this is unnecessary since the shape is in the object while the shape
> local is live here.

methodReadBarrier() can allocate and trigger a GC. obj will be held alive by the machine stack pointer to it, but if some code in the GC callback changes the property shape can be unrooted.

I skipped past the rest of the comments, since I think the list above is sufficient to make my point: we cannot rely on human inspection for rooting. It has never worked well for us, thats why we got the conservative scanner in the first place. I see only 3 ways to ensure rooting:

1. static analysis
2. conservative scanning
3. V8-like typed handles and very careful design

Humans reading code doesn't work. Bill's patch is an ugly hack neither of us likes either, but it seems lower risk than (2). We should look into (1) for future versions if we want to get away from (2), and I really like (3) too. But for now this hack is the best way to address the problems we are seeing in FF4. As soon we move shapes into the GC heap we can take this rooters out (days after FF4 branches).

If you have performance concerns, we should run SS and V8 and narrowly take out any rooters that hit critical paths. This late in the release cycle I rather take an extra rooter or two in code paths we don't care about and where we will remove them from for future releases anyway than rely on humans trying to judge whether the rooter is necessary or not, because we will get it wrong almost every time.

Any kind of callback past the JSAPI invalidates any assumptions we can make, and even within our own code we are just one patch away from someone adding a GC point without regard for our prior analysis and assumptions.

I think we should take bill's patch, minus any rooters that regress performance, and file a followup to remove this immediately after FF4 along with the shapes in GC heap patch land. I am open to alternative suggestions though.

Comment 6

[David Mandelin [:dmandelin]]

Do we really have evidence that adding shape rooting is going to help us in Fx4? GC mark phase crashes are about 1.5% of the total--big enough that if we can fix it, it would be very valuable, but small enough so that if we don't know how to fix it, we can reasonably punt. Without good evidence this will actually reduce crashes, it's just shooting in the dark, and I'm sure there is higher-value stuff Bill could be working on.

Comment 7

[Brendan Eich [:brendan]]

Comment 5 is pure FUD, and a poor excuse for code review failing to analyze the particulars! In js_NativeGet, for instance, there are no call-outs through API hooks justifying the hoisting of the rooter.

Most of the words in comment 5 are bootstrapped on the first paragraph, but that is bogus too. No finalizer or GC end callback deletes a property from a live object. If this really were an issue we could stop it there, not by adding unnecessary rooting all over.

Comment 6 is too kind. This is not shooting in the dark, it's firing on civilians.

I'm still pretty outraged about this snipe hunt. I was mean to roc about spending energy on a non-blocker (and trying to bum's-rush something that has an orderly deprecation/obsolescence plan; this was the remove sharp variables bug). I have to be mean here too, because being nice is not going to cut through the FUD. Back to blockers!

This bug is not s-s, either. That's another piece of FUD.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Comment 5 is pure FUD, and a poor excuse for code review failing to analyze the
> particulars! In js_NativeGet, for instance, there are no call-outs through API
> hooks justifying the hoisting of the rooter.

And if C++ inline methods are too hard to analyze we should go back to C public member accesses. If you can't tell where the call-outs hide then there's no way to enforce any invariant (even a rooter won't help when shape presence in the object is required; see the propertyRemovals counter).

A better patch would be to add something like a smart pointer helper to pass by conversion-reference (a la getter_AddRefs), for the propp out parameter of JSObject::lookupProperty. That could root, or better, require that the shape is still in pobj on destruction.

Almost anything smarter and consolidated, focused on the concern, would be better than wasting time adding redundant rooting.

But we have more than enough evidence to chase from crash reports and blocker bugs, that should take precedence over this idea.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> A better patch would be to add something like a smart pointer helper to pass by
> conversion-reference (a la getter_AddRefs), for the propp out parameter of
> JSObject::lookupProperty. That could root, or better, require that the shape is
> still in pobj on destruction.

In DEBUG builds only.

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Comment 5 is pure FUD, and a poor excuse for code review failing to analyze the
> particulars! In js_NativeGet, for instance, there are no call-outs through API
> hooks justifying the hoisting of the rooter.

One  more thing: that hoisting is bad for performance because the fast paths in js_NativeGet and js_NativeSet are carefully organized to return early. So the explicit block scope around the rooter later, in the slot (getter or setter call-out) path, is intentional and it should not be meddled with just because C++ code reading requires more knowledge of inline methods.

/be

Comment 11

[Andreas Gal :gal]

Attachment: patch

I apologize. I was under the assumption that the GC callbacks can trigger JS API calls. I pushed the attached patch to try. If it doesn't trip anything, I am wrong and this bug is indeed invalid.

Comment 12

[Andreas Gal :gal]

http://hg.mozilla.org/try/pushloghtml?changeset=21f7fe13befb

Comment 13

[Brendan Eich [:brendan]]

Andreas, that patch is too restrictive. It will probably fail on try but that does not prove there's a problem in the places the patch touches. False dilemma: either no API calls from the JSGC_END callback, or add shape rooting (but not presence in object) all over.

Look again at the particulars, in this case the JS_AliasProperty case where you worried js_Atomize could last-ditch-GC and some JSGC_END callback would delete the property.

First, there's no sane way a GC callback can delete a property from a live object. Any such delete is a bug to fix (and not fixable by rooting -- more below after "Third").

Second, the JSGC_END callback runs after GC is done and we loop only on shutdown, so there is no way even if a GC end callback *did* delete the property for the shape to go free. You'd have to run another GC, but last-ditch runs only one.

Third, suppose the delete from GC callback was required for some insane reason. The JS_AliasProeprty API caller is trying to alias a property that may own a slot and the slot would go away from underneath it. So rooting the shape is not enough!

The right sanity-checking patch would be something like sampling propertyRemovals and asserting it has not gone up.

/be

Comment 14

[Andreas Gal :gal]

The patch is intended to prove me wrong, not prove me right. If it passes, GC does not recurse back into JS and this bug is invalid. If it fails, it doesn't mean I am right. The JS API call could be harmless and just reading state (i.e. JS_IsAboutToBeFinalized). In that case I will exclude those cases and re-try. If a couple of iterations of this don't dig up anything, the bugs is invalid. Extensions still could do something insane, but I care a lot less about that than our own baseline code.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Now we do conservative scanning.