798589 - Crash [@ JSScript::ionScript] or "Assertion failure: hasIonScript(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

x = []
y = new Set
z = (function () {
    y.__proto__
})
x.push(0)
wrap(y)
x.forEach(function () {
    for (var j = 0; j < 99999; j++) {
        z()
    }
})

asserts js debug shell on m-c changeset 58f3ccaa02b8 without any CLI arguments at Assertion failure: hasIonScript(), and crashes js opt shell at JSScript::ionScript

Seems like a null deref, but setting s-s to be safe and assuming sec-moderate.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   109194:fa3d21b84a63
user:        Kannan Vijayan
date:        Wed Oct 03 22:55:24 2012 -0400
summary:     Bug 795803 - Enable ICing of JSNative and PropertyOp getters. (r=jandem)

Comment 1

[Bob Clary [:bc] (inactive)]

http://www.lemonde.fr/
reproduced on Windows 7 32bit builds on 32bit, Linux 32bit and OSX 64bit.

Comment 2

[Bob Clary [:bc] (inactive)]

rhel6 32bit
bp-3d382dc2-9543-46cb-ba89-2541c2121006
[@ js::ion::IonCacheGetProperty::attachCallGetter ] 

win7 32bit
bp-99ccbfa5-97ca-4e7d-8fc6-aeaaa2121006
[@ js::ion::IonCacheGetProperty::attachCallGetter(JSContext*, js::ion::IonScript*, JSObject*, JSObject*, js::Shape const*, js::ion::SafepointIndex const*, void*) 

crash seems more reliable in opt builds for me. in debug builds I had to reload several times.

Comment 3

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

Nominating for tracking due to appearance in real world sites.

Even though the testcase seems to be a null deref, it seems to be crashing the browser with JIT code on the stack, so assuming worse-case and raising severity to sec-critical pending further analysis.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Fix.

Using wrong ionScript to get the frameSize().  Don't want the source script for the cache, but the jit-script containing it, which is passed in as an argument.

Comment 5

[Kannan Vijayan [:djvj]]

This is trivial enough to not really require an r?, but I can't push it in anyway since m-i is closed.  Putting it up for review, but I'll be pushing it in
if/when m-i re-opens.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 668724 [details] [diff] [review]
Fix.

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

Indeed, still better with a review ;)

Comment 7

[Kannan Vijayan [:djvj]]

Pushed: https://hg.mozilla.org/mozilla-central/rev/b47091bd3144

Comment 8

[Jan de Mooij [:jandem]]

Landed on m-c so closing.

Comment 9

[Ed Morley [:emorley]]

Have triggered another nightly (on 9f677c2bb33d) to pick up the fix for this, seeing how close we are to uplift :-)

Comment 10

[Carsten Book [:Tomcat]]

just for reference another crash where this could be seen during the topsite scan was http://www.joy.cn

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.