1127443 - Remove proto parameter from JS_NewObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

This parameter is kind of evil. If you pass null we search for the right proto, but otherwise it behaves just like calling JS_NewObjectWithGivenProto.

Comment 1

[Tom S [:evilpie]]

Attachment: Move some JS_NewObject callers to JS_NewObjectWithGiveProto

Moves callers that use the JS_NewObject proto argument to JS_NewObjectWithGivenProto

Comment 2

[Tom S [:evilpie]]

Attachment: Remove JS_NewObject proto argument.

Comment 3

[Tom S [:evilpie]]

Attachment: Don't mark objects created by JS_NewObjectWithGivenProto as having unknown properties.

If you passed a non-null object as proto to JS_NewObject, we would already omit this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8556590 [details] [diff] [review]
Move some JS_NewObject callers to JS_NewObjectWithGiveProto

Why the behavior change in InitTearOffJSObject?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I assume the "don't mark" changeset is landing first, by the way?

Comment 6

[Tom S [:evilpie]]

Using JS_GetObjectPrototype as the proto, is the same as using JS::NullPtr(). Just did this change here so I could simplify in the next bug.

I was going to land all the patches in one go, but I guess I can land that first.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8556593 [details] [diff] [review]
Remove JS_NewObject proto argument.

Mind filing a bout on the GlobalResolve bit?  I'm 99% sure "obj" there must be cx->global() so we can drop that parent arg.

> xpc::NewOutObject(JSContext* cx, JSObject* scope)

Pretty sure that scope and cx are same-compartment here, so we could just drop the parent arg to JS_NewObject here as well, as well as dropping the "scope" argument to NewOutObject.  File a followup on that?

r=me on the changes outside js/src

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8556590 [details] [diff] [review]
Move some JS_NewObject callers to JS_NewObjectWithGiveProto

Oh, indeed, JS_GetObjectPrototype, not JS_GetPrototype.  Silly me!

r=me on the non-js/src bits

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8556596 [details] [diff] [review]
Don't mark objects created by JS_NewObjectWithGivenProto as having unknown properties.

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

Thanks, didn't know we had that call.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8556593 [details] [diff] [review]
Remove JS_NewObject proto argument.

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +463,5 @@
>  
>      RootedObject obj(cx);
>      {
>          JSAutoCompartment ac(cx, scope);
> +        obj = JS_NewObject(cx, nullptr, scope);

This can be JS_NewPlainObject, right?

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1466,5 @@
>  JSObject*
>  xpc::NewOutObject(JSContext* cx, JSObject* scope)
>  {
>      RootedObject global(cx, JS_GetGlobalForObject(cx, scope));
> +    return JS_NewObject(cx, nullptr, global);

And here.

Comment 11

[Tom S [:evilpie]]

I already opened a bug to look at all the parent arguments.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4b7be30f7d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87f2bd784f41
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f806794d466

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c4b7be30f7d
https://hg.mozilla.org/mozilla-central/rev/87f2bd784f41
https://hg.mozilla.org/mozilla-central/rev/3f806794d466

Comment 13

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Tom, can you update the documentation according to the modification made in this bug?

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewObject

Comment 14

[Tom S [:evilpie]]

Thanks for reminding me. The documentation here is already super confusing, but I tried to do some small cleanup.