596031 - obj/obj2 mix-up in js_GetPropertyHelper()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

js_GetPropertyHelper() has this code:

    if (!obj2->isNative())
        return obj2->getProperty(cx, id, vp);

jorendorff says this is a bug.  Somehow 'obj' should be consulted when getting the property.

Comment 1

[Brendan Eich [:brendan]]

Jason, why is this a bug? It has been this way forever:

3.2          (fur      24-Apr-98):     if (!OBJ_IS_NATIVE(obj2)) {
3.2          (fur      24-Apr-98):      OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)sprop);
3.2          (fur      24-Apr-98):      return OBJ_GET_PROPERTY(cx, obj2, id, vp);
3.2          (fur      24-Apr-98):     }

(from cvs annotate searching back on cvs.mozilla.org/mozilla/js/src/jsobj.c to first change post-3.1 -- 3.1 was the initial CVS revision). Log message:

revision 3.2
date: 1998/04/24 00:30:04;  author: fur;  state: Exp;  lines: +1424 -717
Initial checkin of JavaScript 1.3, migrated from JSFUN13_BRANCH in /m/src repository

A non-native on the proto chain was found to contain the wanted id, so the property value comes from asking that object directly. There's no way for obj2 to know about obj (the native), which is not compatible with obj2.

/be

Comment 2

[Brendan Eich [:brendan]]

3.2 added JSObjectOps for LiveConnect integration, in a Netscape-private CVS repo, /m/src. That's the source of the "JavaScript 1.3" changes. Ah, memories.

I don't think this bug is valid.

/be

Comment 3

[Nicholas Nethercote [inactive]]

Jason said he would comment further... the problem apparently has something to do with proxies, which is why it's only recently become a problem.

Comment 4

[Brendan Eich [:brendan]]

Oh, no doubt. But we want a number of ObjectOps reforms based on Proxies and ES5 meta-methodology. I don't see how to do a spot-fix here, since ObjectOps.getProperty will have to change.

It's plausible we don't even want a "high-level vtable" as opposed to Class, the "low-level observer": gal proposed using if (isProxy()) tests all over to tail-call proxy code, else fall into native code.

/be

Comment 5

[Nicholas Nethercote [inactive]]

Since Jason asked me to open this bug but never bothered to comment, and Brendan thinks it's bogus, I will close it.  Please reopen if you disagree.

Comment 6

[Jason Orendorff [:jorendorff]]

Here's why I thought this was a bug:

Suppose A is the prototype of B and A.x is an accessor property. Then B.x will call the getter, passing B as the this-argument.

Unless A is a proxy! Then B.x behaves like A.x, and there's nothing A can do about it.

I see this is written into the Proxy semantics at <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics>. It is probably implied by ES5. It still seems wrong to me. Anyone else see what I'm talking about?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> I see this is written into the Proxy semantics at
> <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics>. It is
> probably implied by ES5. It still seems wrong to me. Anyone else see what I'm
> talking about?

Yes. See bug 607863.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> > I see this is written into the Proxy semantics at
> > <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics>. It is
> > probably implied by ES5. It still seems wrong to me. Anyone else see what I'm
> > talking about?
> 
> Yes. See bug 607863.

Let's blame that one on JSPROP_SHARED, Object.prototype.__proto__ instead of what Rhino and other impls do: magic id that works on all objects.

Some tension here with "dictionary" use-case of o = {__proto__:null, ...} or (now with ES5) o = Object.create(null, ...). Probably don't want to pollute those things. More in bug 607863.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This fixes this bug by pushing the receiver through all gets/sets. I narrowly only fixed this for proxies, but I could easily lift that restriction now. If there are easy and clever ways to reduce the number of obj, obj I added here (or if one of those points out a bug, please tell me!

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Created attachment 486756 [details] [diff] [review]
> Proposed fix
> 
> This fixes this bug by pushing the receiver through all gets/sets. I narrowly
> only fixed this for proxies, but I could easily lift that restriction now. If
> there are easy and clever ways to reduce the number of obj, obj I added here
> (or if one of those points out a bug, please tell me!

Overload a static inline helper that passes obj, obj from just one obj formal. That's what I did in my (needs rebasing on this bug's patch, and more work to boot) patch for bug 603201.

Can you do that and reattach? Should be quick.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> Can you do that and reattach? Should be quick.

I looked, and for 2 or 1 case of (obj, obj), I don't think it's worth another inline function. I'd prefer to leave things as-is.

Comment 12

[Johnny Stenback (:jst)]

This blocks beta7 cause it breaks jetpack.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 486756 [details] [diff] [review]
Proposed fix

>+js_GetPropertyStub(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, js::Value *vp)
>+js_SetPropertyStub(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, js::Value *vp,
>+                   JSBool strict)

"Stub" means no-op or default minimal implementation, but this retasks that suffix. Suggest "Full" instead, or just overload, but you'll need to cast to select the right function in the ?: expressions.

>+proxy_SetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp,
>+                  JSBool strict)
> {
>     // TODO: throwing away strict
>-    return JSProxy::set(cx, obj, obj, id, vp);
>+    return JSProxy::set(cx, obj, receiver, id, vp);
> }
 . . .
> JSWrapper::set(JSContext *cx, JSObject *wrapper, JSObject *receiver, jsid id, Value *vp)
> {
>-    SET(JS_SetPropertyById(cx, wrappedObject(wrapper), id, Jsvalify(vp)));
>+    // XXX strict?
>+    SET(wrappedObject(wrapper)->setProperty(cx, receiver, id, vp, false));
> }

No TODO and XXX, use FIXME: and cite the bug (write the tests now if you are feeling civic-minded) or bugs, which block es5strict. I'm assuming there are bugs here -- if not, comment why it's ok to pass false for strict.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I tried to land this and it bounced the narrow fix for the bug exposed in mochitests is:

diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -5240,19 +5240,20 @@ js_SetPropertyHelper(JSContext *cx, JSOb
 #ifdef JS_TRACER
               error: // TRACE_2 jumps here in case of error.
                 return JS_FALSE;
 #endif
             }
         }
 
         attrs = shape->attributes();
-        if (pobj != receiver) {
+        if (pobj != receiver || obj != receiver) {
             /*
-             * We found id in a prototype object: prepare to share or shadow.
+             * We found id in a prototype object or we found a property for an
+             * object whose prototype delegates to us: prepare to share or shadow.
              *
              * Don't clone a prototype property that doesn't have a slot.
              */
             if (!shape->hasSlot()) {
                 if (defineHow & JSDNP_CACHE_RESULT) {
 #ifdef JS_TRACER
                     JS_ASSERT_NOT_ON_TRACE(cx);
                     PropertyCacheEntry *entry =
@@ -5279,16 +5280,23 @@ js_SetPropertyHelper(JSContext *cx, JSOb
              */
             if (shape->hasShortID()) {
                 flags = Shape::HAS_SHORTID;
                 shortid = shape->shortid;
                 getter = shape->getter();
                 setter = shape->setter();
             }
 
+            if (receiver != obj) {
+                return receiver->isNative()
+                       ? js_DefineNativeProperty(cx, receiver, id, *vp, getter, setter,
+                                                 attrs, flags, shortid, NULL)
+                       : receiver->defineProperty(cx, id, *vp, NULL, NULL, attrs);
+            }
+
             /*
              * Forget we found the proto-property now that we've copied any
              * needed member values.
              */
             shape = NULL;
         }
 
         if (shape) {

I'll add comments, tests and fix the other direction later.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I don't actually think that's the right approach for the set case. Instead, it seems like it would be better to teach js_SetPropertyHelper about proxies (and JSPropertyDescriptors) and not have to do this "oh, we resolved the properly, but really for an object further up our prototype chain.

Brendan, any objections to that?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Ok, to explain the last comment:

When getting, if you run into an object that you don't understand (such as a proxy), then asking *it* to call the getter for you is no big deal because there are only two cases: call the getter with whatever receiver you're given or return the existing value that you find.

When setting, however, when you run into an object that you don't understand, then you need to actually modify the original object in the shadowing case, and IMO it makes much more sense to do so from the setProperty hook of *that* object instead of and unrelated hook much further down the prototype chain.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch

I'll write tests that we can check in tomorrow (I have some locally, but they're not automated).

Comment 18

[Johnny Stenback (:jst)]

FWIW, I just pushed this to try (85abae22b304).

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/9ec91c8f9b8e

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

For posterity, I check this in as <http://hg.mozilla.org/tracemonkey/rev/9a16d6dfa3c4> to tracemonkey earlier today.