352873 - Assertion "JSVAL_IS_OBJECT(rval)" or "(jsval *)mark >= sp" involving finally{return} in "with"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> (function() { try { with({}) { try { } finally { return; }  }  } finally {  } })()
Assertion failure: JSVAL_IS_OBJECT(rval), at jsinterp.c:2361

In an opt js shell, this looks like a null deref crash.

Comment 1

[Jesse Ruderman]

Different assertion with a similar testcase but with "with" in a different place.

js> (function(){ try { try { } finally { return; } } finally { with({}) { } } })()
Assertion failure: (jsval *)mark >= sp, at jsinterp.c:1292

In an opt js shell, this doesn't crash or halt execution.

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

This is a regression from bug 350312 (which had a followup bug 350837).  It's a one character fix, not including the comment.  Without it the stack during non-local jumps such as return, break, or continue from finally may be too full by one value per finally block being jumped out of.  In the testcase this is fairly benign because the non-local jump is a return.  Not so for non-return transfers.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix with better comment

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 238821 [details] [diff] [review]
fix with better comment

I bet this fixes that let assertion bug I was looking at earlier.

Comment 5

[Mike Schroepfer]

Comment on attachment 238821 [details] [diff] [review]
fix with better comment

a=schrep

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.212; previous revision: 3.211
done

/be

Comment 7

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 8

[Brendan Eich [:brendan]]

*** Bug 352644 has been marked as a duplicate of this bug. ***

Comment 9

[Bob Clary [:bc] (inactive)]

Checking in regress-352873-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352873-01.js,v  <--  regress-352873-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-352873-02.js,v
done
Checking in regress-352873-02.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352873-02.js,v  <--  regress-352873-02.js
initial revision: 1.1
done

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060919 windows/mac*/linux

Comment 11

[Daniel Veditz [:dveditz]]

restoring lost blocking flag

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 238821 [details] [diff] [review]
fix with better comment

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[Brendan Eich [:brendan]]

Fixed on the 1.8.0 branch:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.3.2.11; previous revision: 3.128.2.3.2.10
done

/be

Comment 14

[Bob Clary [:bc] (inactive)]

1.8.0.8 20060927

Assertion failure: strcmp(rval, retsub_pc_cookie) == 0, at /work/mozilla/builds/ff/1.5.0/mozilla/js/src/jsopcode.c:1237

Comment 15

[Brendan Eich [:brendan]]

There are lots of decompiler bugs not fixed on the 1.8.0 branch.  You can verify that this fixes the assertion failure shown in comment 0, even if the other bugs still bite.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to comment #15)

verified fixed 1.8.0.8 20061003 windows/mac*/linux assuming the new assert doesn't hide the older assert.

Comment 17

[Bob Clary [:bc] (inactive)]

Note that on 1.8.0.9 winxp the assert is now in PopOff

    /* ss->top points to the next free slot; be paranoid about underflow. */
    top = ss->top;
    JS_ASSERT(top != 0);