Typed objects are not ArrayBufferViews (yet)

RESOLVED FIXED in Firefox 35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Bug 1003997 made ArrayBuffer.isView return false for typed objects. Bug 1061404 accidentally reverted that change. The reasoning in bug 1003997 is still valid.
(Assignee)

Updated

3 years ago
Blocks: 1061404
(Assignee)

Comment 1

3 years ago
Created attachment 8504272 [details] [diff] [review]
Typed objects are not ArrayBufferViews (yet)
Attachment #8504272 - Flags: review?(jwalden+bmo)

Updated

3 years ago
Attachment #8504272 - Flags: review?(jwalden+bmo) → review+

Comment 2

3 years ago
...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.
(Assignee)

Comment 3

3 years ago
Created attachment 8504382 [details] [diff] [review]
Typed objects are not ArrayBufferViews (yet)

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)
(Assignee)

Comment 4

3 years ago
Created attachment 8504411 [details] [diff] [review]
Typed objects are not ArrayBufferViews (yet),

Minimal change, for backporting.
Attachment #8504411 - Flags: review?(jwalden+bmo)

Comment 5

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Comment 8

3 years ago
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?
status-firefox35: --- → affected
status-firefox36: --- → fixed

Updated

3 years ago
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.