Last Comment Bug 369666 - inner function declaration in let-induced outer function body block gets wrong scope
: inner function declaration in let-induced outer function body block gets wron...
Status: VERIFIED FIXED
[sg:critical?] freed memory use?
: verified1.8.1.4
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9alpha1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks: js1.7src 357754
  Show dependency treegraph
 
Reported: 2007-02-07 15:59 PST by Brendan Eich [:brendan]
Modified: 2007-08-14 00:57 PDT (History)
6 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x-
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (9.99 KB, patch)
2007-02-07 15:59 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix, v2 (13.28 KB, patch)
2007-02-08 17:07 PST, Brendan Eich [:brendan]
mrbkap: review+
crowderbt: review+
igor: review+
dveditz: approval1.8.1.4+
Details | Diff | Review
mini-testcase (403 bytes, text/plain)
2007-02-08 17:07 PST, Brendan Eich [:brendan]
no flags Details
js1_7/regress/regress-369666-01.js (2.66 KB, text/plain)
2007-02-16 11:55 PST, Bob Clary [:bc:]
no flags Details
js1_7/regress/regress-369666-02.js (2.72 KB, text/plain)
2007-02-16 11:56 PST, Bob Clary [:bc:]
no flags Details
hand-merge to 1.8 (13.52 KB, patch)
2007-04-19 15:27 PDT, Brian Crowder
igor: review+
dveditz: approval1.8.1.4+
Details | Diff | Review
diff between 1.8 patch file and trunk patch file (3.61 KB, text/plain)
2007-04-19 15:28 PDT, Brian Crowder
no flags Details

Description Brendan Eich [:brendan] 2007-02-07 15:59:52 PST
Created attachment 254357 [details] [diff] [review]
fix

The fix for bug 357754 was just wrong: the JSOP_DEFLOCALFUN interpreter case would clone the compiler-created inner function object using a runtime block scope object cloned from the body block, but then throw that object away. Each such local function would get its own unique cloned block for scope. They all would therefore be unable to see the stack-stored values of let variables if any such inner function escaped, and in looking, they would crash.

Related bug found while testing: the splitting of JSOP_RETURN into JSOP_SETRVAL and a later JSOP_SETRVAL for return from finally must also happen for return from a block scope (whether outer body or multiple, nested block scopes). Without this splitting, JSOP_LEAVEBLOCK won't run before return, and if the return value or an out parameter helps the an inner function escape, calling that escapee will run into the first bug.

Testcases:

function foo() {
    let x = 42

    function bar() {
        return x;
    }

    return bar;
}

print(foo()());

baz = false;

function foo2() {
    let x = 42

    function bar() {
        return x;
    }

    try {
        if (baz)
            return bar;
    } finally {
        print('finally', x);
    }
    return bar;
}

print(foo2()());

/be
Comment 1 Brendan Eich [:brendan] 2007-02-07 16:07:00 PST
From comment 0:

> and a later JSOP_SETRVAL for return from finally must also happen for return

s/SETRVAL/RETRVAL/

I tried different approaches to fixing this, always trying to push work out of runtime into compile time. Alternatives considered and rejected:

1. Emit JSOP_ENTERBLOCK for the body block in the script prolog (hard to do in current emitter design; breaks JS_ExecuteScriptPart).

2. Emit JSOP_ENTER/LEAVEBLOCK around prolog as well as body, somehow passing the cloned runtime block scope object from first to second if both prolog and main were being executed (messy again, but does not break JS_ExecuteScriptPart).

3. Reorganize emitter to avoid OBJ_SET_PARENT(cx, fun->object, obj) added by this patch (parser creates function objects in its pass, does not know that a later let induces a body block, so this OBJ_SET_PARENT is unavoidable, even though it couples emitter to parser strongly, i.e., parse once and emit several redundant times will fight to update fun->object's parent -- "don't do this").

SpiderMonkey uses the script prolog to implement the "Entering an Execution Context" semantics in ECMA-262 Edition 3 Section 10.2. This use of bytecode does not mix well with body-block enter and leave bytecode in the main part of the script. The alternative top-down design would not use bytecode for either the "entering an execution context" prolog work *or* the body-block enter and leave. I'm not prepared to move to such an architecture for SpiderMonkey, so I hope this patch is "it".

/be
Comment 2 Brendan Eich [:brendan] 2007-02-08 12:34:25 PST
Comment on attachment 254357 [details] [diff] [review]
fix

Argh, this still bugilly makes a cloned block per JSOP_DEFLOCALFUN, parenting only the cloned function object created by that op.

/be
Comment 3 Brendan Eich [:brendan] 2007-02-08 12:35:37 PST
My testing was before I restored the old block and scope chains in JSOP_DEFLOCALFUN, which meant I was accumulating block clones on the scope chain, so things seemed to work. Without that accumulation, crash-ola trying to convert 0xdadadada in a jsval to a string (in DEBUG builds).

/be
Comment 4 Brendan Eich [:brendan] 2007-02-08 17:07:14 PST
Created attachment 254474 [details] [diff] [review]
fix, v2
Comment 5 Brendan Eich [:brendan] 2007-02-08 17:07:44 PST
Created attachment 254475 [details]
mini-testcase
Comment 6 Brendan Eich [:brendan] 2007-02-08 17:08:29 PST
This is not needed on the 1.8.0 branch, only on the 1.8 branch.

/be
Comment 7 Brendan Eich [:brendan] 2007-02-09 16:46:25 PST
Committed with unit digit transposition error in bug number citation:

js/src/js.c 3.130
js/src/jsemit.c 3.234
js/src/jsemit.h 3.70
js/src/jsfun.h 3.28
js/src/jsinterp.c 3.326
js/src/jsscript.c 3.134

/be
Comment 8 Brendan Eich [:brendan] 2007-02-09 16:47:05 PST
Comment on attachment 254474 [details] [diff] [review]
fix, v2

Needed fix in due course, if not sooner.

/be
Comment 9 Bob Clary [:bc:] 2007-02-16 11:55:47 PST
Created attachment 255368 [details]
js1_7/regress/regress-369666-01.js
Comment 10 Bob Clary [:bc:] 2007-02-16 11:56:22 PST
Created attachment 255369 [details]
js1_7/regress/regress-369666-02.js
Comment 11 Bob Clary [:bc:] 2007-02-19 13:02:53 PST
verified fixed 20070217 1.9.0 windows/mac*/linux
Comment 12 Daniel Veditz [:dveditz] 2007-02-27 14:46:32 PST
On the 1.8 branch I don't see any crashes.
Comment 13 Brendan Eich [:brendan] 2007-02-27 15:14:37 PST
I do:

Yoyodyne:~/Hacking/trunk/mozilla/js/srcmoz18 brendaneich$ !gdb
gdb Darwin_DBG.OBJ/js
GNU gdb 6.1-20040303 (Apple version gdb-437) (Sun Dec 25 08:26:53 GMT 2005)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-apple-darwin"...Reading symbols for shared libraries .... done

(gdb) r -v 170 ../src.bench/foo.js
Starting program: /Users/brendaneich/Hacking/trunk/mozilla/js/srcmoz18/Darwin_DBG.OBJ/js -v 170 ../src.bench/foo.js
Reading symbols for shared libraries .. done

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xdadadad8
0x000c96f1 in js_ValueToString (cx=0x500210, v=-623191334) at jsstr.c:2689
2689            str = js_NumberToString(cx, *JSVAL_TO_DOUBLE(v));
(gdb) q
The program is running.  Exit anyway? (y or n) y  

Are you testing a JS shell?

/be
Comment 14 Bob Clary [:bc:] 2007-02-27 16:35:14 PST
Looks like the crash is mac only on 1.8.1
Comment 15 Brendan Eich [:brendan] 2007-02-27 17:03:13 PST
Try optimized js shell builds on other platforms -- I would be surprised if some did not crash reproducibly.

/be
Comment 16 Daniel Veditz [:dveditz] 2007-03-21 11:28:39 PDT
Comment on attachment 254474 [details] [diff] [review]
fix, v2

Can we get a second review before landing this on the branch? We're a little regression-shy after 2.0.0.2
Comment 17 Daniel Veditz [:dveditz] 2007-03-30 11:17:20 PDT
Comment on attachment 254474 [details] [diff] [review]
fix, v2

no response from Igor, try Brian for the second review
Comment 18 Brian Crowder 2007-03-30 12:06:33 PDT
I'm glad to review this and experiment with the patch, but Igor's examination of this code will be more productive than mine, most likely.
Comment 19 Brian Crowder 2007-03-30 13:38:44 PDT
Comment on attachment 254474 [details] [diff] [review]
fix, v2

rubber-stamp
Comment 20 Daniel Veditz [:dveditz] 2007-04-02 10:46:47 PDT
Comment on attachment 254474 [details] [diff] [review]
fix, v2

rubber-stamp doesn't really help assuage our fears of a regression with this size change in this critical section of code. Back to trying Igor for the second review, I guess
Comment 21 Brendan Eich [:brendan] 2007-04-02 10:53:12 PDT
I'm happy if Igor reviews, however I think at this point the concern should be how well tested the fix was.

Please also notice that this bug is about let declarations, new in JS1.7. Not likely to break the web at this point. You can inspect the patch with more context to see that all of the opcodes affected are specific to BLOCKs (or LET), except for JSOP_DEFLOCALFUN. Close reading of that case shows no change for the pre-JS1.7 case of a local function defined with no let declaration at the same body-block scope as the local function.

/be
Comment 22 Daniel Veditz [:dveditz] 2007-04-04 10:55:36 PDT
Good points, so how well tested was the fix?

The triage team doesn't have the time for "close reading" of patches, and the js engine code is more specialized than most.
Comment 23 Brendan Eich [:brendan] 2007-04-04 12:02:40 PDT
(In reply to comment #22)
> Good points, so how well tested was the fix?

Enough to confirm the bug doesn't reproduce, variations on the testcase don't break, and the JS testsuite including other let tests passes.

> The triage team doesn't have the time for "close reading" of patches, and the
> js engine code is more specialized than most.

So delegate to the reviewer. Asking for a second review won't get you any more close of a reading, or the needed explanation of what the hunks affect (which I gave, finally -- sorry for not doing it sooner). Just don't keep returning the bug for more generalized review, esp. with the new guy (sorry Crowder, but I'm first to admit it takes a while to learn SpiderMonkey's ins and outs).

I'm sure this isn't the last block-scope related bug (we have another on file), but it's a good fix and the other bugs are just other bugs to fix and consider for the branch later. Life goes on.

/be
Comment 24 Igor Bukanov 2007-04-04 12:19:28 PDT
(In reply to comment #20)
> (From update of attachment 254474 [details] [diff] [review])
> rubber-stamp doesn't really help assuage our fears of a regression with this
> size change in this critical section of code. Back to trying Igor for the
> second review, I guess

I think I will give r+ tomoroow for the patch, but I need to digest it first.
Comment 25 Daniel Veditz [:dveditz] 2007-04-09 17:49:52 PDT
Comment on attachment 254474 [details] [diff] [review]
fix, v2

approved for 1.8.1.4, a=dveditz for release-drivers
Comment 26 Brian Crowder 2007-04-19 15:27:50 PDT
Created attachment 262184 [details] [diff] [review]
hand-merge to 1.8

Pretty trivial hand-merge to land on 1.8, diff-of-diffs coming next
Comment 27 Brian Crowder 2007-04-19 15:28:30 PDT
Created attachment 262185 [details]
diff between 1.8 patch file and trunk patch file
Comment 28 Igor Bukanov 2007-04-20 08:15:25 PDT
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

This is a trivial hand-merge indeed.
Comment 29 Brian Crowder 2007-04-20 11:32:58 PDT
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

dveditz:  This is a super-trivial hand-merge to 1.8
Comment 30 Daniel Veditz [:dveditz] 2007-04-20 11:41:47 PDT
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

approved for 1.8.1.4, a=dveditz
Comment 31 Brian Crowder 2007-04-20 11:46:48 PDT
js.c:       3.93.2.15
jsemit.c:   3.128.2.72
jsemit.h:   3.38.20.20
jsfun.h:    3.22.20.4
jsinterp.c: 3.181.2.89
jsscript.c: 3.79.2.24
Comment 32 Bob Clary [:bc:] 2007-05-02 12:16:45 PDT
verified fixed 1.8.1.4 windows/linux/mac* browser,shell
Comment 33 Bob Clary [:bc:] 2007-06-14 15:43:35 PDT
/cvsroot/mozilla/js/tests/js1_7/regress/regress-369666-01.js,v  <--  regress-369666-01.js
initial revision: 1.1

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

Note You need to log in before you can comment on or make changes to this bug.