Closed
Bug 252892
Opened 20 years ago
Closed 20 years ago
for (var i in o) in heavyweight function f does not define i in f's activation
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8alpha3
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(4 files, 4 obsolete files)
374 bytes,
patch
|
Details | Diff | Splinter Review | |
2.45 KB,
text/plain
|
Details | |
15.53 KB,
patch
|
shaver
:
review+
asa
:
approval-aviary+
mkaply
:
approval1.7.5+
|
Details | Diff | Splinter Review |
4.20 KB,
text/plain
|
Details |
Bad bug, shows longstanding lack of testsuite coverage for the testcase (attached next). Patch soon. /be
Assignee | ||
Comment 1•20 years ago
|
||
News URL: news://news.mozilla.org:119/27adbc6a.0407231732.2e798327@posting.google.com (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
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha3
Assignee | ||
Comment 2•20 years ago
|
||
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
Assignee | ||
Comment 3•20 years ago
|
||
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
Attachment #154198 -
Attachment is obsolete: true
Assignee | ||
Comment 4•20 years ago
|
||
Man, this kicked my ass! Easy, forsooth! Ok, shaver will love this. /be
Assignee | ||
Comment 5•20 years ago
|
||
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
Attachment #154220 -
Flags: review?(shaver)
Assignee | ||
Comment 6•20 years ago
|
||
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
Attachment #154220 -
Attachment is obsolete: true
Assignee | ||
Comment 7•20 years ago
|
||
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
Attachment #154423 -
Flags: review?(shaver)
Assignee | ||
Updated•20 years ago
|
Attachment #154220 -
Flags: review?(shaver)
Assignee | ||
Comment 8•20 years ago
|
||
Comment on attachment 154423 [details] [diff] [review] even better fix Argh, buggy. Plus, I can make it even smaller. /be
Attachment #154423 -
Attachment is obsolete: true
Attachment #154423 -
Flags: review?(shaver)
Assignee | ||
Comment 9•20 years ago
|
||
I'll attach a js shell testcase next. /be
Assignee | ||
Comment 10•20 years ago
|
||
Comment on attachment 154461 [details] [diff] [review] best fix More caffeine needed -- almost right. /be
Attachment #154461 -
Attachment is obsolete: true
Assignee | ||
Comment 11•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #154464 -
Attachment is patch: false
Assignee | ||
Comment 12•20 years ago
|
||
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
Assignee | ||
Comment 13•20 years ago
|
||
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
Attachment #154469 -
Flags: review?(shaver)
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.
Attachment #154469 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 15•20 years ago
|
||
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
Flags: blocking1.7.2?
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
Assignee | ||
Comment 16•20 years ago
|
||
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
Attachment #154469 -
Flags: approval1.7.2?
Attachment #154469 -
Flags: approval-aviary?
Comment 17•20 years ago
|
||
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.
Attachment #154469 -
Flags: approval-aviary? → approval-aviary+
Assignee | ||
Comment 18•20 years ago
|
||
Fixed on trunk and aviary branch. Cc'ing mkaply and dveditz for some 1.7.2 love. /be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Comment 19•20 years ago
|
||
Comment on attachment 154469 [details] [diff] [review] best fix, take 2 a=mkaply for 1.7.3
Attachment #154469 -
Flags: approval1.7.3? → approval1.7.3+
Assignee | ||
Comment 20•20 years ago
|
||
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
Keywords: fixed1.7
Updated•20 years ago
|
Flags: blocking1.7.5?
Comment 21•19 years ago
|
||
thanks to be.
Comment 22•19 years ago
|
||
js1_5/Regress/regress-252892.js checked in.
Updated•19 years ago
|
Flags: testcase+
Comment 23•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
failures on all three platforms going back to firefox 1.0/1.7.5
Keywords: fixed-aviary1.0,
fixed1.7
Comment 26•18 years ago
|
||
verified fixed 1.8.0, 1.8, 1.9 20060818 win/mac*/linux
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•