665914 - Crash [@ js::ArrayBuffer::getArrayBuffer] or "Assertion failure: obj,"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

ArrayBuffer.prototype['byteLength']

crashes js opt shell on TM changeset 57ef3b619966 without any CLI arguments at js::ArrayBuffer::getArrayBuffer and asserts js debug shell at Assertion failure: obj,

Comment 1

[Jesse Ruderman]

The first bad revision is:
changeset:   b89374bc34ee
user:        Nikhil Marathe
date:        Tue Jun 14 15:33:11 2011 -0400
summary:     Bug 656519 - Avoid a malloc (and a finalizer) by storing the malloc'd array in our slots instead of in a separate malloc'd structure in our private field. r=mrbkap

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix: Allow getArrayBuffer to check and return NULL

All external API users should use js_IsTypedArray as a check, and getArrayBuffer could always return NULL otherwise. prop_getByteLength was the one exception where things could go wrong. The patch handles the null case. Test case updated.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #2)
> Created attachment 540793 [details] [diff] [review] [review]
> Fix: Allow getArrayBuffer to check and return NULL
> 
> All external API users should use js_IsTypedArray as a check, and
> getArrayBuffer could always return NULL otherwise. prop_getByteLength was
> the one exception where things could go wrong. The patch handles the null
> case. Test case updated.

Nikhil, just wondering whether missed setting a reviewer, or is a new patch forthcoming?

Comment 4

[Andreas Gal :gal]

Comment on attachment 540793 [details] [diff] [review]
Fix: Allow getArrayBuffer to check and return NULL

Why not do the obj != NULL bypass before the gerArrayBuffer?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andreas,

If you mean something like below in prop_getByteLength
if (!obj) {
  // set to 0
}

JSObject *arrayBuffer = getArrayBuffer(obj);

then this code can still fail:
var x = {}
x.__proto__ = ArrayBuffer.prototype
x.byteLength

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 540793 [details] [diff] [review]
Fix: Allow getArrayBuffer to check and return NULL

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

::: js/src/jstypedarray.cpp
@@ +114,4 @@
>  JSObject *
>  ArrayBuffer::getArrayBuffer(JSObject *obj)
>  {
> +    while (obj != NULL && !js_IsArrayBuffer(obj))

Nit: In the JS engine, we don't bother explicitly comparing to null, and instead would just write |while (obj && ...)|.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix: Allow getArrayBuffer to check and return NULL

fix nit.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/e52e78e088e5

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/e52e78e088e5

Comment 10

[Asa Dotzler [:asa]]

This bug was nominated for tracking Firefox 7. Is the fix suitable for landing on Aurora? If so, please nominate the attachment with an approval request and an explanation of the value of the patch and the associated risk. Thanks.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This bug is nominated tracking-firefox7 because it fixes a crash that made it to mozilla-central. (See comment 1)

Comment 12

[Asa Dotzler [:asa]]

We have lots of crashes and release drivers cannot add a lot of value by reading all of our crash reports several times a week. If someone believes this is a fix worth taking on Aurora, then please nominate the attachment with a risk analysis. Thanks.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #12)
> We have lots of crashes and release drivers cannot add a lot of value by
> reading all of our crash reports several times a week. If someone believes
> this is a fix worth taking on Aurora, then please nominate the attachment
> with a risk analysis. Thanks.

Brendan mentions this particular bug should be taken:

http://groups.google.com/group/mozilla.dev.tech.js-engine.internals/browse_thread/thread/626c85124555c0c9

From fuzzers' perspective, it fixes a bug that fuzzers find easily.

Nikil will have to nominate the attachment with a risk analysis.

Comment 14

[Asa Dotzler [:asa]]

we think this made the merge, re-nominate if thta's not the case.

Comment 15

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929