372331 - for-in binds name too early

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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

[Igor Bukanov]

Attachment: 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.

Comment 2

[Brendan Eich [: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.

s/fecth/fetch/g

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?

/be

Comment 3

[Igor Bukanov]

Attachment: Fix and fetch refactoring v2

Here is a version that fixes misspellings and adds the bug references.

Comment 4

[Igor Bukanov]

(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 5

[Brendan Eich [:brendan]]

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.

/be

Comment 6

[Igor Bukanov]

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
done

Comment 7

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-372331.js,v  <--  regress-372331.js
initial revision: 1.1

Comment 8

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070320 win/mac*/linux

Comment 9

[Bob Clary [:bc] (inactive)]

modify test to handle uncaught exception
http://hg.mozilla.org/mozilla-central/rev/dbc69d9720c2