789589 - Implement JS_NewDataView

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

It always takes an ArrayBuffer.

Note that this will require one of (1) not supporting DataViews on cross-compartment ArrayBuffers, or (2) a grotesque amount of code duplication with the typed array creation path, or (3) a massive refactoring to make an ArrayBufferViewObject base class descending from JSObject (which requires reparenting typed arrays).

#3 should really be done before the typed array code gets any hairier.

Comment 1

[Nicholas Nethercote [inactive]]

#3 was done in bug 889146.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> #3 was done in bug 889146.

Well, sort of. Or maybe it was, but then it got messed up again? At any rate, the nice helpful comment about the class hierarchy in ArrayBufferObject.h is a complete lie right now.

Also, I now think #3 is kind of a bad idea now that we have a whole dysfunctional family of ArrayBuffer views (typed arrays, DataViews, typed objects, with SharedArrayBuffers mixed in). ArrayBufferViewObject no longer fits very nicely into the class hierarchy, since it's being used for other stuff. Especially when considering typed objects.

Fortunately, I think that everything is in a good enough state for it to not matter anymore.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Implement JS_NewDataView

Turns out to be nearly trivial at this point, unless I'm missing something major.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Fix the ABO class hierarchy comment to be accurate

Update the ArrayBufferObject.h comment to reflect current reality, messy as it is.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8640253 [details] [diff] [review]
Implement JS_NewDataView

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

So in principle this is sort of fine, but there's a complete lack of consistency about how the arguments are interpreted for a bunch of this, and it's not documented.  That all needs fixing before this can go in.

::: js/src/jsapi-tests/testArrayBufferView.cpp
@@ +85,5 @@
>  
>  static JSObject*
>  CreateDataView(JSContext* cx)
>  {
> +    JS::Rooted<JSObject*> buffer(cx, JS_NewArrayBuffer(cx, 8));

CHECK(buffer);

::: js/src/jsfriendapi.h
@@ +2131,5 @@
> + * Create a new DataView using the given ArrayBuffer for storage.  The
> + * offset and length are optional.
> + */
> +JS_FRIEND_API(JSObject*)
> +JS_NewDataView(JSContext* cx, JS::HandleObject arrayBuffer, uint32_t byteOffset, int32_t length);

That's a curious definition of "optional".

What happens if the buffer isn't an ArrayBuffer or if it's null?  What happens if byteOffset is out of range?  What happens if length extends past the buffer bounds: is this an API misuse, is an exception thrown, is it clamped?  What's length specified in relation to?  uint8_t?  I'd assume so, but in that case it should be named byteLength.

This is generally woefully unclear about what's required of the byteOffset/length requirements.  Add more documentation, please.  It's perfectly fine to have "must be" requirements backed up only by assertions, but the ground rules have to be clear, even if the consequences for violating those rules are unspecified.

::: js/src/vm/TypedArrayObject.cpp
@@ +2353,5 @@
> +{
> +    if (!IsArrayBuffer(arrayBuffer))
> +        return nullptr;
> +    Rooted<ArrayBufferObject*> buffer(cx, &AsArrayBuffer(arrayBuffer));
> +    return DataViewObject::create(cx, byteOffset, length, buffer, nullptr);

This is return-value confusion: nullptr can mean *either* an API misuse, *or* it can mean OOM occurred or an exception was thrown.  Eliminate the former case, by asserting it or something else.

If you assert it, you'll also need to make the doc comment say the ArrayBuffer must be from the current compartment/global; I'd expect the current API to work for any ArrayBuffer from anywhere.

The create() method here expects a uint32_t length, not an int32_t.

If we're adding this path to call that method, we need to resolve the problem of what happens if length/offset are out of range.  Looking at the implementation, because that's the only way to know anything about this, I see that it assumes/asserts that the arguments are <= INT32_MAX, then it *relies* on this to assert that a sum of the arguments is less than the ArrayBuffer's length.  That seems a bit inconsistent.  Either we should consistently assert that all arguments are valid with respect to the ArrayBuffer, or we should be fully testing them.  Given this is the basic operation to create a DataView, I think making it only assert stuff seems right (especially as we have duplicative offset+length<=bufferLength assertions right now, woo woo).

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8640254 [details] [diff] [review]
Fix the ABO class hierarchy comment to be accurate

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

::: js/src/vm/ArrayBufferObject.h
@@ +36,5 @@
>  //       - SharedInt8ArrayObject
>  //       - SharedUint8ArrayObject
>  //       - ...
> +// - JSObject
> +//   - ArrayBufferViewObject

Interesting.  I didn't know this rejiggering occurred, or when it occurred.  *Why* did it occur?  I'm not sure what we're now doing is actually kosher; we have to be doing some sort of reinterpret_cast<> across class hierarchies, somewhere, to make this work, but that's almost certainly not safe under C++ semantics.  Why was this removed?  It seems a pretty dodgy removal to me.

The comment by ArrayBufferObject's definition is also out of date along these lines.

@@ +42,5 @@
>  //
>  // Note that |TypedArrayObjectTemplate| is just an implementation
>  // detail that makes implementing its various subclasses easier.
>  // Note that |TypedArrayObjectTemplate| and |SharedTypedArrayObjectTemplate| are
>  // just implementation details that make implementing their various subclasses easier.

Uh, these two comments are a bit naff, don't you think?  Remove the first one.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Implement JS_NewDataView

Ok, I think this is fixed up. In an unexpected way, since I eventually realized that the way to support cross-compartment wrappers is to go through the Invoke mechanism, which means JS_NewDataView inherits the runtime error checking from there.

I prefer to consider bad parameters to be JSAPI misuse, and documented it as such, but the actual behavior is now more like you're just eval'ing some JS code.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Comment on attachment 8640254 [details] [diff] [review]
> Fix the ABO class hierarchy comment to be accurate
> 
> Review of attachment 8640254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ArrayBufferObject.h
> @@ +36,5 @@
> >  //       - SharedInt8ArrayObject
> >  //       - SharedUint8ArrayObject
> >  //       - ...
> > +// - JSObject
> > +//   - ArrayBufferViewObject
> 
> Interesting.  I didn't know this rejiggering occurred, or when it occurred. 
> *Why* did it occur?  I'm not sure what we're now doing is actually kosher;
> we have to be doing some sort of reinterpret_cast<> across class
> hierarchies, somewhere, to make this work, but that's almost certainly not
> safe under C++ semantics.  Why was this removed?  It seems a pretty dodgy
> removal to me.

I should probably do archaeology to answer that question, but after spending 2 days trying to slot ABVO in as a true superclass and failing, I have an opinion. ABVO doesn't mean much of anything. We don't really have any subclasses that are guaranteed to have an ArrayBuffer at all, since just about everything sometimes inlines its data into the view object. Typed objects reflect that in their class hierarchy (*Inline vs *Outline subclasses). Typed arrays distinguish with internal state bits that can change dynamically. DataViews *could* inherit fully, but they'd be all alone.

So ABVO is being used as an interface, not a class. Here's what it has:

class ArrayBufferViewObject : public JSObject
{
    static ArrayBufferObject* bufferObject(JSContext* cx, Handle<ArrayBufferViewObject*> obj);
    void neuter(void* newData);
    uint8_t* dataPointer();
    void setDataPointer(uint8_t* data);
    static void trace(JSTracer* trc, JSObject* obj);
};

bufferObject works only for typed arrays and dataviews.

neuter does a case analysis to handle various types. So do dataPointer() and setDataPointer(), though the distinctions aren't quite the same as neuter's.

trace() does manual case analysis to figure out what to do for various types, using bufSlot.isObject() and buf.forInlineTypedObject() (the latter returns a boolean).

In short, ABVO is a place to pile common functionality for classes that sometimes do something with ArrayBuffers. Its main reason for existence seems to be the neutering stuff: if you transfer a typed array, you have to neuter its buffer, which means you have to neuter *all* views of that buffer: http://dxr.mozilla.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#533

It would make more sense to call it DetachableObject, perhaps. ABVO is a particularly bad name, because there's also a JS_IsArrayBufferViewObject that has an actual spec meaning. But then I'm not sure what to do with the trace() method. ArrayBufferAwareObject? I'm inclined to remove it from the Object class hierarchy. All of its methods could be static. ArrayBufferAwareOperations? Ugh, no. Maybe just move everything to static methods on ArrayBufferObject?

> 
> The comment by ArrayBufferObject's definition is also out of date along
> these lines.

Fixed, thanks.

> @@ +42,5 @@
> >  //
> >  // Note that |TypedArrayObjectTemplate| is just an implementation
> >  // detail that makes implementing its various subclasses easier.
> >  // Note that |TypedArrayObjectTemplate| and |SharedTypedArrayObjectTemplate| are
> >  // just implementation details that make implementing their various subclasses easier.
> 
> Uh, these two comments are a bit naff, don't you think?  Remove the first
> one.

Yup.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8646515 [details] [diff] [review]
Implement JS_NewDataView

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

Poke me on IRC about the rewritten construction code -- a pastebin of just that new method should go faster than an actual review.  r+ if you do that (can't imagine it'd be screwed up meaningfully, but given it's much of the patch, and a new interface only two people know anything about now, I probably should take a look just in case).

::: js/src/jsapi-tests/testArrayBufferView.cpp
@@ +136,5 @@
> +    JS::Rooted<JSObject*> buffer(cx);
> +    {
> +        AutoCompartment ac(cx, otherGlobal);
> +        if (!(buffer = JS_NewArrayBuffer(cx, 8)))
> +            return nullptr;

buffer = ...;
CHECK(buffer);

otherwise you'll have a returning-nullptr-in-bool-function warnerr.

@@ +141,5 @@
> +        CHECK(buffer->as<ArrayBufferObject>().byteLength() == 8);
> +    }
> +    CHECK(buffer->compartment() == otherGlobal->compartment());
> +    if (!JS_WrapObject(cx, &buffer))
> +        return false;

CHECK(JS_WrapObject(cx, &buffer));

Generally I think we just CHECK all fallible functions, when the failure is not by design of the test.  A few others here, too -- dataview and JS_SetProperty.

::: js/src/jsfriendapi.h
@@ +2131,5 @@
> + * Create a new DataView using the given ArrayBuffer for storage. The given
> + * buffer must be an ArrayBuffer (or a cross-compartment wrapper of an
> + * ArrayBuffer), and the offset and length must fit within the bounds of the
> + * arrayBuffer. Currently, nullptr will be returned and an exception will be
> + * thrown if these conditions do not hold, but do not depend on that behavior.

I'm actually not horribly put out by making the behavior "act like new DataView(arrayBuffer, byteOffset, byteLength) up to and including throwing an exception for bad arguments".  We can't have Handle<ABO> because of ABO being internal and because wrappers, so the chance of lapses seems to go high enough, IMO, to just specify in-terms-of.  But up to you.

::: js/src/vm/TypedArrayObject.cpp
@@ +2351,5 @@
> +JS_NewDataView(JSContext* cx, HandleObject arrayBuffer, uint32_t byteOffset, int32_t byteLength)
> +{
> +    InvokeArgs args(cx);
> +    if (!args.init(3, true))
> +        return nullptr;

I just redid the interface for performing construction calls, so this needs rewriting.  You want to use cx->global().resolveConstructor or whatever it's called to get the DataView function to use as callee, in that new interface.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1039c885317e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a69ece50cf

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1039c885317e
https://hg.mozilla.org/mozilla-central/rev/d2a69ece50cf