349653 - "Assertion failure: OBJ_GET_CLASS(cx, obj) == &js_ArrayClass" with ternary and array comprehension

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
  In a debug build,
  javascript:void ({y: true ? [1 for (x in [2])] : 3 })

Result:
  Assertion failure: OBJ_GET_CLASS(cx, obj) == &js_ArrayClass, at js/src/jsinterp.c:6090

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I can't reproduce this anymore. I'm optimistically marking it as a dupe of bug 349493.

*** This bug has been marked as a duplicate of 349493 ***

Comment 2

[Jesse Ruderman]

I can still reproduce sometimes with the original steps.  Also, I can reproduce reliably with...

Steps to reproduce:
  In a debug build,
  javascript:let (a) true ? [2 for each (z in function(id) { return id })] : 3;

Result: 
  jsinterp.c:6081 crashes or asserts.
  JS_ASSERT(OBJ_GET_CLASS(cx, obj) == &js_ArrayClass);

obj has been observed to be 0x00000000 or 0x80000000 (causing a segfault).  When it asserts, I'm guessing it "obj" is just a bogus address that can be dereferenced without segfaulting.

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix

So the constant folder has to update the unique down-pointer from parent to child when it moves a JSParseNode due to a constant condition selecting a then or else expression or clause.  But for TOK_ARRAYCOMP, there's an up-pointer several nodes down in the TOK_ARRAYPUSH node, pn_array, that needs fixing.

Otherwise we never point from two nodes to one -- the AST is a tree except for this cyclic up link, and there's no need for multiple connections in general.  So I'm not going to mark a FIXME here, since I do not expect more up-pointers to be added to JSParseNode, so there's no point in generalizing.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: altern-patch I like better

This preserves the single-pointer-to-JSParseNode-from-any-other-JSParseNode (where the other node is always the parent, and pn_next does not count for the constant ? or if condition case) property.  If you like it better too, please retract r+ on other patch and obsolete it, and r+ this one.  It fixes the bug, using the nearest C equivalent to a statically scoped outer function variable (cg->arrayCompSlot).

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 236340 [details] [diff] [review]
altern-patch I like better

Yeah, this is better.

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 7

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)

> 
> obj has been observed to be 0x00000000 or 0x80000000 (causing a segfault). 

i believe in some cases 0x80000000 may point to used heap - if one consumes enough memory.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 236340 [details] [diff] [review]
altern-patch I like better

a=beltzner on behalf of 181drivers

Comment 9

[georgi - hopefully not receiving bugspam]

on linux 2.6 creating 2 js with byte size 256MB each makes 0x80000000 mapped on the heap.

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-349653.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-349653.js,v  <--  regress-349653.js
initial revision: 1.1

Comment 11

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch too.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060901 windows/mac*/linux

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 2006090118 windows/mac*/linux

Comment 14

[Bob Clary [:bc] (inactive)]

the test was originally misplaced.
Removing js1_5/Regress/regress-349653.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-349653.js,v  <--  regress-349653.js
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/block/regress-349653.js,v
done
Checking in js1_7/block/regress-349653.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-349653.js,v  <--  regress-349653.js
initial revision: 1.1
done