959012 - Refactor prototype and class object logic to be Xray-friendly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

These are some patches that I wrote to support my Xray-to-JS work in bug 914970, which I realized can land separately.

Green try run here: https://tbpl.mozilla.org/?tree=Try&rev=78e9952dcc8c

Uploading patches and flagging for review.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove unused cx parameter from JS_IdentifyClassPrototype. v1

This is sort of unrelated, part part of the patch queue and might as well land
now.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move js_GetClassObject creation logic into GlobalObject. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Create separate js_GetClassPrototype and js_FindClassPrototype to mimic js_{Get,Find}ClassObject. v1

The current behavior is crazy, and most definitely not Xray-safe.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Clean up and separate the semantics of js_{Get,Find}Class{Prototype,Object}. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move js_GetClassObject creation logic into GlobalObject. v2

Small fixup.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move js_GetClassObject creation logic into GlobalObject. v3

Still not quite right. We need to use the real/imaginary machinery here.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8359075 [details] [diff] [review]
Part 2 - Move js_GetClassObject creation logic into GlobalObject. v3

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

r=me with the one comment addressed.

::: js/src/jsobj.cpp
@@ +2985,5 @@
>  {
>      Rooted<GlobalObject*> global(cxArg, &obj->global());
>  
> +    // We can only resolve constructors with a real JSContext.
> +    if (!global->isStandardClassResolved(key) && cxArg->isJSContext()) {

The previous code behaved like this:

  * in debug buils, it asserted that cxArg->isJSContext().

  * in non-debug builds, it checked anyway, and returned false if
    !cxArg->isJSContext(), the case that shouldn't happen.

I think both behaviors should be preserved.

Comment 8

[Bobby Holley (:bholley)]

Making the dependency hierarchy more linear.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8359014 [details] [diff] [review]
Part 3 - Create separate js_GetClassPrototype and js_FindClassPrototype to mimic js_{Get,Find}ClassObject. v1

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

Sorry, I got massively distracted by multiple bits of craziness in here, all of it pre-existing.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8359015 [details] [diff] [review]
Part 4 - Clean up and separate the semantics of js_{Get,Find}Class{Prototype,Object}. v1

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

r=me, thanks.

::: js/src/jsiter.cpp
@@ +730,5 @@
>  {
>      JS_ASSERT(!JS_IsExceptionPending(cx));
> +    RootedObject ctor(cx);
> +    if (js_GetClassObject(cx, JSProto_StopIteration, &ctor) && ctor)
> +        cx->setPendingException(ObjectValue(*ctor));

Yeah. One of the abovementioned grossnesses was the wacky recursion inhibition in js_GetClassObject... good grief...

::: js/src/jsobj.cpp
@@ +3077,5 @@
>      if (shape && pobj->isNative()) {
>          if (shape->hasSlot()) {
>              v = pobj->nativeGetSlot(shape->slot());
>              if (v.get().isPrimitive())
>                  v.get().setUndefined();

This `if` is useless now, right?

@@ +5271,3 @@
>  {
> +    MOZ_ASSERT(clasp);
> +    JSProtoKey protoKey = GetClassProtoKey(clasp);

House style in js/src is not to assert a pointer is nonnull right before reading through it (since it's going to crash anyway).

@@ +5274,5 @@
>      if (protoKey != JSProto_Null) {
>          if (!js_GetClassPrototype(cx, protoKey, protop))
>              return false;
>          if (protop)
>              return true;

Ugh, there it is again.

@@ +5300,1 @@
>      }

The preexisting code could not return true without setting protop. Please restore that.

::: js/src/jsobj.h
@@ +1334,5 @@
>  
>  /*
> + * Property-lookup-based access to interface and prototype objects for classes.
> + * If the class is built-in (and has a non-null JSProtoKey), these forward to
> + * js_GetClass{Object,Prototype}.

Thanks for the comment.

::: js/src/vm/TypedArrayObject.cpp
@@ +3923,5 @@
>  JSObject *
>  js_InitTypedArrayClasses(JSContext *cx, HandleObject obj)
>  {
>      JS_ASSERT(obj->isNative());
> +    assertSameCompartment(cx, obj);

Isn't the assertion still valid?

Comment 11

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=41825ea3857b

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=41825ea3857b

Looks like I need to include part 1 of bug 962449 in this bug:

https://tbpl.mozilla.org/?tree=Try&rev=89e7229f34df

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/926eec8d0540
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9e35c9af6285
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b34275a6a75d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/185004086e8c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d236f70c2d4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/926eec8d0540
https://hg.mozilla.org/mozilla-central/rev/9e35c9af6285
https://hg.mozilla.org/mozilla-central/rev/b34275a6a75d
https://hg.mozilla.org/mozilla-central/rev/185004086e8c
https://hg.mozilla.org/mozilla-central/rev/8d236f70c2d4