Closed Bug 827225 Opened 7 years ago Closed 7 years ago

"Assertion failure: obj->isTypedArray()" with WebGL readPixels

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 + verified
firefox20 + verified
firefox21 --- verified

People

(Reporter: jruderman, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Assertion failure: obj->isTypedArray(), at js/src/jstypedarray.cpp:3769
Attached file stack
Hey, there's DOM bindings stuff on the stack.  Please cc me when that happens?  ;)
So in the IDL, this is defined as "ArrayBufferView? pixels".

And that's what the binding code passes in.  But then the WebGL code didn't get updated from using JS_GetTypedArrayByteLength looks like...
So per spec this should throw if the type of the passed-in ArrayBufferView doesn't match the requested data type.  Which is fine: we could move the length get to after that check.   But JS_GetTypedArrayType will _also_ assert if the object is not a typed array.  Can we have an API like that but for array buffer views in general?
There are similar issues with texImage2D, looks like.  It's too bad there's no interface that's a subclass for all typed arrays but not data views.  :(
This patch renames the JSAPI call and also makes it accepts any ArrayBufferView, adding in a new TYPE_DATAVIEW to handle the DataView case. TYPE_DATAVIEW isn't an element type like the rest, but given that it was in the ArrayBufferView namespace, it makes sense this way.

Do dmandelin, I'm really pegging you for review + super-review (the code change is pretty trivial) and bz for the renaming in the rest of the tree (following bz's suggestion that a simple JSAPI rename shouldn't require more than one reviewer.)
Assignee: general → sphink
Status: NEW → ASSIGNED
Attachment #698796 - Flags: superreview?(dmandelin)
Attachment #698796 - Flags: review?(bzbarsky)
s/Do/So/
Comment on attachment 698796 [details] [diff] [review]
Born JS_GetTypedArrayType, now JS_GetArrayBufferViewType

r=me.

Note that this bug also needs a second patch to move that JS_GetTypedArrayByteLength call in WebGLContext::ReadPixels further down the function, to after the dataType has been validated.
Attachment #698796 - Flags: review?(bzbarsky) → review+
Attachment #698796 - Flags: superreview?(dmandelin) → superreview+
Oh, I didn't look into what else was needed to fix this problem. But if that's all it is, sure, I'll pony up the patch for it. It builds, at least.
Attachment #698842 - Flags: review?(bzbarsky)
Comment on attachment 698842 [details] [diff] [review]
Avoid JS_GetArrayBufferViewType until safe to call

Sweet.  r=me!
Attachment #698842 - Flags: review?(bzbarsky) → review+
Attachment #698796 - Flags: checkin+
Attachment #698842 - Flags: checkin+
We probably want this on branches too...  I guess data view is on everything including 17, right?

Or is the code safe enough in opt builds that we don't need to worry about that?
https://hg.mozilla.org/mozilla-central/rev/0757dd0a653d
https://hg.mozilla.org/mozilla-central/rev/669bc3bb9a87
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Flags: needinfo?(sphink)
(In reply to Boris Zbarsky (:bz) from comment #12)
> We probably want this on branches too...  I guess data view is on everything
> including 17, right?
> 
> Or is the code safe enough in opt builds that we don't need to worry about
> that?

If this impacts FF17 (without any user complaints), why uplift any risk?
> If this impacts FF17 (without any user complaints), why uplift any risk?

Because typically assertion failures mean a security problem.  I have yet to see any indication that that's not the case here.  In particular, no response to comment 12 indicating this is NOT a security bug.

Re-nominating, since apparently this wasn't clear.
In that case, we'll leave the tracking nom on until Steve answers comment 12.
Flags: needinfo?(sphink)
Comment on attachment 698796 [details] [diff] [review]
Born JS_GetTypedArrayType, now JS_GetArrayBufferViewType

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 575688
User impact if declined: possible read of private data
Testing completed (on m-c, etc.): on m-c for a week or three now
Risk to taking this patch (and alternatives if risky): low. It's patching up a hole in error detection.
String or UUID changes made by this patch: none
Attachment #698796 - Flags: approval-mozilla-beta?
Attachment #698796 - Flags: approval-mozilla-aurora?
Comment on attachment 698842 [details] [diff] [review]
Avoid JS_GetArrayBufferViewType until safe to call

This patch must be paired with the other one on this bug, but to repeat the justification:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 575688
User impact if declined: possible read of private data
Testing completed (on m-c, etc.): on m-c for a week or three now
Risk to taking this patch (and alternatives if risky): low. It's patching up a hole in error detection.
String or UUID changes made by this patch: none
Attachment #698842 - Flags: approval-mozilla-beta?
Attachment #698842 - Flags: approval-mozilla-aurora?
Comment on attachment 698796 [details] [diff] [review]
Born JS_GetTypedArrayType, now JS_GetArrayBufferViewType

Approving given Steve's feedback - we'll want to close up this hole and the fix is low enough risk as this stage in beta cycle.
Attachment #698796 - Flags: approval-mozilla-beta?
Attachment #698796 - Flags: approval-mozilla-beta+
Attachment #698796 - Flags: approval-mozilla-aurora?
Attachment #698796 - Flags: approval-mozilla-aurora+
Attachment #698842 - Flags: approval-mozilla-beta?
Attachment #698842 - Flags: approval-mozilla-beta+
Attachment #698842 - Flags: approval-mozilla-aurora?
Attachment #698842 - Flags: approval-mozilla-aurora+
Reproduced the crash on 2013-01-07 Nightly Debug using testcase from comment 0.

Verified the fix for 2013-01-23 Beta Debug using the same testcase from comment 0. No crashes for this version.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20130122 Firefox/19.0
Build ID: 20130122230158
Verified fixed with Firefox 20 beta 7 debug build, on Mac OSX 10.8.2, using the testcase provided in the description. Firefox doesn't crash after the testcase is opened.

Build ID: 20130323031722
Verified fixed on Firefox 21 beta 2 (latest beta debug build), on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130411 Firefox/21.0 

Build ID:  20130411111427
You need to log in before you can comment on or make changes to this bug.