636989 - Object.getOwnPropertyNames considers enumerable getter inherited properties as own

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Bruant]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: FF4beta12

A test case of the test262 test suite failed (15.2.3.5-4-21 - Object.create - an enumerable inherited accessor property in 'Properties' is not defined in 'obj' (15.2.3.7 step 3))

Reproducible: Always

Steps to Reproduce:
1. run the test cases
Actual Results:  
false

Expected Results:  
true

Here is a test case based on the test262 15.2.3.5-4-21 Test "Object.create - an enumerable inherited accessor property in 'Properties' is not defined in 'obj' (15.2.3.7 step 3)".

(function testcase() {
    var proto = {};
    Object.defineProperty(proto, "prop", {get: function () {return {};}, enumerable: true});
    var ConstructFun = function () {};
    ConstructFun.prototype = proto;
    var child = new ConstructFun;
    return Object.getOwnPropertyNames(child).indexOf('prop') === -1;
}).call();

The original test case can be found here: http://hg.ecmascript.org/tests/test262/file/6591ffbabb2e/test/suite/ietestcenter/chapter15/15.2/15.2.3/15.2.3.5/15.2.3.5-4-21.js

Comment 1

[David Bruant]

Does it block Bug 554013 or Bug 496923?

Comment 2

[David Bruant]

Reducing the test case:

(function testcase() {
  var proto = {};
  Object.defineProperty(proto, "prop", {get: function(){}, enumerable: true});
  var child = Object.create(proto);
  return Object.getOwnPropertyNames(child).indexOf('prop') === -1;
}).call();

Still returning false in April 26th nightly build.

Comment 3

[Mark S. Miller]

Is this a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=637994 ?
If so, enumerability doesn't seem to matter, but configurability does.

Comment 4

[Brendan Eich [:brendan]]

Using this bug to mop up remaining shared permanent dirt left after the patch for bug 637994 landed.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Trying to find out whether we use test262 yet. If so it covers the cases here. If not, or even so if good policy reason exists, I can duplicate tests.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 539619 [details] [diff] [review]
proposed fix

Review of attachment 539619 [details] [diff] [review]:
-----------------------------------------------------------------

Great. r=me with the few nits below.

::: js/src/jsiter.cpp
@@ +189,5 @@
>      if ((pobj->getProto() || pobj->isProxy()) && !ht.add(p, id))
>          return false;
>  
> +    if ((flags & JSITER_OWNONLY) && pobj != obj)
> +        return true;

Instead, change the callers (Snapshot, I think, is the only relevant one) not to walk
the prototype chain if JSITER_OWNONLY is set, and here you can:
  JS_ASSERT_IF(flags & JSITER_OWNONLY, pobj == obj);

::: js/src/jsobj.cpp
@@ +1550,5 @@
>      /*
>       * XXX ECMA spec error compatible: return false unless hasOwnProperty.
>       * The ECMA spec really should be fixed so propertyIsEnumerable and the
>       * for..in loop agree on whether prototype properties are enumerable,
>       * obviously by fixing this method (not by breaking the for..in loop!).

In passing: It might be time to let this comment go. Presumably we won't be revisiting propertyIsEnumerable now that ES5 is done.

@@ +5763,1 @@
>           * If no property, or the property comes unshared or impermanent from

s/unshared or impermanent //

Comment 7

[Jason Orendorff [:jorendorff]]

(and don't forget the tests of course)

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Will ask for gal second review on jsiter.cpp changes. Would be nice to avoid all the parameters and tests in the Enumerate "inner loop body" but the inline should mean it's all optimized appropriately, for the calls from Snapshot.

I put the test from comment 0 into our suite. I did not pull on the test262 ball of string.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 539679 [details] [diff] [review]
proposed fix, v2

Review of attachment 539679 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. No changes requested, one comment out of curiosity.

That jsiter.cpp code looks like it could be simplified more, in a separate bug, someday.

::: js/src/jsiter.cpp
@@ +251,3 @@
>      IdSet ht(cx);
>      if (!ht.init(32))
>          return NULL;

You removed the FIXME comment but didn't make the init() call conditional. JSITER_OWNONLY is the rarer case, I guess.

Comment 10

[Jason Orendorff [:jorendorff]]

Oh, I see, the comment is wrong in the case where obj is a proxy, and it seems better not to tightly couple.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Oh, I see, the comment is wrong in the case where obj is a proxy, and it
> seems better not to tightly couple.

It would require walking up from a non-proxy hunting for a proxy. Not worth it without more profiling evidence.

/be

Comment 12

[Andreas Gal :gal]

Comment on attachment 539679 [details] [diff] [review]
proposed fix, v2

Nicest cleanup in this code in a really long time.

Comment 13

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/68ab9132fad7

/be

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/68ab9132fad7