Closed
Bug 596031
Opened 13 years ago
Closed 12 years ago
obj/obj2 mix-up in js_GetPropertyHelper()
Categories
(Core :: JavaScript Engine, defect)
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)
24.27 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
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
![]() |
Reporter | |
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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
![]() |
Reporter | |
Comment 5•13 years ago
|
||
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: 13 years ago
Resolution: --- → INVALID
Comment 6•13 years ago
|
||
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?
Assignee | ||
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
(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
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee | ||
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
(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
Assignee | ||
Comment 11•12 years ago
|
||
(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: --- → ?
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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?
Assignee | ||
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
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)
Comment 18•12 years ago
|
||
FWIW, I just pushed this to try (85abae22b304).
Updated•12 years ago
|
Attachment #487252 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 19•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9ec91c8f9b8e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 20•12 years ago
|
||
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.
Description
•