375695 - "Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) == fp->blockChain" with |catch| that binds nothing and has a catchguard

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> try { try { throw 1 } catch([] if false) { } } catch(ex) {}
Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) == fp->blockChain, at jsinterp.c:5837

Seems harmless in opt.

Bug 361566 triggers the same assertion, so it's possible that this is a dup of it.

Split from bug 351102 per bug 351102 comment 30.

Comment 1

[Brendan Eich [:brendan]]

The catch guard code is not leaving the block (zero-count block, at that) entered for the catch ([] if false) head:

js> function f(){try { try { throw 1 } catch([] if false) { } } catch(ex) {}}
js> dis(f)
main:
00000:  try
00001:  try
00002:  one
00003:  throw
00004:  goto 31 (27)
00007:  setsp 0
00010:  enterblock depth 0 {}
00013:  exception
00014:  dup
00015:  dup
00016:  pop
00017:  pop
00018:  false
00019:  ifeq 29 (10)
00022:  pop
00023:  leaveblock 0
00026:  goto 31 (5)
00029:  throw
00030:  nop
00031:  goto 51 (20)
00034:  setsp 0
00037:  enterblock depth 0 {ex: 0}
00040:  exception
00041:  setlocalpop 0
00044:  leaveblock 1
00047:  goto 51 (4)
00050:  nop
00051:  stop

Source notes:
  0:     4 [   4] hidden  
  1:    10 [   6] catch    guard delta 9
  3:    15 [   5] decl     offset 2
  5:    22 [   7] hidden  
  6:    23 [   1] catch   
  8:    26 [   3] hidden  
  9:    29 [   3] hidden  
 10:    30 [   1] endbrace
 11:    31 [   1] hidden  
 12:    37 [   6] catch   
 14:    44 [   7] catch    stack depth 1
 16:    47 [   3] hidden  
 17:    50 [   3] endbrace

Exception table:
start   end     catch
  2     7       7
  1     34      34
js> 

/be

Comment 2

[Brendan Eich [:brendan]]

Er, no, the setsp 0 at bytecode 34 should pop fp->blockChain. I'll have to debug...

/be

Comment 3

[Brendan Eich [:brendan]]

The patch for bug 355832 created this problem; the two bugs do not agree on what JSOP_SETSP should do with a zero-count block on fp->blockChain.

/be

Comment 4

[Jesse Ruderman]

"the two bugs"?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This bug and bug 355832.

Brendan, see also: bug 361566, comment 1.

Comment 6

[Brendan Eich [:brendan]]

I'm going to patch here, so making bug 361566 block this bug doesn't make sense. It came first, and I could dup this bug against it. But I've taken this one, and it doesn't really hurt to block the other way.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

This includes the patch for bug 379483.

Let me know if you spot other uses for JSOP_POPN.

/be

Comment 8

[Igor Bukanov]

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

>-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 9)
>+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 10)

Quick nore before doing any review: -10 is already taken on 1.8.1 branch.

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Fixed JSXDR_BYTECODE_VERSION; restore lost SRC_HIDDEN test for JSOP_POPN decomp.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2 updated to track trunk

The patch for bug 379483 landed, so this doesn't contain that hunk.

/be

Comment 11

[Igor Bukanov]

After looking for some time into the patch I still can not get why setsp alone can not be used to unwind the stack and the scope chain. If it can do it for the exception handler, why it can not do it after non-local jump?

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> After looking for some time into the patch I still can not get why setsp alone
> can not be used to unwind the stack and the scope chain. If it can do it for
> the exception handler, why it can not do it after non-local jump?
> 

To be precise: EmitNonLocalJumpFixup issues missing leavewith, leaveblock, popn etc. to restore the stack and scope chain. But setsp is doing exactly the same job on its own. So the idea is to replace  leavewith, leaveblock, popn via single setsp.

Comment 13

[Brendan Eich [:brendan]]

This all grew like topsy. We could use JSOP_SETSP, but you point out something I hadn't noticed: JSOP_SETSP does not emulate JSOP_ENDITER at all. This is not quite a bug, since closing generator-iterators eagerly is an optimization. So I would like to ignore the asymmetry.

But I would also like to shrink EmitNonLocalJumpFixup and the code it generates in a separate bug. Igor: I'll file that and note it in a FIXME comment, and hit you up for re-review shortly, if the rest of the patch looks ok.

/be

Comment 14

[Brendan Eich [:brendan]]

Although I need to review what we agreed on for ES4, about automating generator close calls only when the generator-iterator is started by the for-in loop that's exiting. We did agree to automate only that case, but the question is: do we automate all exits from the loop, or only the normal one that runs through the unhidden JSOP_ENDITER.

Seems to me we'll want to automate all, so we will still JSOP_SETSP to emulate any hidden JSOP_ENDITERs. Comments?

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed fix, ready to land

Comment 16

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsemit.c 3.248
js/src/jsinterp.c 3.346
js/src/jsopcode.c 3.233
js/src/jsopcode.tbl 3.92
js/src/jsxdrapi.h 1.29

/be

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-375695.js,v  <--  regress-375695.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.