Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({verified1.8.1.4})

Trunk
mozilla1.9alpha1
verified1.8.1.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] freed memory use?)

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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
Attachment #254357 - Flags: review?(mrbkap)
Attachment #254357 - Flags: approval1.8.1.3?
Attachment #254357 - Flags: approval1.8.0.11?
(Assignee)

Comment 1

11 years ago
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

Updated

11 years ago
Attachment #254357 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 2

11 years ago
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?
(Assignee)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
Created attachment 254474 [details] [diff] [review]
fix, v2
Attachment #254474 - Flags: review?(mrbkap)
(Assignee)

Comment 5

11 years ago
Created attachment 254475 [details]
mini-testcase
(Assignee)

Comment 6

11 years ago
This is not needed on the 1.8.0 branch, only on the 1.8 branch.

/be
Flags: blocking1.8.1.3?
(Assignee)

Updated

11 years ago
Blocks: 355044

Updated

11 years ago
Attachment #254474 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 7

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

11 years ago
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?

Comment 9

11 years ago
Created attachment 255368 [details]
js1_7/regress/regress-369666-01.js

Comment 10

11 years ago
Created attachment 255369 [details]
js1_7/regress/regress-369666-02.js

Updated

11 years ago
Flags: in-testsuite+

Comment 11

11 years ago
verified fixed 20070217 1.9.0 windows/mac*/linux

Updated

11 years ago
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?
(Assignee)

Comment 13

11 years ago
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

11 years ago
Looks like the crash is mac only on 1.8.1
(Assignee)

Comment 15

11 years ago
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)

Comment 18

11 years ago
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

11 years ago
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)
(Assignee)

Comment 21

10 years ago
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.
(Assignee)

Comment 23

10 years ago
(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

10 years ago
(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.

Updated

10 years ago
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+

Comment 26

10 years ago
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
Attachment #262184 - Flags: review?(brendan)

Comment 27

10 years ago
Created attachment 262185 [details]
diff between 1.8 patch file and trunk patch file

Comment 28

10 years ago
Comment on attachment 262184 [details] [diff] [review]
hand-merge to 1.8

This is a trivial hand-merge indeed.
Attachment #262184 - Flags: review+

Comment 29

10 years ago
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?

Updated

10 years ago
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+

Comment 31

10 years ago
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

Updated

10 years ago
Keywords: regression → fixed1.8.1.4

Comment 32

10 years ago
verified fixed 1.8.1.4 windows/linux/mac* browser,shell
Keywords: fixed1.8.1.4 → verified1.8.1.4
Group: security

Comment 33

10 years ago
/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.