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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: obj->isTypedArray(), at js/src/jstypedarray.cpp:3769

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hey, there's DOM bindings stuff on the stack.  Please cc me when that happens?  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Born JS_GetTypedArrayType, now JS_GetArrayBufferViewType

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.)

Comment 7

[Steve Fink [:sfink] [:s:]]

s/Do/So/

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Avoid JS_GetArrayBufferViewType until safe to call

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698842 [details] [diff] [review]
Avoid JS_GetArrayBufferViewType until safe to call

Sweet.  r=me!

Comment 11

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/669bc3bb9a87
http://hg.mozilla.org/integration/mozilla-inbound/rev/0757dd0a653d

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0757dd0a653d
https://hg.mozilla.org/mozilla-central/rev/669bc3bb9a87

Comment 14

[Alex Keybl [:akeybl]]

(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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Alex Keybl [:akeybl]]

In that case, we'll leave the tracking nom on until Steve answers comment 12.

Comment 18

[Steve Fink [:sfink] [:s:]]

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

Comment 19

[Steve Fink [:sfink] [:s:]]

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

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0f496d7265e
https://hg.mozilla.org/releases/mozilla-beta/rev/0859365a27e3

This is also in my queue to land on Aurora once it reopens.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97f3a2f13f32
https://hg.mozilla.org/releases/mozilla-aurora/rev/454902c3c709

Comment 23

[Virgil Dicu [:virgil] [QA]]

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

Comment 24

[Manuela Muntean [Away]]

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

Comment 25

[Tiziana Sellitto [:tiziana]]

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