Last Comment Bug 636989 - Object.getOwnPropertyNames considers enumerable getter inherited properties as own
: Object.getOwnPropertyNames considers enumerable getter inherited properties a...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Brendan Eich [:brendan]
: Jason Orendorff [:jorendorff]
Depends on: 690031
Blocks: test262 es5 575997
  Show dependency treegraph
Reported: 2011-02-26 04:13 PST by David Bruant
Modified: 2011-09-29 12:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

proposed fix (10.96 KB, patch)
2011-06-15 12:32 PDT, Brendan Eich [:brendan]
jorendorff: review+
Details | Diff | Splinter Review
proposed fix, v2 (14.69 KB, patch)
2011-06-15 15:58 PDT, Brendan Eich [:brendan]
jorendorff: review+
gal: review+
Details | Diff | Splinter Review

Description David Bruant 2011-02-26 04:13:21 PST
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:
Comment 1 David Bruant 2011-02-26 04:18:48 PST
Does it block Bug 554013 or Bug 496923?
Comment 2 David Bruant 2011-04-26 15:31:38 PDT
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.
Comment 3 Mark S. Miller 2011-05-29 22:24:28 PDT
Is this a duplicate of ?
If so, enumerability doesn't seem to matter, but configurability does.
Comment 4 Brendan Eich [:brendan] 2011-06-15 10:27:15 PDT
Using this bug to mop up remaining shared permanent dirt left after the patch for bug 637994 landed.

Comment 5 Brendan Eich [:brendan] 2011-06-15 12:32:54 PDT
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.

Comment 6 Jason Orendorff [:jorendorff] 2011-06-15 12:59:08 PDT
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 //
Comment 7 Jason Orendorff [:jorendorff] 2011-06-15 12:59:56 PDT
(and don't forget the tests of course)
Comment 8 Brendan Eich [:brendan] 2011-06-15 15:58:15 PDT
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.

Comment 9 Jason Orendorff [:jorendorff] 2011-06-15 16:11:55 PDT
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] 2011-06-15 16:13:02 PDT
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] 2011-06-15 16:24:52 PDT
(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 12 Andreas Gal :gal 2011-06-15 16:26:13 PDT
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] 2011-06-17 00:02:53 PDT

Comment 14 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:07:15 PDT
cdleary-bot mozilla-central merge info:

Note You need to log in before you can comment on or make changes to this bug.