Object.getOwnPropertyNames considers enumerable getter inherited properties as own

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: David Bruant, Assigned: brendan)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Does it block Bug 554013 or Bug 496923?
Blocks: 445494
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 652780
(Reporter)

Updated

6 years ago
Depends on: 575997

Comment 3

6 years ago
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.
See Also: → bug 637994
(Assignee)

Updated

6 years ago
Assignee: general → brendan
Blocks: 575997
Status: UNCONFIRMED → ASSIGNED
No longer depends on: 575997
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 4

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

/be
(Assignee)

Comment 5

6 years ago
Created attachment 539619 [details] [diff] [review]
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
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
>       * 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 //
Attachment #539619 - Flags: review?(jorendorff) → review+
(and don't forget the tests of course)
(Assignee)

Comment 8

6 years ago
Created attachment 539679 [details] [diff] [review]
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
Attachment #539619 - Attachment is obsolete: true
Attachment #539679 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 11

6 years ago
(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

6 years ago
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+
(Assignee)

Updated

6 years ago
Summary: Object.getOwnPropertyNames and hasOwnProperty consider enumerable getter inherited properties as own → Object.getOwnPropertyNames considers enumerable getter inherited properties as own
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/tracemonkey/rev/68ab9132fad7

/be
Whiteboard: fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/68ab9132fad7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 690031
You need to log in before you can comment on or make changes to this bug.