TypedArrays and ArrayBuffers both use a separate "slow class" for their prototypes. The name seems to be inherited from dense vs sparse arrays, and isn't particularly meaningful for these uses. This patch treats prototypes as the same type of object as an actual instance, when possible. So obj->isTypedArray/isArrayBuffer returns true for either a TypedArray or TypedArray.prototype. For typed arrays, you can detect something that points to actual data with obj->isFastTypedArray(). For ArrayBuffers, use hasData().

API consumers (e.g. DOM bindings) need to be able to tell apart typed arrays and their protos. I assume that's still possible after this change?

Comment on attachment 611121 [details] [diff] [review] Make TypedArrays and ArrayBuffers use a common fast/slow class mechanism Cancelling this review until I update for the final version of the API.

From private email, quoted without permission, for which I fully expect to fall afoul of our new Code of Conduct overlords and be whipped with poorly-cooked spaghetti: > I need a way to tell apart a typed array instance and the proto > for the typed array in question. As in, if WebIDL has a function > taking Float32Array, then passing an actual array instance should work > but passing the proto should throw. Why do you need to make that distinction? Float32Array.prototype behaves as a zero-length typed array for most purposes. I fully intend for there to be an "official" way of distinguishing these anyway, but it would help me to know why you need to make the distinction. Especially once we have transferrable ArrayBuffers (or more specifically, ArrayBuffers that can be neutered). The API after this patch would be JS_Get(Type)ArrayData == NULL. That covers both prototypes and neutered typed arrays (ie, array buffer views of neutered array buffers, but it turns out that array buffers will have to notify all of their views when they get neutered, so in fact the views themselves will know they are neutered.) Internally, you'd check for just the prototype case with obj->isFastTypedArray(). I can certainly expose that through a friend API, though I hate the "fast" part of the name so it'd be called something different. I'd like to know what it'd be used for, though. > At least as WebIDL and typed arrays are currently specced. Can you clarify this? The only typed array spec I am aware of is the WebIDL spec at http://www.khronos.org/registry/typedarray/specs/latest/ It says nothing at all about prototype behavior, and as I understand it, WebIDL never does. So what part of the spec are you referring to? Or are you just saying that DOM needs something more than what is in the spec? Somewhere else, maybe on IRC?, you mentioned that our implementation doesn't follow the spec. What were you referring to? I can't think of any discrepancies that I'm aware of, but there is a large amount of unspecified territory (and we differ from v8 in that territory).

> Why do you need to make that distinction? Because the spec requires me to as far as I can tell. Yes, this is a bit of an edge case and we could probably get it wrong with no one noticing... until a test suite is written. Which we would then fail. > Float32Array.prototype behaves as a zero-length typed array for most purposes. Not per spec. Per spec, for example, Float32Array.prototype.length should throw. Also, String(Float32Array.prototype) needs to be different than String(some Float32Array instance). And various other differences... > http://www.khronos.org/registry/typedarray/specs/latest/ Yes, indeed. > as I understand it, WebIDL never does. You understand wrong. WebIDL defines prototype behavior in some detail. See http://dev.w3.org/2006/webapi/WebIDL/#interface-prototype-object for the description of the prototype object itself. This links to the definitions for the methods and properties that hang off the prototype. So as a simple example, consider the "length" getter on Float32Array.prototype. This is defined in http://dev.w3.org/2006/webapi/WebIDL/#es-attributes and says: If O is not a platform object that implements the interface on which the attribute was declared, then: If the attribute was specified with the [LenientThis] extended attribute, then return undefined. Otherwise, throw a TypeError. I guess the spec never clearly says whether the prototype object for an interface can be a platform object that implements that interface. Cameron, is that actually allowed? It certainly makes no sense for the vast majority of interfaces, but if we want to claim that's what's going on here.... > What were you referring to? Well, most simply Object.getOwnPropertyDescriptor(Float32Array.prototype, "length") returns a value descriptor with value undefined, not an accessor descriptor as the spec requires. It's not as bad as I thought, actually, since at least the .length actually lives on the prototype, not on individual arrays; I thought we had done it more like Array.

> I guess the spec never clearly says whether the prototype object for an interface > can be a platform object that implements that interface. Cameron, is that actually > allowed? No, it's not. It might not be clear in the spec, but at least the "Platform objects implementing interfaces" section say that the [[Prototype]] of such objects must be the interface prototype object of its primary interface (i.e. for a Float32Array object its [[Prototype]] must be Float32Array.prototype), so that does rule out Float32Array.prototype from being considered a platform object that implements the Float32Array interface.

I am converting this bug to do pretty much the opposite of what it was originally -- now, instead of making prototypes act like the platform objects, I'm adapting them to follow webidl and be separate (and throw TypeErrors when they're supposed to according to webidl.)

Looks like assignee got cleared accidentally.

Comment on attachment 617970 [details] [diff] [review] Obey the webidl overlords Review of attachment 617970 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsclone.cpp @@ +734,3 @@ > switch (tag) { > case SCTAG_TYPED_ARRAY_INT8: > + return in.readArray((uint8_t *) JS_GetInt8ArrayData(obj, context()), nelems); Get rid of the unnecessary casts on all of these. ::: js/src/jstypedarray.cpp @@ +484,5 @@ > { > + if (!(obj = getArrayBuffer(obj))) { > + JSAutoByteString bs(cx, name); > + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, > + JSMSG_INCOMPATIBLE_PROTO, "ArrayBuffer", bs.ptr(), "object"); This has to be swallowing some sort of error if JSAutoByteString OOMs, or whatever -- if (JSAutoByteString bs(cx, name)) or something here before doing the error-reporting, to fix that. @@ +750,2 @@ > obj = obj->getProto(); > return obj; This method shouldn't be passed a non-null obj, should it? Let's do this instead, so that problem can't creep its way in: do { if (obj->isTypedArray()) return obj; } while ((obj = obj->getProto())); return NULL; ::: js/src/jstypedarray.h @@ +288,5 @@ > +inline bool > +IsTypedArrayClass(const Class *clasp) > +{ > + return &TypedArray::fastClasses[0] <= clasp && > + clasp < &TypedArray::fastClasses[TypedArray::TYPE_MAX]; Make the field name just |classes|, not |fastClasses|. ::: js/src/methodjit/PolyIC.cpp @@ +2322,4 @@ > if (!masm.supportsFloatingPoint() && > + (TypedArray::getType(obj) == js::TypedArray::TYPE_FLOAT32 || > + TypedArray::getType(obj) == js::TypedArray::TYPE_FLOAT64 || > + TypedArray::getType(obj) == js::TypedArray::TYPE_UINT32)) Get rid of the js:: prefixes while you're here. @@ +2635,3 @@ > if (!masm.supportsFloatingPoint() && > + (TypedArray::getType(obj) == js::TypedArray::TYPE_FLOAT32 || > + TypedArray::getType(obj) == js::TypedArray::TYPE_FLOAT64)) And here. ::: js/src/tests/js1_8_5/extensions/typedarray.js @@ -80,5 @@ > check(function() thrown, todo); > } > > - check(function() ArrayBuffer.prototype.byteLength == 0); > - This should check for the appropriate error being thrown. @@ +263,5 @@ > checkThrows(function() a.set([1,2,3], 2147483647)); > > a.set(ArrayBuffer.prototype); > + checkThrows(function () a.set(Int16Array.prototype)); > + checkThrows(function () a.set(Int32Array.prototype)); These should check for the appropriate error being thrown, not just for an error being thrown. @@ +323,5 @@ > + checkThrows(function() Int32Array.prototype.byteLength); > + checkThrows(function() Int32Array.prototype.byteOffset); > + checkThrows(function() Float64Array.prototype.length); > + checkThrows(function() Float64Array.prototype.byteLength); > + checkThrows(function() Float64Array.prototype.byteOffset); Check for the appropriate error. @@ +333,5 @@ > + check(function() Float64Array.prototype.length = true); > + check(function() Int32Array.prototype.byteLength = true); > + check(function() Float64Array.prototype.byteLength = true); > + check(function() Int32Array.prototype.byteOffset = true); > + check(function() Float64Array.prototype.byteOffset = true); Check for the appropriate error. @@ +348,5 @@ > + // Except this fails in getOwnPropertyDescriptor, I think because > + // Int32Array.prototype does not provide a lookup hook, and the fallback > + // case ends up calling the getter. Which seems odd to me, but much of this > + // stuff baffles me. It does seem strange that there's no way to do > + // getOwnPropertyDescriptor on any of these attributes. Yeah, we need to fix this and make these JSPropertySpec JSPROP_ACCESSOR sometime. Although I hope we can change the spec to just make these data properties, instead, before then...