499772 - TM: TraceRecorder::test_property_cache missing JSClass.getProperty checks when a property isn't found on an object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Abort if JSClass.getProperty isn't stubbed, guard on the class at runtime

See bug 452899, particularly the verbose log, when getlocalprop is traced: we attempt to trace the reading of wrappedStmt.params, which isn't yet present on the object, so we do the walk-up-the-proto-chain-shape-guarding trick and return undefined.  Then we enter the interpreter, resolve the property returns nothing, then we call JSClass.getProperty -- which is not stubbed and which returns a value, and then subsequent traced methods think they're operating on an object, and (I think, memory hazy, verifying details is an exercise for the reader) when we enter trace we assert when unboxing undefined as an object.

This can bite extensions which have loops which include accesses to properties defined by nsIXPCScriptable::GetProperty (not to mention our own code, as evidenced by the bug this blocks), and I expect such will happen in extensions (to what extent it's difficult to say), hence the wanted? nomination.

Comment 1

[Jeff Walden [:Waldo]]

(Ignore the spurious change, didn't realize I'd let that into this patch.)

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 384469 [details] [diff] [review]
Abort if JSClass.getProperty isn't stubbed, guard on the class at runtime

>From: Jeff Walden <[e-mail]>
>
>Ware JSClass.getProperty when tracing through an undefined-property access.  NOT REVIEWED YET
>
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -9062,6 +9062,19 @@ TraceRecorder::prop(JSObject* obj, LIns*
>     const JSCodeSpec& cs = js_CodeSpec[*cx->fp->regs->pc];
>     if (PCVAL_IS_NULL(pcval)) {
>         /*
>+         * We can't trace here if a value might be returned by the object's
>+         * JSClass.getProperty hook.  We could specialize to guard on just the

Wahh, cheese-eating French spacing.

>+         * identity of JSClass.getProperty, but a mere class guard will be
>+         * slightly faster and is more likely to prevent overtracing if a
>+         * differing class is a good predictor of future aborts or side exits.

How would we know that? This code wouldn't.

S'ok to use a simpler "cover" guard for now.

>+         */
>+        if (OBJ_GET_CLASS(cx, obj)->getProperty != JS_PropertyStub) {
>+            ABORT_TRACE("can't trace through access to undefined property if "
>+                        "JSClass.getProperty hook might return a value");
>+        }
>+        guardClass(obj, obj_ins, OBJ_GET_CLASS(cx, obj), snapshot(MISMATCH_EXIT));
>+
>+        /*
>          * This trace will be valid as long as neither the object nor any object
>          * on its prototype chain change shape.

Bonus for fixing the grammar here by using "changes".

/be

Comment 3

[Jeff Walden [:Waldo]]

Attachment: No more cheese means no more wine :-( , wahh back, adjust some comments and the abort message

While I'm thinking of it, do the shape guards defend against JSClass.getProperty or JSObjectOps.getProperty hooks along the proto chain?  (Gaah, stupid name conflict.)  Is either even called?  I don't *think* the former is from a code skim, but I don't know about the latter.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 384491 [details] [diff] [review]
No more cheese means no more wine :-( , wahh back, adjust some comments and the abort message

I still think the comment is wordy. :)  You had me at "simpler".

> While I'm thinking of it, do the shape guards defend against
> JSClass.getProperty or JSObjectOps.getProperty hooks along the proto chain? 
> (Gaah, stupid name conflict.)  Is either even called?  I don't *think* the
> former is from a code skim, but I don't know about the latter.

JSClass.getProperty is not called in that case, but JSObjectOps.getProperty is, and I don't know if we guard against that.

XPC wrappers apparently use the native JSObjectOps.getProperty, and I don't think it can matter for With objects.  If that is correct, it could only be a problem for E4X objects.

Again, not sure there *is* a problem, but it seems worth checking.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (From update of attachment 384491 [details] [diff] [review])
> I still think the comment is wordy. :)  You had me at "simpler".

Me too. Waldo, shorten!

> > While I'm thinking of it, do the shape guards defend against
> > JSClass.getProperty or JSObjectOps.getProperty hooks along the proto chain? 
> > (Gaah, stupid name conflict.)  Is either even called?  I don't *think* the
> > former is from a code skim, but I don't know about the latter.

Only the direct object's class getProperty is called as a last ditch before returning undefined (with a possible strict warning), see js_GetProperty:

    if (!prop) {
        *vp = JSVAL_VOID;

        if (!OBJ_GET_CLASS(cx, obj)->getProperty(cx, obj, ID_TO_VALUE(id), vp))
            return JS_FALSE;
    ...
 
> JSClass.getProperty is not called in that case,

Not on any proto, but on the direct object it is, as shown above.

> but JSObjectOps.getProperty is,
> and I don't know if we guard against that.

You mean where js_GetProperty finds the id'ed prop in a non-native object and so vectors off via OBJ_GET_PROPERTY:

    if (!OBJ_IS_NATIVE(obj2)) {
        OBJ_DROP_PROPERTY(cx, obj2, prop);
        return OBJ_GET_PROPERTY(cx, obj2, id, vp);
    }

But this won't trace because the property was found in a non-native proto, IIRC. Right? Agree this needs double-checking.

/be

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/21d46a88aed6

(In reply to comment #5)
> Me too. Waldo, shorten!

Okay, shortened a bit more.


> Only the direct object's class getProperty is called as a last ditch before
> returning undefined (with a possible strict warning), see js_GetProperty:

That was my reading, but it seems...not what I'd have expected.  Given:

  var o = Object.create(wrappedStmt);

I'd expect |o.params !== undefined|, but my reading says that just isn't the case.


> But this won't trace because the property was found in a non-native proto,
> IIRC. Right? Agree this needs double-checking.

Yeah, that case is fine, we'd have had problems long ago if it weren't -- see the map_is_native check in test_property_cache.

Time to let bake, then maybe get in 191 if we have the RCs to do it, else for a dot release.  Incidentally, I had an in-person JS assert debug request which turned out to be this bug a second time, so even if no one else hits it (unlikely) *we* will probably keep hitting it without a fix.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> > JSClass.getProperty is not called in that case,
> 
> Not on any proto, but on the direct object it is, as shown above.

Yes, that's exactly what I meant.

> > but JSObjectOps.getProperty is,
> > and I don't know if we guard against that.
> 
> You mean where js_GetProperty finds the id'ed prop in a non-native object and
> so vectors off via OBJ_GET_PROPERTY:
> 
>     if (!OBJ_IS_NATIVE(obj2)) {
>         OBJ_DROP_PROPERTY(cx, obj2, prop);
>         return OBJ_GET_PROPERTY(cx, obj2, id, vp);
>     }
> 
> But this won't trace because the property was found in a non-native proto,
> IIRC. Right? Agree this needs double-checking.

A few objects have a non-native JSObjectOps.getProperty but have scopes and count as native.  The property cache (at least) depends on an invariant that
  JSObjectOps.lookupProperty + js_NativeGet = JSObjectOps.getProperty
for these objects.  That looks reasonable, and I think it holds--I just didn't know about it before.

However I think we do probably have some bugs around these semi-native objects. Here's one:

  p = <a><b>ok</b></a>;
  c = {__proto__: p};
  assertEq(c.b, p.b);

  test.js:6: TypeError: Assertion failed: got (void 0), expected <b>ok</b>

I think that one could be fixed by patching js_LookupPropertyWithFlags:

         proto = LOCKED_OBJ_GET_PROTO(obj);
         JS_UNLOCK_OBJ(cx, obj);
         if (!proto)
             break;
-        if (!OBJ_IS_NATIVE(proto)) {
+        if (proto->map->ops->lookupProperty != js_LookupProperty) {
             if (!OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp))
                 return -1;
             return protoIndex + 1;
         }

But I'm not sure about the implications of this fix. It might result in the tracer filling the property cache in situations where the interpreter wouldn't. I don't know if that would be OK. I do know that *not* filling the property cache could be bad, since JIT and interpreter must see the same result type.  (Currently they see the same result type here because, thanks to the bug, the lookup misses.)

If we do not bump the XML node's shape when it is mutated, that's probably another bug.

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/21d46a88aed6

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 384491 [details] [diff] [review]
No more cheese means no more wine :-( , wahh back, adjust some comments and the abort message

Simple and well-understood fix, crash could bite an indeterminate number of extensions, also can bite if they touch our visible APIs just wrongly enough, and I think a web page might be able to trigger the crash...definitely something to take.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 384491 [details] [diff] [review]
No more cheese means no more wine :-( , wahh back, adjust some comments and the abort message

Let's get this in, please, ASAP.

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/79292a9fdeb1

Comment 12

[Bob Clary [:bc] (inactive)]

Jeff, how would we test this?

Comment 13

[Jeff Walden [:Waldo]]

The bug this blocks added an xpcshell test which hits this.  Other than that we could add a jsshell test if we hacked up an object with a JSClass.getProperty hook; I don't believe any such objects exists in the world of the shell, or at least I saw none when I looked during writing the patch, but it's certainly feasible.  I didn't think it was particularly important to do the shell-hackery and test-writing given the xpcshell-test backstop.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Waldo: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better yet the 3.5.1 release candidate: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/