Closed Bug 369666 Opened 17 years ago Closed 17 years ago

inner function declaration in let-induced outer function body block gets wrong scope

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: brendan, Assigned: brendan)

References

Details

(Keywords: verified1.8.1.4, Whiteboard: [sg:critical?] freed memory use?)

Attachments

(6 files, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
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
Attachment #254357 - Flags: review?(mrbkap)
Attachment #254357 - Flags: approval1.8.1.3?
Attachment #254357 - Flags: approval1.8.0.11?
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
Attachment #254357 - Flags: review?(mrbkap) → review+
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
Attachment #254357 - Attachment is obsolete: true
Attachment #254357 - Flags: review+
Attachment #254357 - Flags: approval1.8.1.3?
Attachment #254357 - Flags: approval1.8.0.11?
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
Attached patch fix, v2Splinter Review
Attachment #254474 - Flags: review?(mrbkap)
Attached file mini-testcase
This is not needed on the 1.8.0 branch, only on the 1.8 branch.

/be
Flags: blocking1.8.1.3?
Blocks: js1.7src
Attachment #254474 - Flags: review?(mrbkap) → review+
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 254474 [details] [diff] [review]
fix, v2

Needed fix in due course, if not sooner.

/be
Attachment #254474 - Flags: approval1.8.1.3?
Flags: in-testsuite+
verified fixed 20070217 1.9.0 windows/mac*/linux
Status: RESOLVED → VERIFIED
On the 1.8 branch I don't see any crashes.
Blocks: 357754
No longer depends on: 357754
Whiteboard: [sg:critical?] freed memory use? trunk only?
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
Looks like the crash is mac only on 1.8.1
Try optimized js shell builds on other platforms -- I would be surprised if some did not crash reproducibly.

/be
Flags: blocking1.8.1.4? → blocking1.8.1.4+
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
Attachment #254474 - Flags: review?(igor)
Whiteboard: [sg:critical?] freed memory use? trunk only? → [sg:critical?] freed memory use?
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x-
Comment on attachment 254474 [details] [diff] [review]
fix, v2

no response from Igor, try Brian for the second review
Attachment #254474 - Flags: review?(igor) → review?(crowder)
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 on attachment 254474 [details] [diff] [review]
fix, v2

rubber-stamp
Attachment #254474 - Flags: review?(crowder) → review+
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
Attachment #254474 - Flags: review?(igor)
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
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.
(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
(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.
Attachment #254474 - Flags: review?(igor) → review+
Comment on attachment 254474 [details] [diff] [review]
fix, v2

approved for 1.8.1.4, a=dveditz for release-drivers
Attachment #254474 - Flags: approval1.8.1.4? → approval1.8.1.4+
Pretty trivial hand-merge to land on 1.8, diff-of-diffs coming next
Attachment #262184 - Flags: review?(brendan)
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

This is a trivial hand-merge indeed.
Attachment #262184 - Flags: review+
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

dveditz:  This is a super-trivial hand-merge to 1.8
Attachment #262184 - Flags: review?(brendan)
Attachment #262184 - Flags: approval1.8.1.4?
Attachment #262184 - Flags: approval1.8.0.12?
Attachment #262184 - Flags: approval1.8.0.12?
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

approved for 1.8.1.4, a=dveditz
Attachment #262184 - Flags: approval1.8.1.4? → approval1.8.1.4+
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
Keywords: regressionfixed1.8.1.4
verified fixed 1.8.1.4 windows/linux/mac* browser,shell
Group: security
/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
Flags: wanted1.8.1.x? → wanted1.8.1.x+
You need to log in before you can comment on or make changes to this bug.