965830 - Make a class to hold a rooted array of Values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: make-auto-value-array-inline

At the moment whenever we have an array of Values (e.g. to pass to Invoke()) we do something like this:

  Value argv[x] = {...};
  AutoValueArray ava(cx, argv, x);

This is an issue because if we forget the AutoValueArray then it's not safe, and even if we don't the analysis incorrectly warns it's not safe anyway.

Instead we can make a class that contains an array of Values and roots them, with a view to using this as the argument type in API functions that currently take Value* in the future.

Note, we could use AutoValueVector for small arrays (as vector will use inline storage for up to 8 values), but it's quite complex what with all possibility for dynamic growth that we don't need here.

Looking at the code we currently have two classes that root an external array of Values - AutoValueArray and AutoArrayRooter - with AutoValueArray being internal and AutoArrayRooter being public.

I'm proposing to re-purpose AutoValueArray for this task (and make it public), and leave AutoArrayRooter to root an external array as it does now.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8367981 [details] [diff] [review]
make-auto-value-array-inline

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

We discussed on IRC and decided that if we expose the inline size field of AutoValueVector we can move from exposing 3 ValueArray classes to 1.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #1)
So idea was to make the inline size of the vector a template argument, defaulted to the current value.  However after experimentation, I don't think this will work.

Doing this makes AutoValueVector a template, which means that we can't pass one as a function argument without templating the function on the inline size.  This means templating all the functions where pass these internally.  

Also if we are to put these in the API, we need a single type that we can use, not a template (this was a problem for my original solution too).  A way around this would be to use an implicitly constructed adaptor class that only allows creation from any type of rooted array.

Next, it seems we would still need to say AutoValueVector<> to declare these as you can't omit the <> even if all your template arguments are defaulted.

Finally, the import of JS::AutotValueVector into the js namespace no longer works, because using declarations don't work on templates.  

Mostly this just seems like C++ being annoying.  The only real solution I can see is to have a separate template class from AutoValueVector for where you want to specify the inline size, which is what we were trying to avoid.

Comment 4

[Terrence Cole [:terrence]]

Ouch! You are right: those are not really tractable problems. I guess we are going to need the wrapper even if we only expose one type.

Comment 5

[Terrence Cole [:terrence]]

Thanks, bugzilla!

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 1 - add-auto-value-array

Change AutoValueArray to be a fixed size stack based array that roots its contents.  Move it to the public API and update internal uses of it.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 2 - hide-auto-array-rooter

Remove AutoArrayRooter from the public API and make it internal.  Update browser code to use another form of rooting.

(I'll request additional review here at a later point)

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 3 - add-handle-value-array

Add HandleValueArray adaptor class which we can use to receive rooted arrays of values in the API.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: 4 - convert-call

Finally, convert JS_CallFunction() and related functions to use HandleArrayValue and update callers.

This will obviously require external review too.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8370200 [details] [diff] [review]
1 - add-auto-value-array

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

I'm surprised there are so many sites internally. Shame we can't use C++11 initializer lists. r=me

::: js/src/gc/RootMarking.cpp
@@ +465,5 @@
>          return;
>        }
>  
>        case VALARRAY: {
> +        AutoValueArray<1> *array = static_cast<AutoValueArray<1> *>(this);

Maybe add a note that the template parameter does not matter here because we use length.

::: js/src/jsapi.h
@@ +195,5 @@
> +/* AutoValueArray roots an internal fixed-size array of Values. */
> +template <size_t N>
> +class AutoValueArray : public AutoGCRooter
> +{
> +    size_t length_;

With the current construction, I think we could make length_ const.

@@ +204,5 @@
> +    AutoValueArray(JSContext *cx
> +                   MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +      : AutoGCRooter(cx, VALARRAY), length_(N), skip_(cx, elements_, N)
> +    {
> +        /* Alwasys initialize in case we GC before assignment. */

Always

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8370203 [details] [diff] [review]
2 - hide-auto-array-rooter

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

A nice cleanup! r=me

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 8370207 [details] [diff] [review]
3 - add-handle-value-array

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

r=me

::: js/src/jsapi.h
@@ +648,5 @@
>  
> +/* A handle to an array of rooted values. */
> +class HandleValueArray
> +{
> +    size_t length_;

I guess this could be const.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8370208 [details] [diff] [review]
4 - convert-call

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

Righteous! r=me

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 8370203 [details] [diff] [review]
2 - hide-auto-array-rooter

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

Requesting review for browser changes.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8370208 [details] [diff] [review]
4 - convert-call

Requesting review for browser changes.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8370203 [details] [diff] [review]
2 - hide-auto-array-rooter

>   obj = thisContent->GetWrapper();
>   // Now wrap things up into the compartment of "obj"
>   JSAutoCompartment ac(aCx, obj);
>-  nsTArray<JS::Value> args(aArguments);
>-  JS::AutoArrayRooter rooter(aCx, args.Length(), args.Elements());
>-  for (size_t i = 0; i < args.Length(); i++) {
>-    if (!JS_WrapValue(aCx, rooter.handleAt(i))) {
>+  JS::AutoValueVector args(aCx);
>+  if (!args.resize(aArguments.Length())) {
>+    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>+    return JS::UndefinedValue();
>+  }
>+
>+  for (size_t i = 0; i < args.length(); i++) {
>+    args[i] = aArguments[i];
>+    if (!JS_WrapValue(aCx, args.handleAt(i))) {
How is this ok. JS_WrapValue may GC, so the next argument may be gone before it is set to args vector.
So please fix or explain why it is ok.


>+static nsresult
>+FinishSetProperty(JSContext *aCx, JS::Handle<JSObject*> aTarget, const char* aPropName,
>+                  uint32_t aArgc, JS::Value *aArgv)
* goes with the type

>+{
>+  for (uint32_t i = 0; i < aArgc; ++i) {
>+    // The contents of aArgv are marked either because they are on the heap as
>+    // part of an ns nsIJSArgArray or on the stack as an AutoValueVector.
>+    if (!JS_WrapValue(aCx, JS::MutableHandle<JS::Value>::fromMarkedLocation(&aArgv[i]))) {
>+      return NS_ERROR_FAILURE;
>+    }
>+  }
>+
>+  JSObject *args = ::JS_NewArrayObject(aCx, aArgc, aArgv);
ditto, and in few other places

> nsJSContext::SetProperty(JS::Handle<JSObject*> aTarget, const char* aPropName, nsISupports* aArgs)
> {
I don't like the changes to SetProperty. Couldn't you keep SetProperty consistent and just deal with some
vector there which ConvertSupportsTojsvals fills.
Or is there some particular reason for your approach.




>diff --git a/dom/base/nsJSEnvironment.h b/dom/base/nsJSEnvironment.h
>--- a/dom/base/nsJSEnvironment.h
>+++ b/dom/base/nsJSEnvironment.h
>@@ -17,16 +17,20 @@
> #include "nsThreadUtils.h"
> 
> class nsICycleCollectorListener;
> class nsIXPConnectJSObjectHolder;
> class nsRootedJSValueArray;
You want to remove also this line

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Btw, I wish we could just remove SetProperty.  I think only nsGlobalWindow::DefineArgumentsProperty uses it.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: 2 - hide-auto-array-rooter v2

Ok, thanks for the comments!  How is this?

> How is this ok. JS_WrapValue may GC, so the next argument may be gone before
> it is set to args vector.

Ugh, you're right, I messed this up.  

> I don't like the changes to SetProperty. Couldn't you keep SetProperty
> consistent and just deal with some
> vector there which ConvertSupportsTojsvals fills.
> Or is there some particular reason for your approach.

I wanted to get rid of the Maybe<> containing and AutoGCRooter, because these rooters must be destroyed in FIFO order and putting them in a Maybe makes this easy to get wrong.  But it's actually ok here anyway, and making tempStorage into a Maybe<AutoValueVector> is a simpler change.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8371643 [details] [diff] [review]
2 - hide-auto-array-rooter v2

> nsJSContext::SetProperty(JS::Handle<JSObject*> aTarget, const char* aPropName, nsISupports* aArgs)
> {
>   uint32_t  argc;
>   JS::Value *argv = nullptr;
> 
>   nsCxPusher pusher;
>   pusher.Push(mContext);
> 
>-  Maybe<nsRootedJSValueArray> tempStorage;
>+  Maybe<JS::AutoValueVector> tempStorage;
> 
>   JS::Rooted<JSObject*> global(mContext, GetWindowProxy());
>   nsresult rv =
>     ConvertSupportsTojsvals(aArgs, global, &argc, &argv, tempStorage);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  JS::AutoArrayRooter array(mContext, argc, argv);
>-
>   // got the arguments, now attach them.
> 
>+
extra newline


>   for (uint32_t i = 0; i < argc; ++i) {
>-    if (!JS_WrapValue(mContext, array.handleAt(i))) {
>+    // argv now points to traced heap data if aArgs is an nsIJSArgArray, or into
>+    // the rooted tempStorage vector.
>+    if (!JS_WrapValue(mContext,
>+                      JS::MutableHandle<JS::Value>::fromMarkedLocation(&argv[i]))) {
>       return NS_ERROR_FAILURE;
>     }
>   }
Hmm, since this isn't performance critical code, I would just always put the stuff to array.
That would keep this all easier to understand.

> nsJSContext::ConvertSupportsTojsvals(nsISupports *aArgs,
>                                      JS::Handle<JSObject*> aScope,
>                                      uint32_t *aArgc,
>                                      JS::Value **aArgv,
>-                                     Maybe<nsRootedJSValueArray> &aTempStorage)
>+                                     Maybe<JS::AutoValueVector> &aTempStorage)
> {
>   nsresult rv = NS_OK;
> 
>   // If the array implements nsIJSArgArray, just grab the values directly.
>   nsCOMPtr<nsIJSArgArray> fastArray = do_QueryInterface(aArgs);
>   if (fastArray != nullptr)
>     return fastArray->GetArgs(aArgc, reinterpret_cast<void **>(aArgv));
So you'd need to append this stuff to the vector, and given that you have now the append, that should be trivial.
No need for aArgc/aArgv params in that case.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: 2 - hide-auto-array-rooter v3

Ok, here is the updated patch.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8372794 [details] [diff] [review]
2 - hide-auto-array-rooter v3

>+  JSObject *array = ::JS_NewArrayObject(mContext, args.length(), args.begin());
* goes with the type

>+                                     JS::AutoValueVector &aArgsOut)
ditto

> {
>   nsresult rv = NS_OK;
> 
>-  // If the array implements nsIJSArgArray, just grab the values directly.
>+  // If the array implements nsIJSArgArray, copy the contents and return.
>   nsCOMPtr<nsIJSArgArray> fastArray = do_QueryInterface(aArgs);
>-  if (fastArray != nullptr)
>-    return fastArray->GetArgs(aArgc, reinterpret_cast<void **>(aArgv));
>+  if (fastArray != nullptr) {
while you're here, if (fastArray) {


>+    uint32_t argc;
>+    JS::Value *argv;
* goes with the type

>+    rv = fastArray->GetArgs(&argc, reinterpret_cast<void **>(&argv));
>+    if (NS_SUCCEEDED(rv) && !aArgsOut.append(argv, argc)) {
>+        rv = NS_ERROR_OUT_OF_MEMORY;
2 space indentation please.

Comment 22

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1357ca3147a
https://hg.mozilla.org/integration/mozilla-inbound/rev/44b82071c4f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee247f44a28b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4aa41ef381

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1357ca3147a
https://hg.mozilla.org/mozilla-central/rev/44b82071c4f3
https://hg.mozilla.org/mozilla-central/rev/ee247f44a28b
https://hg.mozilla.org/mozilla-central/rev/ba4aa41ef381