Closed
Bug 636989
Opened 14 years ago
Closed 14 years ago
Object.getOwnPropertyNames considers enumerable getter inherited properties as own
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: bruant.d, Assigned: brendan)
References
(Blocks 1 open bug)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
14.69 KB,
patch
|
jorendorff
:
review+
gal
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Does it block Bug 554013 or Bug 496923?
Reporter | ||
Comment 2•14 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.
Comment 3•14 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: → 637994
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 4•14 years ago
|
||
Using this bug to mop up remaining shared permanent dirt left after the patch for bug 637994 landed.
/be
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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+
Comment 7•14 years ago
|
||
(and don't forget the tests of course)
Assignee | ||
Comment 8•14 years ago
|
||
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•14 years ago
|
Attachment #539679 -
Flags: review?(gal)
Comment 9•14 years ago
|
||
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+
Comment 10•14 years ago
|
||
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•14 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•14 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•14 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•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 14•14 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/68ab9132fad7
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•