237461 - nested function with same name as declared var botches fun->nvars

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See the news article.  Aleksey's testcase:

function g()
    {
    var core = {};
    core.js = {};
    core.js.init = function()
        {
        var loader = null;
        
        function loader() {}
        };
    return core;
    }
var s = new Script(""+g.toString());
var frozen = s.freeze();              // crash.
print("len:"+frozen.length);

You'll need to hack JS_HAS_XDR_FREEZE_THAW to 1 in jsconfig.h to test.  Patch in
a moment.

/be

Comment 1

[Brendan Eich [:brendan]]

So this breaks XDR.  What else?  Patch now, trying for 1.7b.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix (for applying)

Comment 3

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment (for reviewing)

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 143902 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

This is as safe as anything else for 1.7b.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 143902 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

Oops, that block-local pobj I added shadows a function-wide pobj that I can use
here.  Assume I've fixed that by removing the block-local when reviewing.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 143902 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

Argh, hacking patches between two trees, one with extra changes.  Fixed patches
coming up.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2 -- apply this

Working patch, consolidate prop as well as pobj, elimating thereby-single-use
sprop and avoiding strict aliasing gcc warnings.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment (for reviewing)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 143907 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

Let's try this one on for size.

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 143907 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

Add a test to the suite for this?

Comment 11

[timeless]

brendan: could the normal testsuite notice this some other way than Freeze? if 
not (as i suspect) the test will be fairly useless on average.

Comment 12

[Brendan Eich [:brendan]]

We can add a test area to the suite for XDR.  We'll need to build a special
version of the js shell.

/be

Comment 13

[chris hofmann]

Comment on attachment 143907 [details] [diff] [review]
diff -w version of last attachment (for reviewing)

a=chofmann for 1.7b

Comment 14

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-237461.js

I'll write to Aleksey.

Comment 16

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-237461.js checked in.

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed.