Refactor prototype and class object logic to be Xray-friendly

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla29
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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.
This is sort of unrelated, part part of the patch queue and might as well land
now.
Attachment #8359012 - Flags: review?(jorendorff)
The current behavior is crazy, and most definitely not Xray-safe.
Attachment #8359014 - Flags: review?(jorendorff)
Small fixup.
Attachment #8359013 - Attachment is obsolete: true
Attachment #8359013 - Flags: review?(jorendorff)
Attachment #8359069 - Flags: review?(jorendorff)
Blocks: 959013
Still not quite right. We need to use the real/imaginary machinery here.
Attachment #8359069 - Attachment is obsolete: true
Attachment #8359069 - Flags: review?(jorendorff)
Attachment #8359075 - Flags: review?(jorendorff)
Attachment #8359012 - Flags: review?(jorendorff) → review+
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.
Attachment #8359075 - Flags: review?(jorendorff) → review+
Blocks: 962449
No longer blocks: 959013
Making the dependency hierarchy more linear.
No longer blocks: XrayToJS
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.
Attachment #8359014 - Flags: review?(jorendorff) → review+
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?
Attachment #8359015 - Flags: review?(jorendorff) → review+
(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
You need to log in before you can comment on or make changes to this bug.