Drop cx argument from JSObject field and fixed slots infallible API

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({dev-doc-needed})

unspecified
mozilla13
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
<3
(Assignee)

Comment 2

5 years ago
Created attachment 594576 [details] [diff] [review]
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.
Assignee: general → igor
Attachment #594576 - Flags: review?(jwalden+bmo)
Summary: Drop cx argumrent from JSObject field and fixed slots infallible API → Drop cx argument from JSObject field and fixed slots infallible API
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.
Attachment #594576 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

5 years ago
Blocks: 725595
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/41842e41890e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Assignee)

Comment 7

5 years ago
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
(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.)
You need to log in before you can comment on or make changes to this bug.