643101 - lookupProperty, deleteProperty, get/setAttributes, setProperty need to pass through the receiver object

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

proxy traps want it, amongst others

Comment 1

[Andreas Gal :gal]

Attachment: lookupProperty

Comment 2

[Andreas Gal :gal]

This doesn't pipe receiver into Set/GetAttribute and Delete yet. The next patch will do that.

Comment 3

[Andreas Gal :gal]

Comment on attachment 520419 [details] [diff] [review]
lookupProperty

Bounce to jorendorff if you are overloaded?

Comment 4

[Andreas Gal :gal]

Attachment: deleteProperty (step 2)

Comment 5

[Andreas Gal :gal]

Attachment: get/setAttributes (step 3)

Comment 6

[Andreas Gal :gal]

Attachment: setProperty (step 4)

Comment 7

[Andreas Gal :gal]

I found at least 3 bugs reviewing the code (obj instead of obj2 used after FindProperty, with_getProperty didn't pass receiver through). Please look over my changes very carefully. I really don't like that we have the shortcut signature for lookupProperty without receiver. That hides bugs imo.

Comment 8

[Andreas Gal :gal]

jorendorff, waldo, if either of you wants to steal this I am sure Brendan doesn't mind. This needs some very carefully looking and reasoning for every hunk.

Comment 9

[Andreas Gal :gal]

Attachment: lookupProperty (step 1)

Comment 10

[Andreas Gal :gal]

Attachment: setProperty (part4)

I had to back off the change in FOR_GNAME (I picked up obj2 there instead of obj, but obj2 can be NULL). It should be the global obj either way. No need to mess with this I guess.

Comment 11

[Jason Orendorff [:jorendorff]]

Um. What happened to part 2? Still wanted, right?

Comment 12

[Andreas Gal :gal]

Jason, lets touch base real quick.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 520431 [details] [diff] [review]
setProperty (part4)

part 4 first, naturally...

In jsfun.cpp, StrictArgSetter
>     return js_DeleteProperty(cx, obj, obj, id, tvr.addr(), strict) &&
>-           js_SetProperty(cx, obj, id, vp, strict);
>+        js_SetProperty(cx, obj, obj, id, vp, strict);

Micro-nit: The indentation was right before.

In jsobj.cpp:
> static JSBool
> with_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
> {
>-    return obj->getProto()->getProperty(cx, id, vp);
>+    return obj->getProto()->getProperty(cx, receiver, id, vp);
> }

We don't ever want to pass the With object anywhere an actual *object*
is expected--With objects aren't even supposed to exist, right? So pass
obj->getProto() instead. If you want, this could even
  JS_ASSERT(receiver == obj);
in both with_GetProperty and with_SetProperty. For that to fail, the
object would have to have already been exposed--a capital offense.

I can't get this to bite in a test, because we have defense in depth
against With objects escaping. But please do fix it.

In jsproxy.cpp:
> static JSBool
>-proxy_SetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict)
>+proxy_SetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp, JSBool strict)
> {
>     return JSProxy::set(cx, obj, obj, id, strict, vp);
> }

Huh. This looks wrong to me--should be (cx, obj, receiver...), right?

Again, I can't trip it up in the shell. Does this just never get called?

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 520428 [details] [diff] [review]
get/setAttributes (step 3)

Clearing review flags for now (we don’t think the other patches are needed).

Comment 15

[David Bruant]

Marking this bug as invalid since it is not relevant with the direct proxies design.
Feel free to change the status if I'm mistaken.