252892 - for (var i in o) in heavyweight function f does not define i in f's activation

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Bad bug, shows longstanding lack of testsuite coverage for the testcase
(attached next).  Patch soon.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: js shell testcase from newsgroup post

News URL:
news://news.mozilla.org:119/[e-mail]
(Google Groups URL:
http://groups.google.ca/groups?hl=en&lr=&ie=UTF-8&frame=right&th=282762e2660ebaa7&seekm=27adbc6a.0407231732.2e798327%40posting.google.com#link2).


/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix based on the JS_150_RC6a tag (applies to trunk too)

Easy fix, very old logic bug in the code generator.  Dates back to the
introduction of JSOP_DEFVAR, where this for/in case was not revisited to ensure
that the js_EmitTree TOK_VAR case code ran for a for/in loop starting with a
declared but uninitialized as well as declared and initialized variable.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 154198 [details] [diff] [review]
proposed fix based on the JS_150_RC6a tag (applies to trunk too)

Oops, testsuite discloses a bug in this patch.	v2 patch soon.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: better fix

Man, this kicked my ass!  Easy, forsooth!

Ok, shaver will love this.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 154220 [details] [diff] [review]
better fix

- EmitAtomOp returns its ale, so the new code under TOK_FOR can pass
ALE_INDEX(ale) to MaybeEmitPrologOp.

- Common subroutine MaybeEmitPrologOp for the two cases where we need its guts:
the pre-existing TOK_VAR case in js_EmitTree, and now the TOK_FOR for (var x in
o) case (but *not* the for (var x = i in o) case).

- In the for (var x = i in o) case, where the js_EmitTree called recursively to
emit code for var x = i before the loop has quickened x into a local var or
arg, we would wrongly emit JSOP_FORNAME (another bug, not reported here). We
must fix this by testing (pn3->pn_slot >= 0) and rewriting op to be a JSOP_FOR*
variant.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: even better fix

Size comparison shows this patch winning (this patch is >, previous patch is
<):

49c49
<   29201	    264       0   29465    7319 jsemit.o (ex
Linux_All_OPT.OBJ/libjs.a)
---
>   28857	    264       0   29121    71c1 jsemit.o (ex
Linux_All_OPT.OBJ/libjs.a)
62c62
<   22015	      8       0   22023    5607 jsparse.o (ex
Linux_All_OPT.OBJ/libjs.a)
---
>   22031	      8       0   22039    5617 jsparse.o (ex
Linux_All_OPT.OBJ/libjs.a)
72c72
<  443372	  10068     236  453676   6ec2c Linux_All_OPT.OBJ/libjs.so
---
>  442608	  10068     236  452912   6e930 Linux_All_OPT.OBJ/libjs.so

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 154423 [details] [diff] [review]
even better fix

The use of |= PNX_* is future-proofing, so if there are new flags set earlier,
they won't be stomped and whoever adds them doesn't have to check all
downstream code.  You can see this slightly increased jsparse.o's size (that
was a -O2 Makefile.ref build).	But the jsemit.c flag-based code is
significantly smaller than the code-ier previous patch.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 154423 [details] [diff] [review]
even better fix

Argh, buggy.  Plus, I can make it even smaller.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: best fix

I'll attach a js shell testcase next.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 154461 [details] [diff] [review]
best fix

More caffeine needed -- almost right.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: js shell testcase

Comment 12

[Brendan Eich [:brendan]]

Attachment: best fix, take 2

This is it.  Passes the test, and here is the size diff vs. the initial patch
in this bug:

49c49
<   29201	    264       0   29465    7319 jsemit.o (ex
Linux_All_OPT.OBJ/libjs.a)
---
>   28857	    264       0   29121    71c1 jsemit.o (ex
Linux_All_OPT.OBJ/libjs.a)
62c62
<   22015	      8       0   22023    5607 jsparse.o (ex
Linux_All_OPT.OBJ/libjs.a)
---
>   22031	      8       0   22039    5617 jsparse.o (ex
Linux_All_OPT.OBJ/libjs.a)
72c72
<  443372	  10068     236  453676   6ec2c Linux_All_OPT.OBJ/libjs.so
---
>  442608	  10068     236  452912   6e930 Linux_All_OPT.OBJ/libjs.so

/be

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 154469 [details] [diff] [review]
best fix, take 2

Commented, tested with test attached (but who will update the suite?  I miss
you, pschwartau!).

/be

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 154469 [details] [diff] [review]
best fix, take 2

The additional control coupling between the parser and the TOK_VAR emission
code doesn't thrill me to my very core, but it's a pretty minimal fix, and even
preserves the nutso |for (var x = i in o)| case.  r=shaver.

Comment 15

[Brendan Eich [:brendan]]

That coupling is ok on balance.  At least the parser fully describes subtrees in
ways that can be unambiguously interpreted.  Again, the complexity here arises
from doing prolog JSOP_DEFVAR etc. code generation during the main code-gen tree
walk.  Hoisting vardecls into a per-script-node list as Narcissus does cleans
this up, at the cost of storage for the list.

Anyway, I'm going to check a trunk version of the patch in tomorrow. I'd like to
get this fix into aviary and 1.7.x, and make a JS15_RC6b or some such.  I'll
ping Phil about the latter.

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 154469 [details] [diff] [review]
best fix, take 2

Very old code-generation bug, may as well try to fix it almost everywhere.

/be

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 154469 [details] [diff] [review]
best fix, take 2

a=asa (on behalf of the Aviary drivers) for checkin to the aviary branch.

Comment 18

[Brendan Eich [:brendan]]

Fixed on trunk and aviary branch.  Cc'ing mkaply and dveditz for some 1.7.2 love.

/be

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 154469 [details] [diff] [review]
best fix, take 2

a=mkaply for 1.7.3

Comment 20

[Brendan Eich [:brendan]]

Checked into MOZILLA_1_7_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.96.2.2; previous revision: 3.96.2.1
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.101.2.1; previous revision: 3.101
done
Checking in jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.23.2.1; previous revision: 3.23
done

/be

Comment 21

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-252892.js

thanks to be.

Comment 22

[Bob Clary [:bc] (inactive)]

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

Comment 23

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-252892.js fails ff 1.0.8/1.7.13_20060214 on winxp. Not sure when it regressed yet.

Comment 24

[Bob Clary [:bc] (inactive)]

this fails in firefox 1.0/winxp as well. You can test in the browser using:
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/Regress/regress-252892.js;language=language;javascript>

Comment 25

[Bob Clary [:bc] (inactive)]

failures on all three platforms going back to firefox 1.0/1.7.5

Comment 26

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0, 1.8, 1.9 20060818 win/mac*/linux