Make a class to hold a rooted array of Values

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Posted patch make-auto-value-array-inline (obsolete) — Splinter Review
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.
Attachment #8367981 - Flags: feedback?(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.
Attachment #8367981 - Flags: feedback?(terrence) → feedback-
Duplicate of this bug: 788905

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

5 years ago
(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.
Assignee: nobody → jcoppeard
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.
OS: All → Mac OS X
Hardware: All → x86
Thanks, bugzilla!
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 6

5 years ago
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.
Attachment #8367981 - Attachment is obsolete: true
Attachment #8370200 - Flags: review?(terrence)
(Assignee)

Comment 7

5 years ago
Posted patch 2 - hide-auto-array-rooter (obsolete) — Splinter Review
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)
Attachment #8370203 - Flags: review?(terrence)
(Assignee)

Comment 8

5 years ago
Add HandleValueArray adaptor class which we can use to receive rooted arrays of values in the API.
Attachment #8370207 - Flags: review?(terrence)
(Assignee)

Comment 9

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

This will obviously require external review too.
Attachment #8370208 - Flags: review?(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
Attachment #8370200 - Flags: review?(terrence) → review+
Comment on attachment 8370203 [details] [diff] [review]
2 - hide-auto-array-rooter

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

A nice cleanup! r=me
Attachment #8370203 - Flags: review?(terrence) → review+
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.
Attachment #8370207 - Flags: review?(terrence) → review+
Comment on attachment 8370208 [details] [diff] [review]
4 - convert-call

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

Righteous! r=me
Attachment #8370208 - Flags: review?(terrence) → review+
(Assignee)

Comment 14

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

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

Requesting review for browser changes.
Attachment #8370203 - Flags: review?(bugs)
(Assignee)

Comment 15

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

Requesting review for browser changes.
Attachment #8370208 - Flags: review?(bugs)
Attachment #8370208 - Flags: review?(bugs) → review+
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
Attachment #8370203 - Flags: review?(bugs) → review-
Btw, I wish we could just remove SetProperty.  I think only nsGlobalWindow::DefineArgumentsProperty uses it.
(Assignee)

Comment 18

5 years ago
Posted patch 2 - hide-auto-array-rooter v2 (obsolete) — Splinter Review
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.
Attachment #8370203 - Attachment is obsolete: true
Attachment #8371643 - Flags: review?(bugs)
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.
Attachment #8371643 - Flags: review?(bugs) → review-
(Assignee)

Comment 20

5 years ago
Ok, here is the updated patch.
Attachment #8371643 - Attachment is obsolete: true
Attachment #8372794 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #8372794 - Attachment description: hide-auto-array-rooter → 2 - hide-auto-array-rooter v3
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.
Attachment #8372794 - Flags: review?(bugs) → review+
Depends on: 977234
Duplicate of this bug: 963918
You need to log in before you can comment on or make changes to this bug.