1010658 - Crash [@ mozilla::dom::InstanceClassHasProtoAtDepth] after changing window.__proto__ with WebIDL window bindings

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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.

Comment 1

[Jesse Ruderman]

Attachment: testcase that asserts before crashing

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

Comment 2

[Jesse Ruderman]

Attachment: stack

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: experiment

This is an example of the shadow struct approach.

Comment 6

[Eric Faust [:efaust]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 8

[Eric Faust [:efaust]]

(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 9

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop storing a DOMClass* in a slot on DOM prototypes

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 12

[Eric Faust [:efaust]]

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 13

[Eric Faust [:efaust]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Paul Silaghi, QA [:pauly]]

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