Closed Bug 1082141 Opened 10 years ago Closed 10 years ago

Typed objects are not ArrayBufferViews (yet)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(3 files)

Bug 1003997 made ArrayBuffer.isView return false for typed objects. Bug 1061404 accidentally reverted that change. The reasoning in bug 1003997 is still valid.
Blocks: 1061404
Attachment #8504272 - Flags: review?(jwalden+bmo)
Attachment #8504272 - Flags: review?(jwalden+bmo) → review+
...and quickly backed out because JSObject::as<T> asserts JSObject::is<T>, and as of (I think) bug 1061404 we have as<ArrayBufferViewObject>() coercions to consider as well.  (Both of us acked for "is<ArrayBuffer" and didn't think of this extra case, sigh.)

sfink is considering a larger class-hierarchy change to sub-distinguish ArrayBufferViewObject within the set of objects that happen to observe an ArrayBuffer.  A bit byzantine, but then this whole typed array/arraybuffer system is that.  Blah.
Ok, I'll admit I haven't stared closely at this patch, I just got it working. But given how typed objects are set up, I'm feeling like using the type system for things-that-hang-onto-ArrayBuffers is not really helping. So I killed ArrayBufferViewObject.
Attachment #8504382 - Flags: review?(jwalden+bmo)
Minimal change, for backporting.
Attachment #8504411 - Flags: review?(jwalden+bmo)
Comment on attachment 8504411 [details] [diff] [review]
Typed objects are not ArrayBufferViews (yet),

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +780,5 @@
>  ArrayBufferViewObject *
>  ArrayBufferObject::firstView()
>  {
>      return getSlot(FIRST_VIEW_SLOT).isObject()
> +        ? reinterpret_cast<ArrayBufferViewObject*>(&getSlot(FIRST_VIEW_SLOT).toObject())

I think static_cast<> will work here, no?

@@ +797,5 @@
>      // Note: we don't pass in an ArrayBufferViewObject as the argument due to
>      // tricky inheritance in the various view classes. View classes do not
>      // inherit from ArrayBufferViewObject so won't be upcast automatically.
> +    MOZ_ASSERT(viewArg->is<ArrayBufferViewObject>() || viewArg->is<TypedObject>());
> +    ArrayBufferViewObject *view = reinterpret_cast<ArrayBufferViewObject*>(viewArg);

static_cast<>

@@ +1212,5 @@
>      JSObject *obj = CheckedUnwrap(objArg);
>      if (!obj)
>          return nullptr;
> +    if (!obj->is<ArrayBufferViewObject>())
> +        return nullptr;

Don't add this if-check.  The API requires that the passed-in object have been vetted as an ABVO, and it's a user error if this is ever hit.  We don't want to paper over that.

In fact, the sole (non-test) caller of this method only ever passes in legitimate ArrayBufferViewObjects (specifically, webaudio code, passing in only Float32Array instances).  So this (combined with the API rule) means this'll never fail, and it's dead code.  Please remove it.

@@ +1214,5 @@
>          return nullptr;
> +    if (!obj->is<ArrayBufferViewObject>())
> +        return nullptr;
> +
> +    Rooted<ArrayBufferViewObject *> viewObject(cx, reinterpret_cast<ArrayBufferViewObject*>(obj));

static_cast<>

But really, since it's an API error to pass in a non-ABVO, I think this entire hunk can just be left alone.  Right?
Attachment #8504411 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/596f4a27f22c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8504411 [details] [diff] [review]
Typed objects are not ArrayBufferViews (yet),

Approval Request Comment
[Feature/regressing bug #]: 1061404
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: on m-c
[Risks and why]: low risk, since this is restoring to a previous state, from an accidental regression
[String/UUID change made/needed]: none
Attachment #8504411 - Flags: approval-mozilla-aurora?
Attachment #8504382 - Flags: review?(jwalden+bmo)
Attachment #8504411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.