Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][fixed-in-tracemonkey])

Attachments

(1 attachment)

Reporter

Description

9 years ago
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

Comment 1

9 years ago
Simpler:

(1 ? 2 : delete(0?0:{})).w;

Comment 2

9 years ago
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
Assignee

Comment 3

9 years ago
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.
Assignee

Comment 4

9 years ago
Posted patch v1Splinter Review
Nope! It was in JSParseNode::become.
Assignee: general → jorendorff
Attachment #504561 - Flags: review?(jimb)
blocking2.0: --- → betaN+
Whiteboard: hardblocker

Comment 5

9 years ago
Comment on attachment 504561 [details] [diff] [review]
v1

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+
Assignee

Comment 6

9 years ago
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.

https://hg.mozilla.org/tracemonkey/rev/42a8233d986f
Assignee

Updated

9 years ago
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.

/be

Comment 8

9 years ago
(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.

/be
Assignee

Comment 10

9 years ago
I'll add an assertion today.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter

Updated

8 years ago
Blocks: 676763
Reporter

Comment 13

7 years ago
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.