361273 - Assert fail: cg->stackDepth >= 0, at jsemit.c:164

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gavin Reaney]

In low memory conditions it's possible to get the following assertion failure reported (stack trace will follow shortly):

Assertion failure: cg->stackDepth >= 0, at jsemit.c:164

The root cause is this error handling in jsemit:

#define EMIT_ATOM_INDEX_OP(op, atomIndex)                                     \
    JS_BEGIN_MACRO                                                            \
        if (!EmitAtomIndexOp(cx, op, atomIndex, cg) < 0)                      \
            return JS_FALSE;                                                  \
    JS_END_MACRO

EmitAtomIndexOp returns a JSBool so any error returned will actually be ignored by mistake, leading to the assert fail later.

In a release build we get stack underflow reported when the compiled script is run, and JS_ArenaAllocate can get itself into an infinite loop that only exits when malloc fails (more details on this soon).

Comment 1

[Gavin Reaney]

Attachment: Stack trace of assert failure

Comment 2

[Gavin Reaney]

Attachment: Patch to simulate malloc fail

To reproduce the assert fail:

- apply the patch
- run the JS shell
- enter the following script:

obj = new Object()

Comment 3

[Gavin Reaney]

Attachment: Release build behaviour of this bug

The attachment shows what would happen in a release build of the JS shell with the single malloc failure simulated by attachment 246017 [details] [diff] [review].

JS_ArenaAllocate is being called with a request for a block of 4294967288 bytes which it attempts to honour:

    gross = hdrsz + JS_MAX(nb, pool->arenasize);
    b = (JSArena *) malloc(gross);

The calculation of 'gross' overflows and we end up requesting 15 bytes instead. Since this is less than 'nb' we remain stuck in a near infinite loop that will exit only when malloc fails (in low memory situations this should happen quite quickly, but it still seems like something we should guard against).

I'll upload a patch soon with the simple fix to jsemit, and some detection code for the wrap-around described above. This may be of interest for some of the other bugs that report that 'firefox uses all CPU and memory', although it's just a random guess that the two might be related.

Comment 4

[Gavin Reaney]

Attachment: Fixed jsemit error handling + extra check in JS_ArenaAllocate

Ready for review.

Comment 5

[Brendan Eich [:brendan]]

Attachment: version of Gavin's patch that I'm landing

Comment 6

[Brendan Eich [:brendan]]

Attachment: better version, just landed

Gavin, thanks for another great find and fix.  Landed on trunk:

Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.31; previous revision: 3.30
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.228; previous revision: 3.227
done

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 246051 [details] [diff] [review]
better version, just landed

Affects the 1.8.0 branch too.

/be

Comment 8

[Jay Patel [:jay]]

Comment on attachment 246051 [details] [diff] [review]
better version, just landed

Approved for both branches.  a=jay for drivers.  Please land ASAP on 1.8.1 and 1.8.0 branches.

Comment 9

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.28.8.3; previous revision: 3.28.8.2
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.58; previous revision: 3.128.2.57
done

and (with merging) on the 1.8.0 branch:

Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.28.16.2; previous revision: 3.28.16.1
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.3.2.12; previous revision: 3.128.2.3.2.11
done

/be