Figure out a plan for rooting the TypedArray structs

RESOLVED FIXED in mozilla26



5 years ago
5 years ago


(Reporter: bz, Assigned: bz)


Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)



(3 attachments)

The basic problem is that the TypedArray structs have a JSObject* member.  That's obviously no good.

Some options I've considered:

1)  Change the member to a Rooted<JSObject*>.  Verify that this does not
    significantly regress WebGL uniform setters (I suspect it won't) and that we
    guarantee LIFO ordering for TypedArray destructors stuff, even with all the
    Maybe games we play with it, both on the stack and in unions (I _think_ we
    do this in practice, but proving it is a bit hard; if we decide to go with
    this approach I'll need to spend some more time on it).  If we ever start
    allowing typed array structs inside dictionaries we may be in trouble,
    though, and it's even worse for sequences because those destroy in FIFO

2)  Change the member to a Handle<JSObject*>, and require callers to pass in a
    useful Handle that won't die before the TypedArray object dies.  In
    practice, I don't believe current callers can easily do that short of
    setting up a Rooted on the stack somewhere... and in the case of unions or,
    in the future, dictionaries or sequences.

3)  Change the member to a Handle<Value> and require callers to pass in a
    useful Handle that won't die before the TypedArray object dies.   I'm pretty
    sure that we'll be ok on the lifetime issue in bindings code for typed array
    arguments (where we can just use the handle to the argument value), and we
    already disallow typed arrays inside dictionaries or sequences precisely
    because of rooting issues, so this would work.  If we ever have to allow
    typed arrays inside dictionaries or sequences, though, this would blow up.

4)  Store a JSObject* member and explicitly root it with JS_AddRoot stuff.  The
    chance of this not regressing the WebGL stuff is low.

5)  Use different idl types, or some sort of IDL annotations, for cases when we
    want the C++ callee to have access to the JSObject* and not just the
    length+data.  Use different C++ types for the two cases and use approach #4
    for the case when we need the object.

Any other bright ideas?  I'm not a huge fan of any of the ones I've got there so far....
How about combining an AutoRooter and Maybe, something like:

struct RootedTypedArray : public CustomAutoRooter {
  TypedArray ta; // Current one, with JSObject* member
  virtual void trace(JSTracer *trc) {
    if (ta.obj)
      traceObject(trc, &ta.obj, "TypedArray");
  RootedTypedArray(JSContext *cx) : CustomAutoRooter(cx) {
    ta.obj = NULL;
  bool construct(JSObject *obj) {
    ta.obj = JS_GetObjectAsNybble12Array(obj, &ta.len, &;
    return ta.obj;
  TypedArray* get() {
    return &ta;

then be sure to always pass TypedArrays around by pointer, not value? Or maybe

  typedef TypedArray *TypedArrayHandle;

and pass TypedArrayHandle to callees? :-)
How is that qualitatively different from solution #1 from comment 0?  Seems like it has all the same LIFO ordering requirements; the only difference is that it would maybe work for rooting the object even if the RootedTypedArray is on the heap, as long as the LIFO ordering was guaranteed.  But we don't put these structs on the heap anyway, so it's not much benefit at the moment to allow that... and the only way we would in the future is via sequences but those already violate the LIFO requirement.
#1 is talking about using Rooted and Maybe<Rooted>, but Maybe<Rooted> is a problem from the LIFO perspective. So I was just factoring out the LIFO from the Maybe<> part -- as in, with my solution you would always incur the cost of appending to a linked list, but the trace hook wouldn't do anything in the cases where your Maybe<> was never constructed.

For collections, it would be best to have a single entry pushed onto the LIFO stack, and then have that entry's trace hook go over the gcpointers in whatever order is most convenient. That means the above RootedTypedArray isn't useful for collections. Right now, it seems like the easiest mechanism we provide for tacking things onto the LIFO stack is CustomAutoRooter. (Or rather, inheriting from AutoGCRooter and constructing it with a cx, but CustomAutoRooter is more convenient if you just want to route the trace call through a vtable.)

I think terrence is saying more or less the same thing over in bug 868715 comment 7. (Note that updatable collections will need barriering on updates, which I think terrence is probably typing up right now over in that bug.)
> So I was just factoring out the LIFO from the Maybe<> part

Won't work; your entire struct would need to be inside the Maybe<> in some of these cases, because that Maybe<> is actually inside a C union, so you really can't construct it until you know which of the disjoint types you are (it's also not a Maybe<>, but more of a Maybe<>-like).

Note that the TypedArray itself does not have any Maybe inside; it just sometimes lives inside one, like every single other binding thing....
Doesn't that just mean you need to lift the Maybe-thing up higher? As in, it needs to be on something unconditionally present (not in a union), and the trace() call just has to decide which union fork is live and pull out the right pointer to mark it.
There is is nothing unconditionally present, pretty much.
So my current plan is a modified version of comment 1.  Specifically:

1)  Create Rooted* variants of all the typed array structs that inherit from the actual
    structs and also inherit from CustomAutoRooter and trace the object.
2)  Use these on the stack in bindings and C++ code that currently does TypedArray
    things.  Rely on the rooting analysis to catch cases that don't, or something.
3)  Require that TypedArray structs can only be passed by reference, so that we can't
    end up accidentally making a copy of the JSObject*
4)  Use the actual TypedArray, not the subclass, inside of sequences and dictionaries,
    and use their existing CustomAutoRooters (from bug 868715) to trace the object.
I guess we can't easily use TypedArray inside sequence/dictionary anyway for now, since there is no no-arg constructor...
That plan runs right into the fact that we already play holder games with typed arrays to make nullable ones look like pointers.

Peter, how do you feel about me making nullable typed arrays into Nullable<Uint8Array> and whatnot?  That way I can just use a rooting holder and be done with it...  That used to be hard when we couldn't pass constructor arguments to a decltype, but now we can.
Flags: needinfo?(peterv)
Any progress here? I'm looking to implement an interface that has a Uint8Array readonly attribute, I would prefer to use WebIDL rather than doing it the old fashioned way if possible.
There is no problem with using a Uint8Array readonly attribute, last I checked.  This bug is about writable attributes and methods taking typed arrays, and is only relevant in the exact rooting world anyway (well, unless you want typed arrays in sequences or dictionaries or something).
Ah yes, the error I'm seeing is in fact because I have a Uint8Array member of a dictionary that's used an optional init constructor parameter. Testcase: 

[Constructor(optional FooEventInit fooInitDict)]
interface FooEvent : Event {

dictionary FooEventInit : EventInit {
  Uint8Array someArray;

Leading to this build error:

TypeError: Can't handle member arraybuffers or arraybuffer views because making sure all the objects are properly rooted is hard
Chrise, I spun off bug 900898 on doing comment 7 item 4.  That should be good enough to get you going, and it doesn't have the problem I described in comment 9.
So bug 900898 incidentally handled both the issue from comment 8 and the one from comment 9.  I should really get a move on this bug.
Flags: needinfo?(bzbarsky)
Notes to self:

1)  We want to use a holderType with Optional: otherwise callees see types like Optional<RootedTypedArray<Uint32Array> >, which is annoying.

2)  We can't use a holder with unions, so we want to use the RootedTypedArray declType there.

3)  We need to teach unions to pass cx to the RootedTypedArray constructor.
Created attachment 796255 [details] [diff] [review]
part 1.  Introduce a RootedTypedArray class.
Attachment #796255 - Flags: review?(terrence)
Created attachment 796260 [details] [diff] [review]
part 2.  Disallow copy-construction of typed array structs, so people can't accidentally pass them by value and end up unrooted.
Attachment #796260 - Flags: review?(bugs)
Created attachment 796261 [details] [diff] [review]
part 3.  Use RootedTypedArray in codegen.
Attachment #796261 - Flags: review?(bugs)
Flags: needinfo?(bzbarsky)
Comment on attachment 796255 [details] [diff] [review]
part 1.  Introduce a RootedTypedArray class.

Review of attachment 796255 [details] [diff] [review]:

That was probably much harder to write than it was to understand. Nice. r=me
Attachment #796255 - Flags: review?(terrence) → review+
Comment on attachment 796255 [details] [diff] [review]
part 1.  Introduce a RootedTypedArray class.

Review of attachment 796255 [details] [diff] [review]:

Please put the stars to the left :)
Comment on attachment 796261 [details] [diff] [review]
part 3.  Use RootedTypedArray in codegen.

>+    template <typename T1>
>+    T& SetValue(const T1 &t1)
>+    {
>+      new (storage.addr()) T(t1);
>+      return *storage.addr();
>+    }
return *(new (storage.addr()) T(t1));

>     if type.isSpiderMonkeyInterface():
>         assert not isEnforceRange and not isClamp
>         name =
>-        declType = CGGeneric(name)
>+        arrayType = CGGeneric(name)
>+        declType = arrayType
>         if type.nullable():
>             declType = CGTemplatedType("Nullable", declType)
>             objRef = "${declName}.SetValue()"
>         else:
>             objRef = "${declName}"
>         template = (
>             "if (!%s.Init(&${val}.toObject())) {\n"
>             "%s" # No newline here because onFailureBadType() handles that
>             "}\n" %
>             (objRef,
>              CGIndenter(onFailureBadType(failureCode,
>         template = wrapObjectTemplate(template, type, "${declName}.SetNull()",
>                                       failureCode)
>+        if not isMember:
>+            # This is a bit annoying.  In a union we don't want to have a
>+            # holder, since unions don't support that.  But if we're optional we
>+            # want to have a holder, so that the callee doesn't see
>+            # Optional<RootedTypedArray<ArrayType> >.  So do a holder if we're
>+            # optional and use a RootedTypedArray otherwise.
>+            if isOptional:
>+                holderType = CGTemplatedType("TypedArrayRooter", arrayType)
>+                # If our typed array is nullable, this will set the Nullable to
>+                # be not-null, but that's ok because we make an explicit
>+                # SetNull() call on it as needed if our JS value is actually
>+                # null.  XXXbz Because "Maybe" takes const refs for constructor
>+                # arguments, we can't pass a reference here; have to pass a
>+                # pointer.
>+                holderArgs = "cx, &%s" % objRef
>+                declArgs = None
>+            else:
>+                holderType = None
>+                holderArgs = None
>+                declType = CGTemplatedType("RootedTypedArray", declType)
>+                declArgs = "cx"
>+        else:
>+            holderType = None
>+            holderArgs = None
>+            declArgs = None
>         return JSToNativeConversionInfo(template,
>                                         declType=declType,
>-                                        dealWithOptional=isOptional)
>+                                        holderType=holderType,
>+                                        dealWithOptional=isOptional,
>+                                        declArgs=declArgs,
>+                                        holderArgs=holderArgs)

Still trying to parse this all.
> Wouldn't 
> return *(new (storage.addr()) T(t1));
> work.

Good question.  I'm not actually sure, and in any case I modeled it on the 2-argument SetValue() for now...
Flags: needinfo?(peterv)
You need to log in before you can comment on or make changes to this bug.