Closed Bug 636989 Opened 13 years ago Closed 13 years ago

Object.getOwnPropertyNames considers enumerable getter inherited properties as own


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bruant.d, Assigned: brendan)


(Blocks 1 open bug)


(Whiteboard: fixed-in-tracemonkey)


(1 file, 1 obsolete file)

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 ( - Object.create - an enumerable inherited accessor property in 'Properties' is not defined in 'obj' ( step 3))

Reproducible: Always

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

Expected Results:  

Here is a test case based on the test262 Test "Object.create - an enumerable inherited accessor property in 'Properties' is not defined in 'obj' ( 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;

The original test case can be found here:
Does it block Bug 554013 or Bug 496923?
Blocks: es5
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;

Still returning false in April 26th nightly build.
Blocks: test262
Depends on: 575997
Is this a duplicate of ?
If so, enumerability doesn't seem to matter, but configurability does.
See Also: → 637994
Assignee: general → brendan
Blocks: 575997
No longer depends on: 575997
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Using this bug to mop up remaining shared permanent dirt left after the patch for bug 637994 landed.

Attached patch proposed fix (obsolete) — Splinter Review
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.

Attachment #539619 - Flags: review?(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
>       * loop agree on whether prototype properties are enumerable,
>       * obviously by fixing this method (not by breaking the 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 //
Attachment #539619 - Flags: review?(jorendorff) → review+
(and don't forget the tests of course)
Attached patch proposed fix, v2Splinter Review
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.

Attachment #539619 - Attachment is obsolete: true
Attachment #539679 - Flags: review?(jorendorff)
Attachment #539679 - Flags: review?(gal)
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.
Attachment #539679 - Flags: review?(jorendorff) → review+
Oh, I see, the comment is wrong in the case where obj is a proxy, and it seems better not to tightly couple.
(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.

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

Nicest cleanup in this code in a really long time.
Attachment #539679 - Flags: review?(gal) → review+
Summary: Object.getOwnPropertyNames and hasOwnProperty consider enumerable getter inherited properties as own → Object.getOwnPropertyNames considers enumerable getter inherited properties as own

Whiteboard: fixed-in-tracemonkey
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 690031
You need to log in before you can comment on or make changes to this bug.