Closed Bug 596031 Opened 11 years ago Closed 11 years ago

obj/obj2 mix-up in js_GetPropertyHelper()

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: n.nethercote, Assigned: mrbkap)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
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
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
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.
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
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
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?
(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.
(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
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached patch Proposed fix (obsolete) — Splinter Review
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!
Assignee: nnethercote → mrbkap
Status: REOPENED → ASSIGNED
Attachment #486756 - Flags: review?(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
(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.
Blocks: 607764
blocking2.0: --- → ?
This blocks beta7 cause it breaks jetpack.
blocking2.0: ? → beta7+
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
Attachment #486756 - Flags: review?(brendan) → review+
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.
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?
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.
Attached patch patchSplinter Review
I'll write tests that we can check in tomorrow (I have some locally, but they're not automated).
Attachment #486756 - Attachment is obsolete: true
Attachment #487252 - Flags: review?(brendan)
FWIW, I just pushed this to try (85abae22b304).
Attachment #487252 - Flags: review?(brendan) → review+
http://hg.mozilla.org/mozilla-central/rev/9ec91c8f9b8e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
For posterity, I check this in as <http://hg.mozilla.org/tracemonkey/rev/9a16d6dfa3c4> to tracemonkey earlier today.
Whiteboard: fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.