684922 - Crash due to too much recursion through GetCustomIterator

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stacks

q = {}.__proto__
function h() {
    q.p = q
}
h()
function t() {
    for each(let y in []) {
        y.__iterator__ = Iterator
        for (c in []) {}
    }
}
t()

crashes js debug shell on m-c changeset db9e99d537f2 with the patch from bug 683966 without any CLI arguments at js::PropertyTable::search and crashes js opt shell at PrimitiveToObject.

Based on the stack, this seems to be a recursive overflow, but locking s-s just to be safe.

Comment 1

[Daniel Veditz [:dveditz]]

recursive stack exhaustion would be sg:dos, but the stacks you attached don't show that kind of error.

Comment 2

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

(In reply to Daniel Veditz from comment #1)
> recursive stack exhaustion would be sg:dos, but the stacks you attached
> don't show that kind of error.

I should probably attach longer stacks next time, but they seem to repeatedly go on for thousands of lines.

Comment 3

[Jesse Ruderman]

One way to make "too much recursion" explicit is to strip out all the distractions, like I did in bug 671797 ;)

I happen to know that 0x00007fff5f3ffffc is typical "guard page at the end of stack memory" on Mac.  It would be nice if gdb gave more information about the inaccessible memory page.

Comment 4

[Jesse Ruderman]

By the way, in fuzzing/js-known/mozilla-central/assertions.txt, you can write

# Bug 684922
[TMR] GetCustomIterator

The "TMR" is special thing for too-much-recursion crashes. It instructs detect_interesting_crashes to only consider a crash report to be a match if the offending function is seen on 9+ lines.

You have to pick a function from the repeating portion of the stack, but that's saner than picking a function from the top.

Comment 5

[Jesse Ruderman]

Simpler testcase:

var op = Object.prototype;
op.b = op;
op.__iterator__ = Iterator;
for (var c in {}) {}

Comment 6

[Daniel Veditz [:dveditz]]

Ok to unhide the bug then?

Comment 7

[Jesse Ruderman]

Yes.

Comment 8

[Scoobidiver (away)]

The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X in 9.0b2.

Comment 9

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

(In reply to Scoobidiver from comment #8)
> The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X
> in 9.0b2.

Given that we have simple testcases in comment 0 and comment 5, plus it being a topcrasher (assuming the correlation), could someone please kindly take a look?

Nominating for tracking from versions 9 onwards.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Patch

This is an infinite recursion bug. The cycle goes through GetCustomIterator, which seems like a reasonable place to put a JS_CHECK_RECURSION.

I'd be pretty surprised if this had an effect on the topcrashes, but I wouldn't mind being lucky.

Comment 11

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/720247db9dc3

I don't think we need to track this bug, but we should check out the crashstats after it lands.

Comment 12

[David Mandelin [:dmandelin]]

Followup fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/ff1c18668452

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/720247db9dc3
https://hg.mozilla.org/mozilla-central/rev/ff1c18668452

Comment 15

[Christian Holler (:decoder)]

Do we want to fix this on Aurora as well?

Comment 16

[Jesse Ruderman]

(In reply to Scoobidiver from comment #8)
> The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X
> in 9.0b2.

I looked at some of the js::PropertyTable::search crash reports and none of the stacks showed deep recursion. So the topcrash is not this bug.

Comment 17

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

fwiw I put the testcase in comment 5 in a <script> tag and opt Firefox crashed [@ PrimitiveToObject ] - the js::PropertyTable::search signature I got was off a debug shell.

Comment 18

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.