Closed Bug 626436 Opened 11 years ago Closed 11 years ago

Crash in js_EmitTree


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: decoder, Assigned: jorendorff)



(Keywords: crash, regression, testcase, Whiteboard: [hardblocker][fixed-in-tracemonkey])


(1 file)

The following code will crash TM tip both on 32 and 64 bit:

( 'false'? length(input + ''): delete(null?0:{}),0 ).watch('x', function f() { });

The crash looks non-harmful at first glance, it looks like an invalid field write to a null pointer:

Program received signal SIGSEGV, Segmentation fault.
0x080ca6bc in js_EmitTree (cx=0x8420fb0, cg=0xbfffec0c, pn=0x0) at jsemit.cpp:4556
4556        pn->pn_offset = top = CG_OFFSET(cg);
(gdb) print pn
$1 = (JSParseNode *) 0x0

(1 ? 2 : delete(0?0:{})).w;
The first bad revision is:
changeset:   f1be82c29a1e
user:        Jim Blandy
date:        Fri Jan 14 18:09:09 2011 -0800
summary:     Bug 501908: Avoid O(n^2) behavior when recycling large trees. r=igor
Blocks: 501908
Keywords: regression
Heh, I made the same shorter test case here.

It crashes in js_EmitTree:
      case TOK_COMMA:
         * Emit SRC_PCDELTA notes on each JSOP_POP between comma operands.
         * These notes help the decompiler bracket the bytecodes generated
         * from each sub-expression that follows a comma.
        off = noteIndex = -1;
        for (pn2 = pn->pn_head; ; pn2 = pn2->pn_next) {
            if (!js_EmitTree(cx, cg, pn2))
                return JS_FALSE;

But pn->pn_head is null. By the time we get here, the COMMA node is already trashed.

js_FoldConstants is somehow defacing it, under RecycleTree, in the process of optimizing (0 ? 0 : {}) to just ({}). I guess RecycleTree is buggy.
Attached patch v1Splinter Review
Nope! It was in JSParseNode::become.
Assignee: general → jorendorff
Attachment #504561 - Flags: review?(jimb)
blocking2.0: --- → betaN+
Whiteboard: hardblocker
Comment on attachment 504561 [details] [diff] [review]

Wonderful!  However, it looks super-strange to have pn2's tail pointing at pn's head field; a brief comment pointing out that this is just going to get copied into pn, and that pn2 will be cleared, would be great.
Attachment #504561 - Flags: review?(jimb) → review+
OK. I moved the strange code down so that it happens after the copying. It looks less strange that way. I added a comment too.
Whiteboard: hardblocker → [hardblocker][fixed-in-tracemonkey]
Setting tail = &head resets to the empty-list basis case, but only in part. Why not use makeEmpty() to set pn_count to 0 too? rs=me on followup.

(In reply to comment #7)
> Setting tail = &head resets to the empty-list basis case, but only in part. Why
> not use makeEmpty() to set pn_count to 0 too? rs=me on followup.

And/or, an assertion that pn_count is zero? We're just copying an existing empty list, so we should just assert that it was well-formed to begin with.
Sure, just need the new &pn_head. Yeah, asserting seems good.

I'll add an assertion today.
Closed: 11 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-626436.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.