292731 - for-in statement calls valueOf method

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Yuh-Ruey Chen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502 Firefox/1.0+

If you run an for-in statement on an object, even an empty for-in statement, the
object's valueOf method will be called.

I looked in the ECMAScript 3rd ed spec, and I can't see why the valueOf method
should be called here.  IE and Opera don't call it.

I wondered if the evaluating of the object expression in the for-in calls the
valueOf, but when I tried an if statement that should do the same type of
evaluation (comparing the object reference to window), valueOf isn't called.

Reproducible: Always

Steps to Reproduce:
1) Create an object that has a custom valueOf method.
2) for (prop in obj);

Actual Results:  
valueOf method is called

Expected Results:  
valueOf method should not be called

Comment 1

[Yuh-Ruey Chen]

Attachment: test case

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: testcase (without alerts)

Reading <http://bclary.com/2004/11/07/ecma-262#a-12.6.4>, the case for
valueOf() being called could be made. Brendan?

Comment 3

[Brendan Eich [:brendan]]

This is a very old extension (over 8 years?) and I'm loath to change it now.  It
may be disallowed by ECMA-262 Edition 3 Section 16, but I haven't yet attempted
a close reading of that section applied to this specific case.  What exactly is
the problem it causes?

/be

Comment 4

[Yuh-Ruey Chen]

It looks like the with and for-in statements have the exact same processing of
the object expression:

1. Evaluate Expression.

2. Call GetValue(Result(1)).

3. Call ToObject(Result(2)).

And the with statement does not call valueOf.

(In reply to comment #3)
> This is a very old extension (over 8 years?) and I'm loath to change it now.  It
> may be disallowed by ECMA-262 Edition 3 Section 16, but I haven't yet attempted
> a close reading of that section applied to this specific case.  What exactly is
> the problem it causes?
> 
> /be

No problem, but why would it need to call valueOf?

Comment 5

[Brendan Eich [:brendan]]

The engine is inconsistent about this, as well as old, pre-ECMA.  Ok, let's try
changing this for 1.8.

/be

Comment 6

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/Statements/12.6.3.js,v  <--  12.6.3.js
initial revision: 1.1

Comment 7

[Brendan Eich [:brendan]]

Actually, I think fixing this will break the patch for ambiguous scriptable
plugin members that Pete Zha has prepared in bug 293972.  API compatibility
means never having to say you're sorry, because you never break old APIs.

So I would rather change with's implementation to match for-in's.  Comments?

/be

Comment 8

[Yuh-Ruey Chen]

FYI, accessing properties also has the same 3 steps:

3. Evaluate Expression.

4. Call GetValue(Result(3)).

5. Call ToObject(Result(2)).

http://bclary.com/2004/11/07/ecma-262#a-11.2.1

(In reply to comment #7)
> Actually, I think fixing this will break the patch for ambiguous scriptable
> plugin members that Pete Zha has prepared in bug 293972.  API compatibility
> means never having to say you're sorry, because you never break old APIs.
> 
> So I would rather change with's implementation to match for-in's.  Comments?
> 
> /be

I'm fine with that.  If I were to define a custom valueOf for an object, I would
never let it set or do anything and would just return something instead.  Since
the for-in discards the value returned from valueOf, there should be no side
effects, except a bit of inefficiency.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Since the for-in discards the value returned from valueOf, there should be no
> side effects, except a bit of inefficiency.

for-in code does not discard an object reference returned from
valueOf("object"), though -- that case works as intended.

js> o = {valueOf: function (hint) { if (hint == "object") return [4,5,6] }};
[object Object]
js> for (i in o) print(i)
0
1
2
js> for each (v in o) print(v)
4
5
6

/be

Comment 10

[Asa Dotzler [:asa]]

brendan, do we still need to block on this? 

Comment 11

[Brendan Eich [:brendan]]

mrbkap has kindly volunteered to take this.  It's easy to fix.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: always call valueOf

This is, I think, the solution that Brendan and I were talking about earlier.
Now, instead of only sometimes calling valueOf, we always call it. Consistancy
is the winner with this patch. Now we should always call valueOf.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 193102 [details] [diff] [review]
always call valueOf

r+a=me, thanks!

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH and trunk.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This looks like a bit of a Tp hit (0.5-1%)

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I just backed out the fix for this bug and checked in a fix that did the
opposite (applying the optimization in more places), since the original fix
caused the Tp hit. Reopening until we know what the numbers from the most recent
checkin look like.

Comment 17

[Brendan Eich [:brendan]]

Note that ECMA never calls [[DefaultValue]] (which calls valueOf(hint)) with
hint "object", so we are now thinking about breaking a SpiderMonkey extension of
very long standing, in order to restore the lost Tp perf (and perhaps even gain
a bit), and to match ECMA.

The latter is really more for future implementations of JS we care about that
might have to ship in products using SpiderMonkey.  I'm all in favor of reducing
the extensions required in such scenarios.  Let's not require Narcissus to
support the "object" hint.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: new patch

This is the patch that was checked into trunk. It restores the optimization and
uses it in more places. Looking for approval.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Marking as FIXED and removing fixed1.8, since this there is (for now) an
inconsistancy between the trunk and the branch that will be solved with this
checkin.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 193512 [details] [diff] [review]
new patch

This is better.  Let's see if anyone notices (the way for..in called
valueOf("object") was odd and did not match other such cases as noted in this
bug, so I'd be surprised).

/be

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

attachment 193512 [details] [diff] [review] checked into MOZILLA_1_8_BRANCH.

Comment 22

[James Ross]

(In reply to comment #20)
> (From update of attachment 193512 [details] [diff] [review] [edit])
> This is better.  Let's see if anyone notice

ChatZilla noticed, rather badly. The code often does this:
  if (typeof foo == "object")
    for (var p in foo)
which obviously breaks now, since |null| is typeof "object", but fails in the
for..in with a TypeError.

The problem is really a conflict within the ECMA spec itself. It specifically
defines |typeof null| as "object" (11.4.3), and yet also specifically says
ToObject(null) should throw a TypeError (9.9). That just doesn't make sense.
Either the null value is an object or it isn't.

Now, I don't expect anyone to care, but really, the spec is only making things
worse here. Some parts of the spec, like 15.2.1.1 (calling Object as a
function), specifically handle null and undefined separately from the normal
code path of calling ToObject. The for..in bit doesn't care. What would it take
to just make for..in of null not enumerate anything, like it used to? (or,
alternatively, make |typeof null| return something else? I suspect this is very
unlikely to ever happen, though)

Comment 23

[Mike Shaver (:shaver emeritus)]

Would

  if (foo instanceof Object)

work for you?  It's false for null (which is not an "instance of" anything, and
more importantly for the JS world does not delegate to anything) and true for
all the objects I've been able to test with.

An alternative might be to enumerate over Object(foo), which will promote null
to a new, empty object at some small performance cost.

That typeof returns "object" for null has been a source of quite some pain in
ECMA and JS, but it's not really something we can change any more.  Changing the
meaning of ToObject(null) to matching |Object(null)| would have repercussions
throughout the spec, including the behaviour of this code:

  var foo = null;
  var bar = foo.x;

which would then give bar the value |undefined|, instead of throwing an error.

But IE and Safari have the pre-292731 behaviour, according to my quick tests and
requests on IRC for help, so this might cause us to regress in terms of web
compatibility.  Thanks to Silver for being the canary in that coal mine.

Comment 24

[James Ross]

Thanks for the suggestions, I'll try some of them later. instanceof is not a
workable solution, though (I'm not sure yet if it is just the separate window
issue, or something else, but it is returning false for critical ChatZilla
objects that really are objects :) ).

My personal opinion on dealing with the for..in of null would be to add a
special case, much like Object(foo) does. Of course, this would be nothing more
than a compatibility hack (unless the spec was declared wrong or updated).

I also noticed that for..in of undefined used to work - is that also failing
now? (I know of some code that only got review because undefined was ok in for..in)

Now if only today's Firefox build didn't just die with JS errors on startup. :(

Comment 25

[Mike Shaver (:shaver emeritus)]

We very recently (last 2-3 days) fixed some issues with wrapper identity and
such across scopes that were severed by the split windows, so you might try
instanceof again.

undefined will throw as well, today, for the same ECMA conformance reason as
with null.

I really think we should at least take this off the branch until we can assess
the compatibility effects.

Comment 26

[James Ross]

I'm testing the instanceof stuff in 2005071504, which is pre-split window, but
post bug 254067 (which is the thing I was refering to in my comment). That bug
really messed up ChatZilla, and made instanceof virtually unusable in it, so it
may just be that. I think we have no choice but to take the performance hit of
doing |for...in| of |Object(foo)|.

Comment 27

[Brendan Eich [:brendan]]

Compatibility is king, and this change seems to have caused bug 305724.

We should make for..in tolerate null (and undefined?  That would restore the
lost compatibility completely, and why take chances at this point?).

Silver: sorry this broke you, I'm actually working the ECMA TG1 group in charge
of Edition 4 of the JS standard (ECMA-262) to fix it so typeof null === "null".
 That means Edition 4 for (i in null); will throw, again.  But any JS2/ES4 usage
will require an explicit, opt-in version selection.

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: restore the desired lost compat...

...  while still avoiding valueOf hook-calling (and at the same time not again
deoptimizing JSOP_TOOBJECT by losing the !JSVAL_IS_PRIMITIVE(v) fast path).

mrbkap says r=him.  I'm checking this into the branch and trunk.

/be

Comment 29

[Brendan Eich [:brendan]]

Fixed on trunk and MOZILLA_1_8_BRANCH.

/be

Comment 30

[Brendan Eich [:brendan]]

*** Bug 305724 has been marked as a duplicate of this bug. ***

Comment 31

[Scott MacGregor]

For historical reference & duping purposes, the original patch on the 1.8 branch
broke the Account Manager for Thunderbird and suite mail. I've verified that
Brendan's patch from this morning fixes that problem. See Bug 305622.

Comment 32

[Boris Zbarsky [:bzbarsky]]

For reference, XPCNativeWrapper is not instanceof Object, and in general a JS
object created in one scope is not instanceof Object in another scope.  For example:

(window.frames[0].document instanceof Object)

is false no matter what while

(window.frames[0].document instanceof window.frames[0].Object)

will generally be true, modulo the split-window stuff and assuming there are no
XPCNativeWrappers in sight.

Comment 33

[Bob Clary [:bc] (inactive)]

v 1.8.0.1, 1.8, 1.9a1 win/linux/mac

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 333730 has been marked as a duplicate of this bug. ***