for-in binds name too early




12 years ago
10 years ago


(Reporter: igor, Assigned: igor)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



12 years ago
There is a comment in the interpretr loop at JSOP_FORNAME case:

             * ECMA 12.6.3 says to eval the LHS after looking for properties
             * to enumerate, and bail without LHS eval if there are no props.
             * We do Find here to share the most code at label do_forinloop.
             * If looking for enumerable properties could have side effects,
             * then we'd have to move this into the common code and condition
             * it on op == JSOP_FORNAME.

Since a generator can definitely have side effects, that code must be moved. Here is an example that demonstrates it:

var index;
var obj = { index: 1 };

function gen()
    delete obj.index;
    yield 2;

with (obj) {
  for (index in gen());

if ('index' in obj)
  throw "for-in binds name to early";

if (index !== 2)
  throw "unexpected value of index: "+index;

Currently it fails with:
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js -v 170 ~/s/x.js
uncaught exception: for-in binds name to early

Comment 1

12 years ago
Created attachment 256979 [details] [diff] [review]
Fix and fetch refcatoring

Taking opportunity to refactor fetch_object/fetch_element_id while fixing the bug. The new version eliminates CHECH_ELEMENT_ID macro and removes redundant SAVE_SP_AND_PC. In total the patch shrinks the stripped jsinterp.o in an optimized browser build on Linux with GCC 4.1.1 from 60012 to 58252 bytes.

Moreover, this also fixes a bug in a build without E4X support. There a code  like null[{toString: someFunction}] will invoke someFunction as FETCH_ELEMENT_ID will call the string conversion on id before calling ToObject on the null in violation of ECMA-262v3 11.2.1. With E4X enabled the bug does not exist as the conversion is delayed until CHECK_ELEMENT_ID.
Attachment #256979 - Flags: review?(brendan)
Comment on attachment 256979 [details] [diff] [review]
Fix and fetch refcatoring

Looks good. I smelled this long ago but turned a blind nose to it -- thanks for fixing it. In the old days, the idea was to avoid SAVE_SP(fp) costs, pushing them into else blocks. But with the threaded interpreter and bug fixes to home sp, the bloat you're fixing crept in.


I'll review in more detail in a bit. In the mean time I should ask (sorry if answered in the bug), does this pass the testsuite?



12 years ago
Summary: for-in binds name to early → for-in binds name too early

Comment 3

12 years ago
Created attachment 257098 [details] [diff] [review]
Fix and fetch refactoring v2

Here is a version that fixes misspellings and adds the bug references.
Attachment #256979 - Attachment is obsolete: true
Attachment #257098 - Flags: review?(brendan)
Attachment #256979 - Flags: review?(brendan)

Comment 4

12 years ago
(In reply to comment #2)
> In the mean time I should ask (sorry if
> answered in the bug), does this pass the testsuite?

Yes - I checked this just now.
Comment on attachment 257098 [details] [diff] [review]
Fix and fetch refactoring v2

Looks good, thanks.

I wonder whether SAVE_SP* are worth it in today's x86 world. I mean the pc and sp locals, really, instead of using fp->... all over the place.

Attachment #257098 - Flags: review?(brendan) → review+

Comment 6

12 years ago
I committed the patch from comment 3 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.337; previous revision: 3.336
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 7

12 years ago
/cvsroot/mozilla/js/tests/js1_7/regress/regress-372331.js,v  <--  regress-372331.js
initial revision: 1.1
Flags: in-testsuite+

Comment 8

12 years ago
verified fixed 1.9.0 20070320 win/mac*/linux

Comment 9

10 years ago
modify test to handle uncaught exception
You need to log in before you can comment on or make changes to this bug.