812392 - Non-leaf DOM prototypes seem to never have pure DOM object types for purposes of type inference

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For example, Node.prototype is the proto of Element.prototype.  So when we create Element.prototype we end up flagging the type for "things whose proto is Node.prototype" as "has non-DOM object".

Talked to bhackett about this, and we have a plan that goes like so:

1)  Fix JS_NewObjectWithUniqueType to create the object with a null proto, then flag the type as a singleton and only then splice in the right prototype.

2)  Fix JSObject::splicePrototype to not call getNewType() on the proto.

3)  Make sure all XPConnect prototypes are created with JS_NewObjectWithUniqueType.

Patch coming up that does these and seems to work over here to get us running on the Ion fast path for Node stuff.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure we don't allow descendant protoss to confuse whether a proto corresponds to a DOM type.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 682286 [details] [diff] [review]
Make sure we don't allow descendant protoss to confuse whether a proto corresponds to a DOM type.

Review of attachment 682286 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.cpp
@@ +120,3 @@
>      if (!obj || !JSObject::setSingletonType(cx, obj))
>          return NULL;
> +    if (!JS_SplicePrototype(cx, obj, proto))

This (or the NULL above) should get a short comment describing the purpose of this workaround.

Comment 3

[Boris Zbarsky [:bzbarsky]]

    /*
     * Create our object with a null proto and then splice in the correct proto
     * after we setSingletonType, so that we don't pollute the default
     * TypeObject attached to our proto with information about our object, since
     * we're not going to be using that TypeObject anyway.
     */

Look reasonable?

Comment 4

[Brian Hackett [Laid off!]]

Sure

Comment 5

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86a3edf2960d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Sorry, something in your push turned the mochitests orange. I had to back the entire thing out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/461a2225b7ba

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch to fix the orange

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Alternate additional patch to fix orange

This patch replaces the previous one.  I'm fine with either one, waldo preferred this one.  Let me know what you think.  All the other callers of this method already pass exactly the proto they want and don't rely on null == Object.prototype, as far as I can tell.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 684308 [details] [diff] [review]
Alternate additional patch to fix orange

Yeah, avoiding the kooky null-means-object.prototype behavior in JS_NewObject* and handling things in the API caller would be good.

Comment 10

[Boris Zbarsky [:bzbarsky]]

With the orange fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e49499b2e9c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e49499b2e9c