131348 - |var obj; for (item in obj) {}| causes error

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Schwartau]

Reported by [e-mail]:


We tracked down our problem with 0.9.9. It was in a for..in statement:

    for (item in object) {
        ....
    }


In this case, the value of |object| was |undefined|. In most implementations, 
this causes the for loop to finish immediately. On 0.9.9, this causes an error.

I am not sure that what 0.9.9 is doing is wrong, but it will have an impact
on portability. If I were you, I would go back to the friendlier behavior.

Comment 1

[Phil Schwartau]

Try this javascript: URL

          javascript: for (item in undefined) {}; alert("Hi");

In NN4.7 and IE6, this succeeds and you get the alertbox.
In Mozilla, you get no alertbox, but an error in the JS Console:


          Error: undefined has no properties
          Source File: javascript: for (item in undefined) {}; alert("Hi");
          Line: 1


On the face of it, this is per spec. But I will let the developers
decide what they think of this one. 


NOTE: If you try the reported line, NONE of the three browsers
shows the alertbox:

          javascript: for (item in obj) {}; alert("Hi");


So I'm not sure what portability issues there are here -

Comment 2

[Phil Schwartau]

Here is the spec; ECMA-262 Edition 3 Final. The key step is number 5.


11.8.7 The in operator
The production RelationalExpression : RelationalExpression in ShiftExpression
is evaluated as follows:

1. Evaluate RelationalExpression.
2. Call GetValue(Result(1)).
3. Evaluate ShiftExpression.
4. Call GetValue(Result(3)).
5. If Result(4) is not an object, throw a TypeError exception.
6. Call ToString(Result(2)).
7. Call the [[HasProperty]] method of Result(4) with parameter Result(6).
8. Return Result(7).

Comment 3

[Phil Schwartau]

I suppose this might be what [e-mail] is running into:

    javascript: var obj; for (item  in obj) {}; alert("Hi");


That is, |obj| has been declared, but not initialized.
Here are the results from the various browsers:


NN4.7, IE6 :  both show the alertbox. No error.
Mozilla    :  no alertbox, and this error:


    Error: undefined has no properties
    Source File: javascript: var obj; for (item  in obj) {}; alert("Hi");
    Line: 1

Comment 4

[Boris Zbarsky [:bzbarsky]]

This was a purposeful change a month or two back to comply with the part of the
spec Phil cites...

Comment 5

[Brendan Eich [:brendan]]

ECMA 262 Edition 3 12.6.4 step 3 leads to 9.9, where the first two table entries
specify that a TypeError be thrown.  A fix for this standards compliance bug
shipped in mozilla0.9.9.

Cc'ing waldemar in case the working group might reconsider in light of the
longstanding Netscape (at least; dunno about IE) behavior.

/be

Comment 6

[Brendan Eich [:brendan]]

/me is red-faced and resolves to read the bug, not just the nearest bugmail
message in his Inbox...

/be

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

FWIW, this was changed in bug 121744 - "JS should error on |for(i in
undefined)|, |for(i in null)|"

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 9

[Mike Shaver (:shaver emeritus)]

Phil: it looks like you're quoting the standard portion dealing with the binary
|in| operator, which tests the existence of a property (|if ("p" in o)|), rather
than the portion dealing with object enumeration (| for (p in o)|).

I don't have my ECMA to hand, so I can't offer a better quote at the moment.

Comment 10

[Huazhang Shao]

The "standards compliance fix" in Mozilla 0.9.9 might be a disaster to existing
JavaScript code.

In my project, I will have to manually change more than one hundred lines in
about twenty JavaScript files. My hope is that this change will not be included
in any official Netscape 6.x release.

Comment 11

[Phil Schwartau]

Here is the ECMA spec regarding |for-in|

12.6.4 The for-in Statement
The production IterationStatement :
                         for ( LeftHandSideExpression in Expression ) Statement
is evaluated as follows:

1.  Evaluate the Expression.
2.  Call GetValue(Result(1)).
3.  Call ToObject(Result(2)).
4.  Let V = empty.
5.  Get the name of the next property of Result(3) that doesn’t have the
    DontEnum attribute. If there is no suchproperty, go to step 14.
6.  Evaluate the LeftHandSideExpression ( it may be evaluated repeatedly).
7.  Call PutValue(Result(6), Result(5)).
8.  Evaluate Statement.
9.  If Result(8).value is not empty, let V = Result(8).value.
10. If Result(8).type is break and Result(8).target is in the current
    label set, go to step 14.
11. If Result(8).type is continue and Result(8).target is in the current
    label set, go to step 5.
12. If Result(8) is an abrupt completion, return Result(8).
13. Go to step 5.
14. Return (normal, V, empty).


The production IterationStatement : 
                     for ( var VariableDeclarationNoIn in Expression )Statement
is evaluated as follows:

1.  Evaluate VariableDeclarationNoIn.
2.  Evaluate Expression.
3.  Call GetValue(Result(2)).
4.  Call ToObject(Result(3)).
5.  Let V = empty.
6.  Get the name of the next property of Result(4) that doesn’t have the
    DontEnum attribute. If there is no such property, go to step 15.
7.  Evaluate Result(1) as if it were an Identifier; see 11.1.2
    (yes, it may be evaluated repeatedly).
8.  Call PutValue(Result(7), Result(6)).
9.  Evaluate Statement.
10. If Result(9).value is not empty, let V = Result(9).value.
11. If Result(9).type is break and Result(9).target is in the current
    label set, go to step 15.
12. If Result(9).type is continue and Result(9).target is in the current
    label set, go to step 6.
13. If Result(8) is an abrupt completion, return Result(8).
14. Go to step 6.
15. Return (normal, V, empty).


The mechanics of enumerating the properties (step 5 in the first algorithm,
step 6 in the second) is implementation-dependent. The order of enumeration
is defined by the object. Properties of the object being enumerated may be 
deleted during enumeration. If a property that has not yet been visited
during enumeration is deleted, then it will not be visited. If new properties 
are added to the object being enumerated during enumeration, the newly added 
properties are not guaranteed to be visited in the active enumeration. 
Enumerating the properties of an object includes enumerating properties
of its prototype, and the prototype of the prototype, and so on, recursively; 
but a property of a prototype is not enumerated if it is “shadowed” because
some previous object in the prototype chain has a property with the same name.

Comment 12

[Phil Schwartau]

Mike: thanks for catching my misquote above. I hadn't realized there 
were two different senses for |in|. But as Brendan noted above:

> ECMA 262 Edition 3 12.6.4 step 3 leads to 9.9, where the first two
> table entries specify that a TypeError be thrown.  A fix for this
> standards compliance bug shipped in mozilla0.9.9.

> Cc'ing waldemar in case the working group might reconsider in light
> of the longstanding Netscape (at least; dunno about IE) behavior.

> /be


Here is a synopsis. Taken from bug 121744, where we fixed this bug:
"JS should error on |for(i in undefined)|, |for(i in null)| "


12.6.4 The for-in Statement

The production 
IterationStatement : for ( LeftHandSideExpression in Expression ) Statement
is evaluated as follows:

1. Evaluate the Expression.
2. Call GetValue(Result(1)).
3. Call ToObject(Result(2)).
...


9.9 ToObject

Input Type : Result
Undefined  : Throw a TypeError exception.
Null       : Throw a TypeError exception.
... 

Comment 13

[Phil Schwartau]

--------------------------  SUMMARY  -------------------------------

In bug 121744, we fixed this bug:
"JS should error on |for(i in undefined)|, |for(i in null)| "

We were right to do this from a standards point of view, since 
the ECMA-262 standard specifies this behavior (see above).

But have we broken many web pages by doing this? That seems to be
the issue now. The fact is, we have changed the behavior of JS
from both NN4.7 and IE, where such statements do not produce errors.

To see this, try these javascript: URLS

        javascript: for(i in undefined){};    alert('Done');
        javascript: for(i in null){};         alert('Done');
        javascript: var obj; for(i in obj){}; alert('Done');


In NN4.7 and IE6, you get the alert('Done') on each of these.
In Mozilla/N6, you don't get any alert, but rather these errors:

        Error: undefined has no properties
        Error: null has no properties
        Error: undefined has no properties

Comment 14

[Waldemar Horwat]

I'm not sure it's worth fixing this bug, since we may make this behavior
legitimate in ECMAScript Edition 4.  I posed this issue to the ECMA group and
will see what response I get.

Comment 15

[Phil Schwartau]

Based on Waldemar's Comment #14, should we reopen bug 121744
and undo the fix we made there?

Comment 16

[Waldemar Horwat]

We (the ECMA group) are leaning towards treating this as a bug in the standard.
 What other related cases are there?  Phil: How does the in operator behave on
the various implementations?

Comment 17

[Huazhang Shao]

Standards are written by people, and should serve people.

Comment 18

[Brendan Eich [:brendan]]

The change made to comply with the standard as written is easily undone.  Let's
get this right for mozilla1.0 -- I'm preapproving and taking the liberty of
targeting.  To undo, just 'cvs up -j3.98 -j3.97 jsinterp.c', test, review, and
check in.

/be

Comment 19

[Phil Schwartau]

I have just tested the effect of 'cvs up -j3.98 -j3.97 jsinterp.c'
against the JS testsuite on Linux. It only causes one new failure:


Testcase ecma_3/Statements/regress-121744.js failed:
Bug Number 121744
STATUS: JS should error on |for(i in undefined)|, |for(i in null)|

Failure messages were:

Section 1 of test -
Expected value 'TypeError', Actual value 'Did not generate ANY error!!!'

       etc. 
       etc.


But this is to be expected. Recall bug 121744 is where we instituted
the change to comply with the standard. I will now modify the test to
expect no error, and add annotations both to the test and bug 121744.

Comment 20

[timeless]

this is currently release noted, please be sure to update the release note for 
mozilla1.0

Comment 21

[Phil Schwartau]

I have:

1. Modified the testcase for bug 121744 to run only in Rhino, not SpiderMonkey
2. Added a testcase for the current bug:

        mozilla/js/tests/ecma_3/Statements/regress-131348.js

3. Filed bug 136893 to see if Rhino wants to follow SpiderMonkey on this,
   or continue to conform to the ECMA-262 Edition 3 spec

4. I will relase-note this for Mozilla 1.0 once the current bug is resolved.

Comment 22

[Phil Schwartau]

I've noted this in the Mozilla 1.0 Release Notes Tracking Bug.
See http://bugzilla.mozilla.org/show_bug.cgi?id=133795#c13

timeless: thank you for pointing out the need for this!

Comment 23

[Kenton Hanson (gone)]

Attachment: Reverse of Patch 66477 for bug #121744

Backing out patch #66477

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 79841 [details] [diff] [review]
Reverse of Patch 66477 for bug #121744

r=shaver.

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 79841 [details] [diff] [review]
Reverse of Patch 66477 for bug #121744

sr=[e-mail] -- khanson, please land ASAP on the trunk and petition
drivers for approval to get this into the MOZILLA_1_0_BRANCH.  Thanks,

/be

Comment 26

[Phil Schwartau]

We need to get this in. I've tested the effect of this in 
Comment #19 - looks good. 

NOTE: for some reason, I cannot apply the patch provided in Comment #23:

[//d/JS_EXP/mozilla/js/src] patch < 131348.patch
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.


This happens to me both on Linux and WinNT; don't know what's
going wrong.

Comment 27

[Phil Schwartau]

The problem seems to be ^M Mac line endings;
same issue as at 

   http://bugzilla.mozilla.org/show_bug.cgi?id=89443#c44
   http://bugzilla.mozilla.org/show_bug.cgi?id=89443#c45

Comment 28

[Kenton Hanson (gone)]

Committed to trunk.

Comment 29

[Kenton Hanson (gone)]

Comment on attachment 79841 [details] [diff] [review]
Reverse of Patch 66477 for bug #121744

changed line feeds

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 79841 [details] [diff] [review]
Reverse of Patch 66477 for bug #121744

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 31

[Kenton Hanson (gone)]

Brendan,

I am having difficulty checking this into the Bracnh.  It has all the necessary
aprovals.  Could you please check it in.

Thanks,

Kenton

Comment 32

[Brendan Eich [:brendan]]

I checked the patch into the 1.0 branch.

Patrick, could you give Kenton a hand with MacCVS?  Thanks.

/be

Comment 33

[Phil Schwartau]

Verified Fixed on the trunk and the Mozilla 1.0.0 branch.
Tried these variations:

        javascript: for (item in null) {}; alert("Hi");
        javascript: for (item in undefined) {}; alert("Hi");
        javascript: var obj; for (item in obj) {}; alert("Hi");

Got the alertbox every time, and no errors in the JS Console -