Closed
Bug 1082141
Opened 10 years ago
Closed 10 years ago
Typed objects are not ArrayBufferViews (yet)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(3 files)
1.14 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
33.72 KB,
patch
|
Details | Diff | Splinter Review | |
3.43 KB,
patch
|
Waldo
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
Attachment #8504272 -
Flags: review?(jwalden+bmo)
Updated•10 years ago
|
Attachment #8504272 -
Flags: review?(jwalden+bmo) → review+
Comment 2•10 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•10 years ago
|
||
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•10 years ago
|
||
Minimal change, for backporting.
Attachment #8504411 -
Flags: review?(jwalden+bmo)
Comment 5•10 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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/596f4a27f22c
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/596f4a27f22c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 8•10 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?
Updated•10 years ago
|
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Updated•10 years ago
|
Attachment #8504382 -
Flags: review?(jwalden+bmo)
Updated•10 years ago
|
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.
Description
•