470388 - TM: "Assertion failure: !(fp->flags & JSFRAME_POP_BLOCKS), at ../jstracer.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

for each (let x in [function(){}, new Boolean(false), function(){}]) {}

asserts TM at Assertion failure: !(fp->flags & JSFRAME_POP_BLOCKS), at ../jstracer.cpp:3688

This seems to be a fairly recent regression from a few hours ago and jsfunfuzz _always_ hits it when dealing with dbg js shells due to its simplicity. Seems harmless in opt though.

Approximate regression range on TM:
works (no assert) - http://hg.mozilla.org/tracemonkey/rev/db553f94394c
asserts - http://hg.mozilla.org/tracemonkey/rev/b8d8494faf18

Comment 1

[Brendan Eich [:brendan]]

Bogus assertion due to patch for bug 470300. Will try to fix without neutering.

/be

Comment 2

[Brendan Eich [:brendan]]

Harmless in opt shell. The test forces js_GetScopeChain to reify block objects for the closure in the 0th element of the array being for-each'ed. But the trace ends before the frame pops, so the JSFRAME_POP_BLOCKS flag works as it does in the interpreter. Nesting let in the loop body block shouldn't mess things up because the closures are not in scope of the body. If a closure were in the body we'd abort.

/be

Comment 3

[Jesse Ruderman]

I think this bug can also trigger:

Assertion failure: blockDepth <= StackDepth(script), at ../jsinterp.cpp:6732

Comment 4

[Carsten Book [:Tomcat]]

note, i was running into this during the topsite testrun:

Assertion failure: !(fp->flags & JSFRAME_POP_BLOCKS), at /work/mozilla/builds/1.9.1/mozilla/js/src/jstracer.cpp:3672

http://www.evite.com: EXIT STATUS: CRASHED signal 5 SIGTRAP (84.565852 seconds)

Comment 5

[jag (Peter Annema)]

I "crash" three to four times a day due to this.

Biggest site I've seen it on is IMDb, e.g. http://www.imdb.com/find?s=all&q=dark+knight

Comment 6

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

I've discovered variants of this bug that have the above assert in debug and crash at the following locations in opt: (TM-only)

JS_GetGlobalForObject at 0x000000000000000c

for each (let x in [function(){}, {}, {}, function(){}, function(){}, function(){}]) { ([<y/> for (y in x)]); }

js_PutBlockObject at null

for each (let x in [function(){}, {}, {}, function(){}, function(){}]) { ([<y/> for (y in x)]); }

See bug 471660 for a possibly-related crash at js_UnwindScope that has a similar assert.

(In reply to comment #2)
> Harmless in opt shell.

So far these additional discoveries are crashes at locations that are unlikely to be exploitable.

Comment 7

[Graydon Hoare :graydon]

Reproducible crash on NY times article:

http://www.nytimes.com/2009/01/06/technology/companies/06apple.html

I can look into this. Feels like a blocker to me? Easy to trigger, crashes the browser. Why marked -?

Comment 8

[Andreas Gal :gal]

Yeah, I second graydon's view. Lets work on this.

Comment 9

[Robert Sayre]

It was minused per comment 2: harmless in an opt build. Is it just an assert botch, or is this a crasher in normal builds too?

Comment 10

[Graydon Hoare :graydon]

Attachment: Fix the bug

Sorry if this is "working on a non-blocker"; it was however preventing general use of a debug build, which I'm finding essential for all other bugs that are diagnosable and fixable through use of JS_ASSERT.

Turns out it's a mostly-bogus failure: the assert is only firing when the POP_BLOCKS flag is set on the incoming-and-outgoing frame (calldepth 0), not a reconstructed frame. Sharpening up the assert to exclude this particular combination fixes matters.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 355679 [details] [diff] [review]
Fix the bug

>diff -r adbe8e4b21dc -r e24333561e6b js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp	Mon Jan 05 22:09:23 2009 +0100
>+++ b/js/src/jstracer.cpp	Tue Jan 06 16:43:34 2009 -0800
>@@ -3527,6 +3527,7 @@
>     JS_ASSERT(tm->recoveryDoublePoolPtr >= tm->recoveryDoublePool + MAX_NATIVE_STACK_SLOTS);
> 
> #ifdef DEBUG
>+    bool jsframe_pop_blocks_set_on_entry = bool(cx->fp->flags & JSFRAME_POP_BLOCKS);
>     memset(stack_buffer, 0xCD, sizeof(stack_buffer));
>     memset(global, 0xCD, (globalFrameSize+1)*sizeof(double));
> #endif    
>@@ -3678,7 +3679,8 @@
>        the side exit struct. */
>     JSStackFrame* fp = cx->fp;
> 
>-    JS_ASSERT(!(fp->flags & JSFRAME_POP_BLOCKS));
>+    JS_ASSERT((!(fp->flags & JSFRAME_POP_BLOCKS)) ||

Over-parenthesized on the outside of the !(...), no need. Even better, use JS_ASSERT_IF:

    JS_ASSERT_IF(fp->flags & JSFRAME_POP_BLOCKS,
                 calldepth == 0 && jsframe_pop_blocks_set_on_entry)

r=me with that.

/be

Comment 12

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/e06f46f4f9d9

Comment 13

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

Requesting in‑testsuite? for testcases in comment #0 and comment #6.

Comment 14

[Andreas Gal :gal]

Looks like debug only so removing request for blocking.

Comment 15

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

The testcase in comment #0 got fixed but testcases in comment #6 morphed into something else, spun them off as bug 472450.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e06f46f4f9d9

Comment 17

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

Comment on attachment 355679 [details] [diff] [review]
Fix the bug

a191=beltzner

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a93833dfbb33

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/regress/regress-470388-0{1..3}.js

browser only crash @ js_PutBlockObject js/src/jsobj.cpp:2225 in yesterdays 1.9.2 build with -02 at least

bp-01ef7bf6-14ab-45b2-b647-5dc3a2090123
bp-31506d7e-03b6-4723-8853-36f252090123

I'll be checking these tests in later today.

Comment 20

[Bob Clary [:bc] (inactive)]

Checking in js1_7/regress/regress-470388-01.js
Checking in js1_7/regress/regress-470388-02.js
Checking in js1_7/regress/regress-470388-03.js
http://hg.mozilla.org/mozilla-central/rev/3ce50075697b

Comment 21

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2