With: user_pref("dom.window_experimental_bindings", true); The pref was added in bug 1005966 but the larger project is bug 789261. Bug 854001 had the same crash signature, dunno if it's relevant.

Assertion failure: slot < (((GetObjectClass(obj))->flags >> 8) & (((uint32_t)1 << (8)) - 1)), at jsfriendapi.h:733

Hmm. We're landing in js::jit::IonBuilder::testShouldDOMCall and "proto" is a Function. In particular, it's the thing that was stuck on window.__proto__. Then we pass that to InstanceClassHasProtoAtDepth which crashes, because it expects to get actual DOM proto objects. Eric, Brian, what makes this happen in this case but not for normal DOM objects? I thought setting __proto__ on an object nuked TI for it, but that seems to not be happening here? Is that because the object hasSingletonType()? It's pretty simple to add a check to InstanceClassHasProtoAtDepth that we have a DOM prototype, but that would break if window.__proto__ was set to some random DOM prototype. So we need to either fix things so we don't call into InstanceClassHasProtoAtDepth at all in this case, or just nix this whole InstanceClassHasProtoAtDepth setup, teach the JS engine about DOMClass/DOMJSClass (or at least shadow versions of them?) and directly perform the necessary checks.

Or I guess we could keep the InstanceClassHasProtoAtDepth deal, but pass it the curType->clasp()?

This is an example of the shadow struct approach.

So, a few things. First, I don't think there's anythign special about this case, but I also don't see why jitcode would be invalidated. There shouldn't be any jitcode when that prototype set gets set, right? It happens earlier. In terms of why we don't have unknown properties affecting things, it's because it's an own-property on the global. Since it's a singleObject in the pushedTypes, we can check directly if it's the same object as was seen in the baseline cache, and if so, we don't need any other TI, and we use a direct shape guard (pending some patches that I just reviewed for bhackett). Since the change happened first, the shape is stable, and we get to this. So, where does that leave us? I think in general, we need to handle the null case directly. That's just silly. In terms of the function case (which doesn't fail for me because the function isn't tenured), didn't we add this callback exactly because TypeObject didn't imply class? But now that it does, we should be able to come up with a cleaner solution, right?

> but I also don't see why jitcode would be invalidated. I wouldn't expect jitcode to be invalidated. I would have expected the object that we set __proto__ on to have unknown type so we don't do the DOM optimizations on it. It sounds like for the specific case of globals (?) we don't actually rely on TI here? > But now that it does, we should be able to come up with a cleaner solution, right? Yes, that's what comment 4 and the patch in comment 5 are about. Implying class wasn't enough on its own when I last looked, because we needed the DOMClass, but now that each DOM proxy has a distinct class and they're all DOMJSClass we're totally in business. I also have a patch that keeps the callback instead of doing the shadow objects and just passes the curType->clasp() to the callback. Which of those would you prefer we do?

(In reply to Boris Zbarsky [:bz] from comment #7) > > but I also don't see why jitcode would be invalidated. > > I wouldn't expect jitcode to be invalidated. I would have expected the > object that we set __proto__ on to have unknown type so we don't do the DOM > optimizations on it. It sounds like for the specific case of globals (?) we > don't actually rely on TI here? > We don't rely on TI because the object is of singleton type, and it's an own-property getter, so we just emit the shape guard on the found proto (in this case the global). This could happen for any DOM object meeting that criteria, but it's way easier to find doing a global access like this, because they are explicitly own-property accesses on an object of singleton type. > > But now that it does, we should be able to come up with a cleaner solution, right? > > Yes, that's what comment 4 and the patch in comment 5 are about. Implying > class wasn't enough on its own when I last looked, because we needed the > DOMClass, but now that each DOM proxy has a distinct class and they're all > DOMJSClass we're totally in business. I also have a patch that keeps the > callback instead of doing the shadow objects and just passes the > curType->clasp() to the callback. Which of those would you prefer we do? I think I prefer keeping the callback. Traditionally, we have used callbacks to add browser-only functionality to the engine, where we do not need such things in the shell. This also means that we can talk to the bindings code directly in the callback, which I think is cleaner than doing shadow structs. The null check is trivial, and should have already been there, so that should go in testShouldDOMCall()

Comment on attachment 8423850 [details] [diff] [review] part 1. Stop using the DOMClass stored in DOM_PROTO_INSTANCE_CLASS_SLOT for doing type checks in the jit, and do them directly on the instance classes instead Review of attachment 8423850 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.cpp @@ +772,3 @@ > { > + const DOMClass& domClass = DOMJSClass::FromJSClass(clasp)->mClass; > + return (uint16_t)domClass.mInterfaceChain[depth] == protoID; static_cast please

Comment on attachment 8423850 [details] [diff] [review] part 1. Stop using the DOMClass stored in DOM_PROTO_INSTANCE_CLASS_SLOT for doing type checks in the jit, and do them directly on the instance classes instead Review of attachment 8423850 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! r=me ::: dom/bindings/BindingUtils.cpp @@ +772,3 @@ > { > + const DOMClass& domClass = DOMJSClass::FromJSClass(clasp)->mClass; > + return (uint16_t)domClass.mInterfaceChain[depth] == protoID; cast to uint16_t for comparison to uin32_t? Why?

Comment on attachment 8423851 [details] [diff] [review] part 2. Stop storing a DOMClass* in a slot on DOM prototypes Review of attachment 8423851 [details] [diff] [review]: ----------------------------------------------------------------- This also allows us to stop having a sub-struct for DOMClass, right? We should be able to just slurp those fields cleanly into DOMJSClass.

> cast to uint16_t for comparison to uin32_t? Why? Er, right, we changed the type in the jitinfo but the arguments here are still uint32_t. I'll fix the cast. > This also allows us to stop having a sub-struct for DOMClass, right? I think so, yes. I filed bug 1011660 on that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d52c012effd https://hg.mozilla.org/integration/mozilla-inbound/rev/a200f79a7b8f

https://hg.mozilla.org/mozilla-central/rev/5d52c012effd https://hg.mozilla.org/mozilla-central/rev/a200f79a7b8f

Reproduced in Nightly 2014-05-15. Verified fixed 32.0a1 (2014-05-27), win 7 x64.