Last Comment Bug 724310 - Drop cx argument from JSObject field and fixed slots infallible API
: Drop cx argument from JSObject field and fixed slots infallible API
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 675078
Blocks: 725595
  Show dependency treegraph
 
Reported: 2012-02-04 14:12 PST by Igor Bukanov
Modified: 2012-04-10 17:22 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (359.32 KB, patch)
2012-02-05 12:05 PST, Igor Bukanov
jwalden+bmo: review+
Details | Diff | Review

Description Igor Bukanov 2012-02-04 14:12:12 PST
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 2012-02-05 02:38:33 PST
<3
Comment 2 Igor Bukanov 2012-02-05 12:05:57 PST
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.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-08 16:37:55 PST
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 2012-02-09 12:18:42 PST
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 2012-02-09 12:47:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/41842e41890e - relanded with wixed bug reference
Comment 6 Ed Morley [:emorley] 2012-02-10 05:00:28 PST
https://hg.mozilla.org/mozilla-central/rev/41842e41890e
Comment 7 Igor Bukanov 2012-02-13 04:41:05 PST
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] 2012-04-10 17:22:05 PDT
(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.)

Note You need to log in before you can comment on or make changes to this bug.