245308 - Assertion failure: top < ss->printer->script->depth, at r:/mozilla/js/src/jsopcode.c:606

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[timeless]

###!!! ASSERTION: bad state: 'mState >= s', file 
r:\mozilla\js\src\xpconnect\src\xpcprivate.h, line 885
Document data:text/html,<title>spider loaded successfully
--DOMWINDOW == 5
WARNING: NS_ENSURE_TRUE(mDocument) failed, file 
r:/mozilla/content/base/src/nsDocumentViewer.cpp, line 1457
WARNING: NS_ENSURE_TRUE(mDocument) failed, file 
r:/mozilla/content/base/src/nsDocumentViewer.cpp, line 1457
WARNING: NS_ENSURE_TRUE(mDocument) failed, file 
r:/mozilla/content/base/src/nsDocumentViewer.cpp, line 1457
###!!! ASSERTION: bad state: 'mState >= s', file 
r:\mozilla\js\src\xpconnect\src\xpcprivate.h, line 885
Document http://www.mozilla.org/start/ loaded successfully
Assertion failure: top < ss->printer->script->depth, at 
r:/mozilla/js/src/jsopcode.c:606

i have two flavors of this, one has the stack pretty much overflowed:
 	0012fe10()	
 	user32.dll!TranslateMessage()  + 0xef	
 	user32.dll!PeekMessageW()  + 0xe3	
 	0012fe10()	
 	user32.dll!TranslateMessage()  + 0xef	
 	user32.dll!PeekMessageW()  + 0xe3

Comment 1

[James Ross]

Aww, I thought I'd got a new bug. I've got a JS function that when any attempts
are made to convert to string (toSource/toString) is fails with an OOM error in
Mozilla and this fatal error in JSshell:

Assertion failure: top < ss->printer->script->depth, at jsopcode.c:616

I will try to reduce the function to the minimum required and attach. I can also
attach anything useful that JSshell can produce (e.g. disassembly of the
function) if required.

Comment 2

[James Ross]

function foo(e) {
    try {}
    catch(ex) {
        try {}
        catch(ex) {}
    }
}

That's all that's needed - it's that nested try/catch that's killing it, two of
them sequentially is fine, as is one, and nested in the try block, but nested
like that isn't.

js> dis(foo)
main:
00000:  try
00001:  goto 46 (45)
00004:  setsp 0
00007:  nop
00008:  name "Object"
00011:  pushobj
00012:  newinit
00013:  exception
00014:  initcatchvar "ex"
00017:  enterwith
00018:  try
00019:  goto 41 (22)
00022:  setsp 1
00025:  nop
00026:  name "Object"
00029:  pushobj
00030:  newinit
00031:  exception
00032:  initcatchvar "ex"
00035:  enterwith
00036:  leavewith
00037:  goto 41 (4)
00040:  nop
00041:  leavewith
00042:  goto 46 (4)
00045:  nop

Source notes:
  0:     0 [   0] newline
  1:     1 [   1] hidden
  2:     4 [   3] newline
  3:     7 [   3] catch
  5:    17 [  10] xdelta
  6:    17 [   0] hidden
  7:    18 [   1] newline
  8:    19 [   1] hidden
  9:    22 [   3] newline
 10:    25 [   3] catch
 12:    35 [  10] xdelta
 13:    35 [   0] hidden
 14:    36 [   1] hidden
 15:    37 [   1] hidden
 16:    40 [   3] endbrace
 17:    41 [   1] hidden
 18:    42 [   1] hidden
 19:    45 [   3] endbrace

Exception table:
start   end     catch
  19    22      22
  1     4       4

Comment 3

[Brendan Eich [:brendan]]

Silver, you may have a new bug -- timeless didn't diagnose the source as you did
-- thanks for that.  Is this trunk only, or in 1.7 (and older)?  It may be very
old.  Shaver's an old try/catch/finally hand (he implemented the first cut),
cc'ing him and taking.

Timeless, we may be morphing your bug, but Silver's got the details.

/be

Comment 4

[James Ross]

(In reply to comment #3)
> Is this trunk only, or in 1.7 (and older)?

It crashes in my trunk CVS build (17 hours old), Mozilla 1.7.1 and Firefox
0.9.1, which may have an older version of Spidermonkey (I'm not sure if it's
synced or not). Those are the only builds I have to hand currently, but I'll try
a few older ones now.

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Shaver, you remember bug 104077 (maybe not, it was painful for both of us;
remember chingfa?).  This is much delayed followup.

The code generator's stack model must match the decompiler's.  That means
JSOP_ENTERWITH has to have stack balance 0, and JSOP_LEAVEWITH -1.  The code as
it stood handled the hidden enterwith generated as part of a catch header in
the code for SRC_CATCH, and didn't need the SRC_HIDDEN test in the
JSOP_ENTERWITH case at all.  But JSOP_LEAVEWITH can't be hidden in a catch
clause, not from the point of view of popping the cookie the decompiler must
push for both the SRC_CATCH and the JSOP_ENTERWITH cases.

I changed the cookies to be C-style comments that should stand out if, due to a
bug, they ever leak.

I fixed an unrelated bug where SRC_LABEL was emitted with arity 0 -- naughty,
this hides the subsequent note-byte or two, possibly leading to failure to
decompile.  In disambiguating JSOP_LEAVEWITH, I used SRC_CATCH instead of
SRC_HIDDEN for the normal-flow path out of the catch, and that has arity 1, so
I stuck the code generator's modeled stack depth in the note.  The decompiler
checks that with LOCAL_ASSERT, which does a runtime test and fails on botch.

Everything should be bullet-proof now.

/be

Comment 6

[Brendan Eich [:brendan]]

Silver, thanks for reducing the testcase -- big help.

sseib, this should be the cause of the Out of memory you saw.  Can you confirm?

/be

Comment 7

[James Ross]

(In reply to comment #3)
>  Is this trunk only, or in 1.7 (and older)?  It may be very old.

Crashes on Mozilla 1.7.1, 1.6, 1.5, 1.4, 1.3 and 1.2.

Mozilla 1.1 and 1.0 don't crash, but convert the function to the string
  "function foo(e) {try {} catch (ex) {try {} catch (ex) {}}ðÄÒe}"
which isn't /quite/ right. :)

The corruption seems to stay the same during each run, but varies in general.

I stopped when I got back to Mozilla 1.0. :)

Comment 8

[Samuel Sieb]

Hard to confirm.  When this is checked in, I'll update and run with it and see
if it happens again.

Comment 9

[timeless]

I don't mind the hijacking and i *do* remember that other bug :-). This seems like a reasonable 
explanation for my original crash, we do use a lot of try/catch blocks. The original report mentions two 
problems, an assert in xpconnect and the crash (this bug is about the crash). I may already have 
another bug for the assert.... If I run into some other circumstances for any of this stuff after this bug is 
fixed (and we update spidermonkey ... - as usual i'll want the fix on the branch...), I'll find/file new 
bugs.

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 157281 [details] [diff] [review]
proposed fix

Can you slip the repro case into the test suite, when you next have Phil at
your disposal?	Assuming we pass the suite (really, this time!), r=shaver.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 157281 [details] [diff] [review]
proposed fix

Checked in on the trunk.

/be

Comment 12

[chris hofmann]

is this ready for the branch?

Comment 13

[chris hofmann]

Comment on attachment 157281 [details] [diff] [review]
proposed fix

a=chofmann for aviary and 1.7 branches

Comment 14

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 15

[Brendan Eich [:brendan]]

Fixed everywhere.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-245308.js

Thanks to silver.

Comment 17

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-245308.js checked in.

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed.