Current behavior on this: var ab = ArrayBuffer(5); var p = new Proxy(ab, {}) var ps = Object.getOwnPropertyDescriptor(Object.prototype, "__proto__").set; ps.call(p, {}); is *crickets*. Old behavior before bug 926012 on this was "TypeError: Object.prototype.__proto__ setter called on incompatible ArrayBuffer". Stepping through a bit, something looks pretty rotten. JSObject::setProto doesn't have a obj->getTaggedProto().isLazy(), so we fall into SetClassAndProto with a proxy, which seems pretty wrong -- direct proxies should be forwarding to JSObject::setProto on their target, and that doesn't happen. Or am I missing/forgetting something I knew when I reviewed patches before?

OK, so it looks like there are really two problems at work here. The first is silly and trivial: I forgot to change ScriptedDirectProxies to using TaggedProto::LazyProto, so it just sets to proto on the proxy to null, which isn't the desired behavior. The second is more insidious. Even fixing that, we are then in the JSObject::setProto() system, which doesn't account for any of the checks in ProtoSetterImpl. In particular, I worry about the CheckAccess() call there. Is this something we should be worried about? It seems like maybe it's not something that should go away? Bobby, do you understand enough of that system to know? If the CheckAccess call isn't scary, then just moving the ArrayBuffer check to the top of JSObject::setProto() and making ScriptedDirectProxies actually forward properly should be enough to fix this.

I had this thought that maybe we can just move the CheckAccess call at well. I don't see how this is any different than what we used to have. Bobby, if the code here looks good, then we can just r+ this and stop worrying about it. This also fixes the original ArrayBuffer complaint.

I /think/ after reading some of the security callbacks, that we need to do a similar change for the forwarding proxies in JSObject::getProto(). If so, that mistake has been there for some time. I guess the right place to land that fix is in bug 950897? Again, all of this needs a once over from someone who actually understands what's supposed to be going on, but it seems the only logical way to structure all of this, and indeed the way it seems to be structured is to have the callback answer the question "is the access OK for this property ON THIS OBJECT" regardless of the access that originated it. Maybe none of this matters since we can't pierce security wrappers anyways?

Comment on attachment 8348327 [details] [diff] [review] Fix? Review of attachment 8348327 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, the CheckAccess call is pretty useless. It could probably go away. It's all tied up in the last bundle of CAPS crap, which can't quite tetris into oblivion until we do bug 794943. In the mean time, this patch is fine. ::: js/src/jsobjinlines.h @@ +428,5 @@ > + * have a mutable [[Prototype]]. > + */ > + if (obj->is<js::ArrayBufferObject>()) { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_INCOMPATIBLE_PROTO, > + "Object", "__proto__ setter", "ArrayBuffer"); Is "__proto__ setter" still the right string to pass here?

Backed out for one of the most thorough tree burnings I've seen in recent memory. Achievement unlocked, well played sir :D https://hg.mozilla.org/integration/mozilla-inbound/rev/4a613dc5e5c7 https://tbpl.mozilla.org/php/getParsedLog.php?id=32597630&tree=Mozilla-Inbound

Yeah, OK, that patch ended the world. Let's try again with less striking startup-blocking incorrectness. Remove the CheckAccess call from JSObject::setProto and replace it with an explicit check for Location objects, as discussed on IRC.

Comment on attachment 8359942 [details] [diff] [review] fixTotalMeltdown.patch Review of attachment 8359942 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I want an sr from Blake on the Location hack. Blake, the basic deal is that JSACC_PROTO only ever mattered for the Location object, and just handling that explicitly in the JS engine is easier than trying to rejigger all the CheckAccess stuff. ::: js/src/jsobjinlines.h @@ +441,5 @@ > + } > + > + /* > + * Explicityly disallow mutating the [[Prototype]] of Location objects > + * for flash-related security reasons. How about: Explicityly disallow mutating the [[Prototype]] of Location objects until Location moves to WebIDL bindings and gets proper [Unforgeable] semantics. Then, file a follow up bug to remove this hack, and make it depend on bug 832014.

Comment on attachment 8359942 [details] [diff] [review] fixTotalMeltdown.patch I'm fine with this. JSACC_PROTO should die in a fire as fast as possible.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/19cb3daa91a7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b97134e81798 take 2

https://hg.mozilla.org/mozilla-central/rev/19cb3daa91a7 https://hg.mozilla.org/mozilla-central/rev/b97134e81798