1125356 - Introduce JS_NewPlainObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

I want to remove all uses of JS_NewObject like "JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr());" and replace them with JS_NewPlainObject.

The end goal here is to always have a proto available when creating a new object.

Comment 1

[Tom S [:evilpie]]

Attachment: v1 - Use NewPlainObject in JS

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8553988 [details] [diff] [review]
v1 - Use NewPlainObject in JS

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

::: js/src/jsobj.cpp
@@ +2706,5 @@
>  
>  bool
>  js::FindClassObject(ExclusiveContext *cx, MutableHandleObject protop, const Class *clasp)
>  {
> +    MOZ_CRASH("should not be invoked");

Ignore this :)

Comment 3

[Tom S [:evilpie]]

Attachment: v2 - Use NewPlainObject in JS

So, I think this patch doesn't have random other stuff in it.

Comment 4

[Tom S [:evilpie]]

Attachment: v1 - Use NewPlainObject in the browser

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554021 [details] [diff] [review]
v2 - Use NewPlainObject in JS

Why NewObjectWithClassProto instead of just:

  return js::NewObjectWithGivenProto(cx, &PlainObject::class_,
                                     cx->global()->getOrCreateObjectPrototype(),
                                     cx->global());

seems like that should do less work than NewObjectWithClassProto has to do (all that icky FindClassPrototype junk).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554050 [details] [diff] [review]
v1 - Use NewPlainObject in the browser

>+  JS::Rooted<JSObject*> components(cx, JS_NewPlainObject(cx)); // XXX Pretty sure this ok? cx and global have the same compartment == same global?

Looks like cx and global do indeed end up same-compartment here, based on code inspection.  Please add a MOZ_ASSERT to that effect?

This looks awesome, r=me

(Though, really, if you wanted to add a few void***** arguments to JS_NewPlainObject so people kept having to pass in all those nullptr, we could keep the old ergonomics, right?  ;) )

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8554021 [details] [diff] [review]
v2 - Use NewPlainObject in JS

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

Great stuff.  We should add more of these APIs that abstract away all the legacy garbage in our current APIs, and will over time gradually supplant them.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Why NewObjectWithClassProto instead of just:
> 
>   return js::NewObjectWithGivenProto(cx, &PlainObject::class_,
>                                     
> cx->global()->getOrCreateObjectPrototype(),
>                                      cx->global());
> 
> seems like that should do less work than NewObjectWithClassProto has to do
> (all that icky FindClassPrototype junk).

Fair points, I was just looking for equivalence when reviewing just now.  Do make sure to null-check the getOrCreateObjectPrototype call, tho, and store it into a Rooted through the final object's creation.

Comment 9

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d0ebd5f0f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff86ca36785b

Comment 10

[Boris Zbarsky [:bzbarsky]]

Unfortunately, NewBuiltinClassInstance ends up calling NewObjectWithClassProto, no?  So it's taking the slow path...

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09d0ebd5f0f4
https://hg.mozilla.org/mozilla-central/rev/ff86ca36785b

Comment 12

[Tom S [:evilpie]]

I am not sure why you think this is the "slow" path. We will just lookup the cached prototype for JSProto_Object. So instead of duplicating this everywhere we let the function basically the same thing.

After bug 1125567 this is going to look a lot simpler, which makes this more obvious and has less jumping around in unnecessary functions.

Comment 13

[Boris Zbarsky [:bzbarsky]]

It's the slow path because it involves a bunch of extra OOL function calls.  If bug 1125567 helps, great, but I'd like us to compare the two codepaths once that's done...

Comment 14

[Florian Scholz (Open Web Docs)]

Tom, can you tech review https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewPlainObject and mark this as dev-doc-complete?

Comment 15

[Tom S [:evilpie]]

Looks good, thanks Florian!