829813 - Crash [@ js::Shape::attributes] or [@ js::baseops::GetAttributes]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

for (x in [0]) {
    (function() {
        return Object.propertyIsEnumerable
    })().call([0], x)
}

crashes js debug shell on m-c changeset 44dcffe8792b without any CLI arguments at js::Shape::attributes

Seems like an obvious null-deref but I'll leave it to the devs to open this up.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   118493:f4671ccc4502
user:        Brian Hackett
date:        Thu Jan 10 17:53:11 2013 -0700
summary:     Bug 827490 - Allow native objects to have both slots and dense elements, rm dense/slow array distinction, r=billm, dvander.

Comment 1

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

Related to bug 829795?

Comment 2

[Brian Hackett [Laid off!]]

NULL deref, not s-s.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

This was caused by missing checks for element properties, the attached patch also fixes several similar bugs.  Also, I forgot to fix IsImplicitProperty per your comments in bug 827490, do you still want that?  (I'm neutral about this, it seems kind of weird to have two different methods to mark properties found which don't have shapes, but could still help code clarity.)

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 701570 [details] [diff] [review]
patch

Yes, please do fix the IsImplicitProperty thing. Also, it would be nice if you could assert in these functions that the object is native or non-native, so that we always call the right one.

Also, please add a helper function GetShapeAttributes or something that would work for indexed properties. Then all the calls where you do this:
  IsImplicitProperty(prop) ? JSPROP_ENUMERATE : prop->attributes()
could be replaced by the helper function.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0cc623ea14

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I hate to be such a stickler, but you forgot to fix the one in jsiter.cpp.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: cleanup

Cleanup IsImplicitProperty.  I wasn't able to assert isNative vs. !isNative on the affected objects as they aren't passed to the implicit property functions, and adding more arguments to the functions just junks up the source while not actually helping anything (the wrong object can be passed in).

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 701964 [details] [diff] [review]
cleanup

Thanks. I was thinking that IsImplicitDenseElement could assert that, if the shape isn't 0x1, then it must be native.

Comment 12

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec09b923083

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f0cc623ea14
https://hg.mozilla.org/mozilla-central/rev/4ec09b923083

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397