for-in binds name too early

VERIFIED FIXED

Status

()

VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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
(Assignee)

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.

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
(Assignee)

Updated

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

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)
(Assignee)

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.

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

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
done
Status: NEW → RESOLVED
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
Status: RESOLVED → VERIFIED

Comment 9

10 years ago
modify test to handle uncaught exception
http://hg.mozilla.org/mozilla-central/rev/dbc69d9720c2
You need to log in before you can comment on or make changes to this bug.