Remove FindClassPrototype/FindClassObject

RESOLVED FIXED in mozilla40

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

When creating a new object (especially with JS_NewObject), you get the option to pass a nullptr as proto.

In that case we try to figure out the proto this new object should have. And because user-defined JSClasses usually don't have a cached proto we actually end up searching for it by looking up global[clasp->classname].prototype!
I believe this basically never works, because there is nothing with that classname usually.
This is insane in any case, we should either have a cached proto, an explicit proto or use Object.prototype.

BindingJSObjectCreator::CreateObject uses JS_NewObject, the proto for that seems to come from GetProtoObjectHandle, and it looks like that might be null. What is the behavior that DOM wants in that case?
There is an other use of JS_NewObject in BindingUtils, but that assert that it has a proto, so using JS_NewObjectWithGivenProto or JS_NewObjectWithUniqueType would probably work there.

XPConnect has a few instances of JS_NewObject that are not immediately obvious, but for the rest Object.prototype is probably ok.
> the proto for that seems to come from GetProtoObjectHandle, and it looks like that
> might be null

I see the following calls to BindingJSObjectCreator::CreateObject:

1)  From CGWrapWithCacheMethod.
2)  From CGWrapNonWrapperCacheMethod.

Both null-check the result of GetProtoObjectHandle and return false if null.  Null basically means some failure allocating the relevant DOM prototype object, so is fatal.  We can add an assert in BindingJSObjectCreator::CreateObject that proto is non-null, to make this clearer.  And using JS_NewObjectWithGivenProto totally makes sense here and in CreateInterfaceObject in BindingUtils.cpp.
This moves all the stuff closer together and makes it easier to understand.
The goal of this bug is to basically remove FindClassPrototype.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8554257 - Flags: review?(jwalden+bmo)
We have to be careful with JS_NewObjectWithGivenProto, it seems like the object gets marked as having unknown properties, that seems bad?
Comment on attachment 8554257 [details] [diff] [review]
v1 - Simplify FindClassPrototype/FindClassObject machinery

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

Look okay, although there's enough hair disentangled here it wouldn't surprise me to later find I missed a bug comparing pre/post-patch states of the world.

::: js/src/jsobj.cpp
@@ +1319,5 @@
>  
>      return obj;
>  }
>  
> +

Don't add this.

@@ +1332,5 @@
>      return JSProto_Null;
>  }
>  
> +static MOZ_ALWAYS_INLINE bool
> +NativeGetPureInline(NativeObject *pobj, Shape *shape, Value *vp)

Make vp a MutableHandleValue, since the only use is that.  And just make this inline, not REALLY REALLY INLINE.  It's only called one place so should generally be inlined anyway, and there's really not a good reason that it absolutely has to be inlined at all costs -- this is not obscenely obscenely perf-caring code.

@@ +1386,5 @@
> +    }
> +    return true;
> +}
> +
> +// Find the appropriate proto for a class, there are three different ways to achieve this:

Comma splice!  Make this two separate sentences.

@@ +1389,5 @@
> +
> +// Find the appropriate proto for a class, there are three different ways to achieve this:
> +// 1. Built-in classes have a cached proto and anonymous classes get Object.prototype.
> +// 2. Lookup global[clasp->name].prototype
> +// 3. Fallback to Object.prototype

Perhaps worth a comment that "Step 2 is in some circumstances an observable operation, which is probably wrong as a matter of specifications.  It's legacy garbage that we're working to remove eventually." or somesuch.

@@ +1390,5 @@
> +// Find the appropriate proto for a class, there are three different ways to achieve this:
> +// 1. Built-in classes have a cached proto and anonymous classes get Object.prototype.
> +// 2. Lookup global[clasp->name].prototype
> +// 3. Fallback to Object.prototype
> +static inline bool

Just static bool.  I doubt the "inline" here is at all respected, except to the extent that this method is called in only a single place and will be inlined there anyway.
Attachment #8554257 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace4047fc410

I am going to vote for just removing the "prototype" look-up. I changed all the JS_NewObject callers to something reasonable already. A try push that would crash if we found an object saved in prototype never did crash either, so this feature is totally unused.
Keywords: leave-open
Let's do this.
Attachment #8567056 - Flags: review?(jwalden+bmo)
Attachment #8567056 - Flags: feedback?(bzbarsky)
Comment on attachment 8567056 [details] [diff] [review]
v1 - Remove FindClassPrototype/FindClassObject

The new comment on NewObjectWithClassProtoCommon is .. uninformative.  As in, I now have no clue what proto the return value will have in what cases.

Past that, ripping out this gunk sounds great to me.
Attachment #8567056 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8567056 [details] [diff] [review]
v1 - Remove FindClassPrototype/FindClassObject

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

Stealing review.

I stumbled upon this bug after writing a virtually identical patch which you can see here:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc45b303e96d

::: js/src/jsobj.cpp
@@ +1279,5 @@
>      return obj;
>  }
>  
>  static JSProtoKey
> +ClassProtoKeyOrObject(const js::Class *clasp)

If you want, instead of renaming this function, you can delete it. There's only one call site, below.

Note that it never returns JSProto_Null, then read on...

@@ +1289,5 @@
>  }
>  
>  static bool
>  NewObjectWithClassProtoIsCachable(ExclusiveContext *cxArg, HandleObject parent,
>                                    JSProtoKey protoKey, NewObjectKind newKind, const Class *clasp)

You can delete the JSProtoKey argument, since the caller never passes JSProto_Null.

@@ +1316,5 @@
>      HandleObject parent = maybeParent ? maybeParent : GlobalObject::upcast(cxArg->global());
>  
> +    // For classes with a proto key, the prototype created during class
> +    // initialization is stored in an immutable slot on the global.
> +    JSProtoKey protoKey = ClassProtoKeyOrObject(clasp);

These three lines can be deleted.

@@ +1332,5 @@
>          }
>      }
>  
>      RootedObject proto(cxArg, protoArg);
> +    if (!GetBuiltinPrototype(cxArg, protoKey, &proto))

Here are the lines I added here:

+    /*
+     * Find the appropriate proto for clasp. Built-in classes have a cached
+     * proto on cx->global(); all others get %ObjectPrototype%.
+     */
+    JSProtoKey protoKey = JSCLASS_CACHED_PROTO_KEY(clasp);
+    if (protoKey == JSProto_Null)
+        protoKey = JSProto_Object;
Attachment #8567056 - Flags: review?(jwalden+bmo) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.