The attached testcase demonstrates the problem. It prints "undefined" without any changes, and "direct get of foo" when commenting in lines 3 or 11. Changing the (non-commented) [[Has]] trap to return false makes that "undefined" again.

OK, did a bit of digging here. Here's what I found: There are two tweakables in the testcase. Let's take each of them in turn: Let's start with |t|. If we make the object we do the access on a proxy, we call the |get| handler, but if we make it a native object that has a proxy on the prototype chain, then it fails. This is because of: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.cpp#288 If the object has a get hook directly, then we call it. Otherwise, we end up in GetPropertyHelper(). This is where the trouble begins. GetPropertyHelper erroneously calls LookupPropertyWithFlagsInline(), which calls JSObject::lookupGeneric() on the proxy. This *fails* here: http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.cpp#2733 if the |has| hook returns false. This happens because the DirectScriptedProxyHandler |has| hook checks the target, as per spec. Overriding that hook, then, changes the value returned, and thus the behavior. So, how do we fix this? From talks with till online, it seems like the sanest thing to do is rip LookupPropertyWithFlagsInline() out of GetPropertyHelper() altogether. All we really need, it looks like, is the shape and holder of the property, if found. So, for native objects, we should loop up the prototype chain on nativeLookup() and see what we get, bailing to calling JSObject::getGeneric() on the prototype if we find a non-native object. Since that will percolate up the rest of the prototype chain with the correct operation, we should be overall correct here. Jeff, what do you think about this approach? Does this seem sane from an engine point of view? A spec point of view?

As long as you call JSObject::getGeneric passing along the original object as receiver, I think that should be okay. The overall logic flow is probably still going to read very hackish, to be sure, but that sounds like it might fix the semantic wrongness, at least.

Comment on attachment 8549760 [details] [diff] [review] part 1 - Move Shape::get into NativeObject.cpp and rename it to CallGetter. Eliminate unused parameters and consolidate. Add some comments; delete some dead code. No change in behavior Review of attachment 8549760 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Just a few nits about comments. ::: js/src/vm/NativeObject.cpp @@ +1591,5 @@ > return true; > } > > bool > +js::NativeGetExistingProperty(JSContext *cx, HandleObject receiver, HandleNativeObject obj, N-n-no more pobj anywhere? \o/ @@ +1746,5 @@ > return true; > } > > // This call site is hot -- use the always-inlined variant of > // NativeGetExistingProperty(). Do we want the comment unedited, when that name corresponds to no function? @@ +1754,5 @@ > bool > js::NativeGetProperty(JSContext *cx, HandleNativeObject obj, HandleObject receiver, HandleId id, > MutableHandleValue vp) > { > /* This call site is hot -- use the always-inlined variant of GetPropertyHelper(). */ Same as above @@ +1761,5 @@ > > bool > js::NativeGetPropertyNoGC(JSContext *cx, NativeObject *obj, JSObject *receiver, jsid id, Value *vp) > { > AutoAssertNoException nogc(cx); Pre-existing, nothing to do with you: what a strange name. I guess it grew out of AutoAssertNoGC somehow, or someone changed the RAII class and didn't bother renaming it.

Comment on attachment 8549767 [details] [diff] [review] part 2 - Factor out GetNonexistentProperty. It's nice to separate it from the main path, because everything it does except setting vp to undefined is nonstandard. No change in behavior Review of attachment 8549767 [details] [diff] [review]: ----------------------------------------------------------------- Thank you thank you thank you for commenting all of this again. It's probably the best part about this rampage is the comments left behind.

Comment on attachment 8549768 [details] [diff] [review] part 3 - Reimplement GetPropertyInline to match ES6 9.1.8 Review of attachment 8549768 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Good tests. We still may need to be wary of strange behavior of DOM code using JS_LookupPropertyById() or whatever, and calling the wrong traps on proxies that inherit from other proxies, but this is not that problem.

(In reply to Eric Faust [:efaust] from comment #7) > > // This call site is hot -- use the always-inlined variant of > > // NativeGetExistingProperty(). > > Do we want the comment unedited, when that name corresponds to no function? Updated. > > js::NativeGetProperty(JSContext *cx, HandleNativeObject obj, HandleObject receiver, HandleId id, > > MutableHandleValue vp) > > { > > /* This call site is hot -- use the always-inlined variant of GetPropertyHelper(). */ Deleted. > > AutoAssertNoException nogc(cx); > > Pre-existing, nothing to do with you: what a strange name. Indeed! Changed.

Comment on attachment 8549768 [details] [diff] [review] part 3 - Reimplement GetPropertyInline to match ES6 9.1.8 Review of attachment 8549768 [details] [diff] [review]: ----------------------------------------------------------------- Pushed to try, after trivial rebasing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f2ad1bec83 ::: js/src/vm/NativeObject.cpp @@ +1784,5 @@ > + // doesn't have that many elements. > + // - We're being called from a resolve hook to assign to the property > + // being resolved. > + // - We're running in parallel mode so we've already done the whole > + // lookup (in LookupPropertyPure above). This is not actually true.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7a248c492f https://hg.mozilla.org/integration/mozilla-inbound/rev/b878c753fbe7 https://hg.mozilla.org/integration/mozilla-inbound/rev/615f118f2787

https://hg.mozilla.org/mozilla-central/rev/7a7a248c492f https://hg.mozilla.org/mozilla-central/rev/b878c753fbe7 https://hg.mozilla.org/mozilla-central/rev/615f118f2787