724310 - Drop cx argument from JSObject field and fixed slots infallible API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

With single-threaded runtime we no longer need the cx parameter in infallible JS API that read/write fields and fixed slots of JSObject like:

JS_(Get|Set)Private
JS_GetPrototype
JS_(Get|Set)ReservedSlot

It would be nice to remove the cx argument from these so the callers can use them without having a cx instance with entered compartment or worry about errors.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

<3

Comment 2

[Igor Bukanov]

Attachment: v1

The patch removes the cx parameter from the API that manipulates fields and slots in JSObject and fixes up all the callers. The patch also propagates the cx removal from the callers when possible.

The only non-trivial part of the patch is the removal of js_(Set|Get)ReservedSlot, which now calls (Set|Get)ReservedSlot after doing the native check and calling GCPoke. Those things currently are  done in js_(Set|Get)ReservedSlot and I preserved them to minimize the semantic changes in 360K patch.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 594576 [details] [diff] [review]
v1

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

SkJSDisplayable.cpp is -- I think -- third-party Skia code.  Does it actually get built?  And should we be modifying it?  Find someone associated with the original checkin to review that change, if so.  Otherwise, those bits look fine.

It doesn't look like js::SetReservedSlot does anything that JS_SetReservedSlot doesn't.  Followup to change external users to the latter, and make the former internal and non-JS_FRIEND_API?

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ -1779,5 @@
>    if (PL_DHASH_ENTRY_IS_BUSY(entry) && entry->mJSObj) {
>      // Found a live NPObject wrapper, null out its JSObjects' private
>      // data.
>  
> -    JSContext *cx = GetJSContext(entry->mNpp);

Please investigate how else GetJSContext is used, and make sure it's not detritus now.

::: dom/workers/Events.cpp
@@ +237,4 @@
>    }
>  
> +  static jsval
> +  GetPropertyCommon(JSObject* aObj, int32 aSlot)

You could also replace this and calls to it with JS_GetReservedSlot, perhaps.

::: dom/workers/WorkerScope.cpp
@@ +821,5 @@
>  };
>  
>  WorkerGlobalScope*
>  WorkerGlobalScope::GetInstancePrivate(JSContext* aCx, JSObject* aObj,
>                                        const char* aFunctionName)

This function doesn't need to be passed JSContext* any more, does it?

::: js/src/ctypes/CTypes.cpp
@@ +739,5 @@
>    // to the appropriate CTypeProtoSlot. (SLOT_UINT64PROTO is the last slot
>    // of [[Class]] "CTypeProto" that we fill in this automated manner.)
>    for (uint32_t i = 0; i <= SLOT_UINT64PROTO; ++i) {
> +    JS_SetReservedSlot(proto, i, OBJECT_TO_JSVAL(protos[i]));
> +  }

Remove the braces now that this is single-line.

@@ +4206,1 @@
>    fields.forget();

Use |PRIVATE_TO_JSVAL(fields.forget())| and remove the then-unnecessary |fields.forget()|.  The previous ordering was only necessary under the assumption that JS_SetReservedSlot was fallible.

@@ +4968,1 @@
>    fninfo.forget();

Unify this get/forget sequence the same way as requested previously.

@@ +5376,1 @@
>    cinfo.forget();

Another get/forget pair to change.

::: js/src/jsapi.cpp
@@ -3178,5 @@
>  
>  JS_PUBLIC_API(void *)
> -JS_GetPrivate(JSContext *cx, JSObject *obj)
> -{
> -    /* This function can be called by a finalizer. */

Please leave this comment in all the methods you're touching in this file.  The simple lack of JSContext* in the API isn't nearly as explicit as a comment on what would otherwise be a very very new API assumption.  Explicit is better than implicit, as they say.

@@ +4408,5 @@
> +JS_PUBLIC_API(jsval)
> +JS_GetReservedSlot(JSObject *obj, uint32_t index)
> +{
> +    if (!obj->isNative())
> +        return UndefinedValue();

Couldn't we just assert that the object is native?  I mean, we assert that the slot's in-range and all, already, because we require the caller to know what he's doing.  It seems weird to be lenient about nativeness but not about slot-numbering.

But I guess if this is what the old code did, we should leave it this way -- for now.  Followup for this?

@@ +4417,5 @@
> +JS_PUBLIC_API(void)
> +JS_SetReservedSlot(JSObject *obj, uint32_t index, jsval v)
> +{
> +    if (!obj->isNative())
> +        return;

Same.

Comment 4

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/223f74353f63 - landed with the commit message that refers to the wrong bug 723517

Comment 5

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41842e41890e - relanded with wixed bug reference

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/41842e41890e

Comment 7

[Igor Bukanov]

We need to update the documentation that the following function no longer takes the cx argument.

JS_GetPrivate
JS_GetParent
JS_GetPrototype
JS_GetReservedSlot
JS_SetPrivate
JS_SetReservedSlot

Comment 8

[David Mandelin [:dmandelin]]

(In reply to Igor Bukanov from comment #7)
> We need to update the documentation that the following function no longer
> takes the cx argument.
> 
> JS_GetPrivate
> JS_GetParent
> JS_GetPrototype
> JS_GetReservedSlot
> JS_SetPrivate
> JS_SetReservedSlot

Edits made. Thanks for dropping in the note. (I saw it right away, just didn't get to it on my to-do list until today.)