90596 - uneval, toSource() omit toString on objects overriding toString

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Phil Schwartau]

Discovered by [e-mail]. In the standalone JS shell:

js> var obj = {
                color : 'red',  
                sayHello : function() {return 'Hello';},  
                toString : function() {return 5;}
              }


js> obj.toSource()
({color:"red", sayHello:(function () {return "Hello";})})
js> uneval(obj)
({color:"red", sayHello:(function () {return "Hello";})})


Notice that the locally-defined toString method has not been included.
This can't be correct, can it? We expect uneval(obj), obj.toSource() 
to provide templates for new objects of the same type -

Comment 1

[Brendan Eich [:brendan]]

See the XXXbe comments about inheriting attrs, getter, setter, and tinyid from a
proto-property of the same name.  Another, simpler testcase is

  var o = {hasOwnProperty:42}
  print(o.toSource())

which prints ({}).

/be

Comment 2

[Alex Vincent [:WeirdAl]]

I've noticed this sort of thing before in uses of for...in statements.  The 
ECMAScript specs include a [DontEnum] attribute which (as I understand it) the 
JS engine hides from the end user.  I've never been able to override this 
attribute by replacing a [DontEnum]-enabled property with another under the 
same name.

Not sure if that means anything -- all the properties and methods of 
Object.prototype are generally not enumerable.  Replacing them doesn't appear 
to affect the enumerability of them, as far as I know.

I'm not complaining.  There are times when I wish I could set whether a 
property was enumerable or not.

Comment 3

[Phil Schwartau]

Three testcases added to JS test suite:

         mozilla/js/tests/js1_5/Object/regress-90596-001.js
         mozilla/js/tests/js1_5/Object/regress-90596-002.js
         mozilla/js/tests/js1_5/Object/regress-90596-003.js

The first expects toSource() to include toString if the latter is overridden.
The second expects uneval() to include toString under this condition.
The third expects for..in to include toString under this condition.

Comment 4

[Alex Vincent [:WeirdAl]]

Phil, myself not being a Netscape or Mozilla engineer, I hope you'll forgive me 
if I express an opinion for which I am probably unqualified.  :D

I personally believe this is not a bug.  Section 8.6.1, ECMAScript 3rd Edition:

DontEnum:  The property is not to be enumerated by a for-in enumeration (section
12.6.4).

Section 15.2.3.1, ECMAScript 3rd Edition:
 Object.prototype
The initial value of Object.prototype is the Object prototype object (section 
15.2.4).
This property has the attributes { DontEnum, DontDelete, ReadOnly }.

That includes Object.prototype.toString().  I think you'll find similar 
statements for each of the core constructors.  I'd love to see a reference 
cited (I really would!!!) where ECMAScript states replacing a property name 
overrides DontEnum.

Comment 5

[Alex Vincent [:WeirdAl]]

I've just searched through my copy of ECMAScript 3rd edition.  No claims in 
there on whether DontEnum can be changed or not.

Comment 6

[Kenton Hanson (gone)]

Looking for an opinion as to whether this is really a bug.

Adding Waldemar to the cc list.

Comment 7

[Phil Schwartau]

My own opinion, FWIW, is that if ECMAScript is going to allow a DontEnum 
property to be overridden by the user's own, then the new property should
no longer be DontEnum. If the very meaning of the property is mutable,
then why should the DontEnum property be immutable?

If the user does ovverride, why shouldn't he expect uneval to work
on his new object? (See original report at top).

Also note Brendan's Comment #1 above. 

Comment 8

[Waldemar Horwat]

This is a bug and violates the ECMA standard.  The DontEnum attribute of a
property is immutable, but that's not relevant here.  In the obj example, the
toString property that returns 5 is user-defined and should not be DontEnum.  It
happens to shadow the system toString property defined in Object.prototype which
has DontEnum set, but these are two distinct, independent properties.

If you're following the ECMA standard, the crucial section is [[Put]] in
8.6.2.2.  The {...} object literal syntax eventually calls [[Put]] on the newly
created object to add a "toString" property to it.  There is no existing
property with that name in the object itself ([[Put]] doesn't look at the
prototype except via [[CanPut]]), so it creates one.  Step 6 states that the
newly created property has empty attributes.

Had you replaced the toString inside Object.prototype, then the replacement
would also be DontEnum -- step 4 of 8.6.2.2 ensures that attributes are immutable.

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix, preserving pre-ECMA compatibility

Looking for testing and review for 0.9.7.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 59927 [details] [diff] [review]
proposed fix, preserving pre-ECMA compatibility

Oops, broke CanPut compliance.

/be

Comment 11

[Alex Vincent [:WeirdAl]]

Attachment: Testcase based on for...in

All right then.  Personally, I was very uncertain after reading Phil's argument
last night.  My apologies for casting doubt on this issue for four months, and
here's a testcase comparing a replacement of Object.prototype.toString()
against y.toString().

Comment 12

[Brendan Eich [:brendan]]

Attachment: proposed fix, take two

Comment 13

[Phil Schwartau]

I applied the patch in Comment #12 above and ran the JS testsuite
on WinNT against the debug JS shell. The patch makes the testcases
for this bug pass, but introduces five regressions. I will attach
a hyperlinked list of these regressions below.

Comment 14

[Phil Schwartau]

Attachment: Testsuite regressions caused by the latest patch

Comment 15

[Phil Schwartau]

Aplogies - I had a STALE TAG and neglected to do cvs co -A to clear it.
I will re-test the patch now. 

Comment 16

[Phil Schwartau]

OK, with the patch applied to clean source, there are only three
regressions introduced, not five. All three have to do with arguments:


*-* Testcase ecma/ExecutionContexts/10.1.6.js failed:

Failure messages were:
TestFunction(1,2,3) = [object Object] FAILED! expected: value of the argument                                                                        
property


*-* Testcase ecma_3/Function/regress-94506.js failed:
Bug Number 94506
STATUS: Testing functions employing identifiers named "arguments"
Failure messages were:
FAILED!: [reported from test()] Section 2 of test
FAILED!: [reported from test()] Type mismatch, expected type number, actual type 
object
FAILED!: [reported from test()] Expected value '55', Actual value '[object 
Object]'


*-* Testcase js1_4/Functions/function-001.js failed:
Bug Number 324455

Failure messages were:
return arguments when function contains an arguments property = [object Object] 
FAILED! expected: PASS
return function.arguments when function contains an arguments property = [object 
Object] FAILED! expected: PASS

Comment 17

[Kenton Hanson (gone)]

Retargeting for 0.9.8.

Comment 18

[Brendan Eich [:brendan]]

Out of time for 0.9.8 -- I'll help in 0.9.9 (reminder bugmail hereby sent).

/be

Comment 19

[Brendan Eich [:brendan]]

Taking.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: the fix

This passes the testsuite for me, fixing the three regression tests for this
bug that have been failing for a long time now.

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 120340 [details] [diff] [review]
the fix

Roger, this ought to be familiar territory from bug 94693.

The bulk of the change may be easier to see if you apply the patch and read the
before and after paragraphs, side by side.  I moved the read_only_error: block
down to the end, to get it out of the way, and half-indented the label and
closing brace.

/be

Comment 22

[rogerl (gone)]

Comment on attachment 120340 [details] [diff] [review]
the fix

r=rogerl. 
Is it ok to test sprop->flags after unlocking the scope? The code was carefult
not to do that before.
[Just for grins, I also ran the shell under purify for the regession cases w.o.
problem.]

Comment 23

[Phil Schwartau]

> This passes the testsuite for me, fixing the three regression tests
> for this bug that have been failing for a long time now.

It also passes the testsuite for me on WinNT, in both the debug and
optimized JS shell. It's great to see those three failures go away!

Comment 24

[Brendan Eich [:brendan]]

Yes, sprop is alive and well after unlocking the scope, unlike before the
property tree went in (bug 62164).  That's what the comment

                                        Note that due to the garbage-collected
+             * property tree maintained by jsscope.c in rt, we need not worry
+             * about sprop going away behind our back after we've unlocked
+             * scope, above.

is trying to say.  Maybe I should move it up to the JS_UNLOCK_SCOPE(cx, scope);
line?

/be

Comment 25

[Brendan Eich [:brendan]]

And in case it wasn't obvious, I prefer to minimize code size and length of
critical section by unlocking scope early, before the JSPROP_SHARED early return
special case.  Alternative is to JS_UNLOCK_SCOPE(cx, scope) in two places, for
no reason.

I'll juggle the comments a bit before checking in.

/be

Comment 26

[Phil Schwartau]

Verified FIXED.

Tested in the JS shell on WinNT, Linux and Mac OSX. The three testcases
for this bug now pass, and no test regressions have been introduced -