Object.values(document.body.childNodes) is empty
Reported by
blueraja...@gmail.com,
May 8 2017
|
||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:53.0) Gecko/20100101 Firefox/53.0 Steps to reproduce the problem: > Object.values(document.body.childNodes) [] What is the expected behavior? In Firefox, I get this: > Object.values(document.body.childNodes) [#text "", <div#notify-container>, #text "", ...] What went wrong? Also note that in both Firefox and Chrome, Object.keys() returns > Object.keys(document.body.childNodes) ["0", "1", "2", "3", "4", ...] It should not be possible for Object.keys() to be non-empty while Object.values() is empty, so this is clearly a Chrome bug Did this work before? N/A Chrome version: 58.0.3029.96<Copy from: 'about:version'> Channel: stable OS Version: OS X 10.10 Flash Version: Shockwave Flash 25.0 r0
,
May 9 2017
Tested in chrome # 58.0.3029.96 on Mac 10.12.3 and tested in other browser Firefox also.Observed different behaviors. Please find the screen shots for your reference. @Reporter: Could you please let me know if i have missed anything and if possible,Please create new profile without extensions and apps.Re-check once and let us know the observations and expected behaviour of the issue which would help us to triage the issue further. Thanks in Advance.
,
May 12 2017
I don't understand what I'm being asked to do. You were able to reproduce the issue. My original post did not mention any addons...
,
May 12 2017
Thank you for providing more feedback. Adding requester "rbasuvula@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 13 2017
,
May 13 2017
The key difference between what Chromium is doing, and what Firefox is doing, is that the indexed properties of NodeList are not enumerable in Chromium, and in Firefox they are. Given this quirk, v8 correctly excludes the non-enumerale values from the result. I have no idea which engine, if any is being spec compliant here. I've spent a few minutes digging through WebIDL and DOM and haven't found anything to justify either approach yet.
,
May 13 2017
From discussion with Anne, it looks look a Blink bindings bug, https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty is not applied to objects with indexed properties, which it should per https://heycam.github.io/webidl/#dfn-legacy-platform-object.
,
May 13 2017
,
May 13 2017
If it were just a matter of enumerable vs non-, then Object.keys() should be empty too.
,
May 13 2017
It looks like Object.keys() is simply trusting the results of the named/indexed interceptors, whereas Object.values/entries() explicitly checks if the descriptors are enumerable or not (which it needs to do for JSProxy objects, but also does for API objects with dictionary property stores --- which applies to NodeList). So no, Object.keys() is behaving incorrectly, but Object.values() and entries should do the right thing if the enumerable bit is what it should be.
,
May 13 2017
,
May 13 2017
Example: for-in enumeration is not honouring the enumerable bit from the indexed properties interceptor either. There are a few different subsystems which seem to be broken WRT named/indexed properties. To be sure, part of this is a bug in v8 (ignoring enumerable bit from interceptor calls). But as far as v8 returning the wrong results for Object.values(<NodeList>), that's a bindings bug.
,
May 17 2017
/CC jochen: would it make sense to add debug assertions to the API to make sure results from the enumerator callbacks have appropriate property descriptors?
,
May 17 2017
sure!
,
May 30 2017
,
Jun 6 2017
caitp@, it sounds like you're looking at this?
,
Jun 6 2017
I'm not entirely sure how we want to solve the bindings issue (WRT named/indexed getter properties having incorrect "enumerable" flag). I can produce a short-term fix in v8 for Object.values()/entries() which will just trust the results of the enumerator callback for API objects (and likely increase performance quite a bit for those cases), so I'll work on that, but fixing the enumerator callback itself also needs a solution.
,
Jul 11 2017
Same for Object.values(document.querySelectorAll('body')) :(
I tried Firefox and it gets this right.
,
Jul 19 2017
I've stumbled upon this bug while working on legacy platform objects, I'll send a CL for this in a bit.
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d299da2bb35b40e9cc2a64a8e273d8301d234a3 commit 4d299da2bb35b40e9cc2a64a8e273d8301d234a3 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Thu Jul 20 15:30:07 2017 bindings: Return the right property descriptors for legacy platform objects. The existing implementation of named and indexed properties in IDL interfaces did not return the right property descriptor values according to https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty: * Indexed properties always had [[Writable]] set to true (its value must depend on the presence of an indexed property setter) and [[Enumerable]] set to false (due to V8's default value when a handler does not set either a query or descriptor callback; it must always be true). * Named properties always set [[Writable]] to true (its value must depend on the presence of a named property setter). Since we initialize both |NamedPropertyHandlerConfiguration| and |IndexedPropertyHandlerConfiguration| in our code with different constructors, the fix for the two cases above is not "symmetrical": * |IndexedPropertyHandlerConfiguration| is constructed with a callback for [[DefineOwnProperty]], so we cannot specify an |IndexedPropertyQueryCallback| like |NamedPropertyHandlerConfiguration| does (see https://codereview.chromium.org/2311873002). In this case, we add an |IndexedPropertyDescriptorCallback| that is responsible for implementing the steps from the WebIDL spec and returning a v8::Object with the properties a property descriptor must have. * |NamedPropertyHandlerConfiguration|'s case is easier because we can just fix its existing |GenericNamedPropertyQueryCallback| and return either v8::None or v8::ReadOnly depending on the presence of a named callback setter. It is also important to note that when an IDL interface is declared with [LegacyUnenumerableNamedProperties] its named properties should not be enumerable; we still need to add support for that extended attribute to the bindings code though, so Object.keys() or Object.values() on e.g. HTMLCollection still return more entries than they should. Bug: 703990 , 719703 Change-Id: I772f92a377a1f5013f4f78e5d361a43974378d1a Reviewed-on: https://chromium-review.googlesource.com/576183 Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Cr-Commit-Position: refs/heads/master@{#488252} [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/dom/collections/HTMLCollection-supported-property-indices-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Document-getElementsByTagName-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-getElementsByTagName-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/attributes-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-indexed-properties-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/external/wpt/html/semantics/forms/the-form-element/form-indexed-element-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/fast/js/getOwnPropertyDescriptor-expected.txt [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/LayoutTests/fast/js/resources/getOwnPropertyDescriptor.js [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/templates/interface.h.tmpl [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/idls/core/TestSpecialOperationsNotEnumerable.idl [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface5.idl [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperationsNotEnumerable.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperationsNotEnumerable.h [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp [modify] https://crrev.com/4d299da2bb35b40e9cc2a64a8e273d8301d234a3/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.h
,
Jul 20 2017
The commit above fixes most of the issues, but we're still not 100% compliant; for that, we need to fix bug 703990 to prevent named properties from being enumerable in certain cases.
,
Jul 21 2017
,
Jul 21 2017
... we're also blocked on the V8 bug from comment #22, which prevents Object.keys() from working correctly with interfaces that have [LegacyUnenumerableNamedProperties].
,
Aug 22 2017
The asymmetry between Object.keys()/Object.values() in the original report, as well as what phistuck reported in comment #11 have been fixed by https://chromium-review.googlesource.com/576183 (comment #20), which is part of M61. To be clear, I'm specifically talking about those issues reported here (which revolve around NodeList and wrong property descriptors for legacy platform objects). NodeList only supports indexed properties, but not named properties, which makes the fix from comment #20 enough. Other common interfaces such as HTMLCollection and HTMLAllCollection are still buggy in M61. For example, named properties appear in for-in loops as well as Object.keys(). The complete fix is material for M62 and M63: it requires V8's https://chromium-review.googlesource.com/609068, which only landed a few days ago, as well as converting some IDLs to [LegacyUnenumerableNamedProperties]. Those interested in the above can follow bug 703990 , where I'm still landing new CLs and slowly converting IDLs to [LUNP].
,
Aug 28 2017
,
Sep 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cf8a4164dc2d7dc306b094fadf84052d482d44b7 commit cf8a4164dc2d7dc306b094fadf84052d482d44b7 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Wed Sep 13 10:48:39 2017 bindings: Expose an indexed property descriptor handler for named properties. Follow-up to https://chromium-review.googlesource.com/576183 ("bindings: Return the right property descriptors for legacy platform objects"), which introduced the indexed descriptor callbacks for bindings. Even if a given IDL (such as Storage.idl) only supports named properties, our bindings layer installs a handler for indexed properties that, in this case, mostly just ends up falling back to the named property handlers. However, we were not defining a descriptor callback for indexed properties when the IDL only supports named properties. Consequently, we were not setting the right property descriptor values for a given numeric property, leading to bizarre cases where, e.g. if Storage only had numeric keys, Object.values() and Object.keys() would return arrays of diffferent lengths (the latter would be an empty one, actually). This change is a bit complicated because we install our indexed and named property handlers differently: the former is created with descriptor and definer callbacks, whereas the latter uses the query callback signature instead. This means the indexed property descriptor callback for named properties needs to call the query callback and translate its return value into a format suitable for descriptor callbacks, which is not very efficient. Ideally, both named and indexed properties should converge in terms of how their handlers are declared. Despite the fact that this is a follow-up commit, the original CL did not introduce a regression; interfaces that only support named properties were just behaving like they did in the past, with an indexed descriptor callback not being set. Bug: 719703 , 764633 Change-Id: I1ebaabe61f5f8b6cadb0cfa8b41664a64e49e51b Reviewed-on: https://chromium-review.googlesource.com/660497 Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#501591} [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/LayoutTests/external/wpt/webstorage/storage_enumerate.html [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/templates/interface.h.tmpl [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl [add] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/idls/modules/TestNotEnumerableNamedGetter.idl [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.cpp [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.h [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInheritedLegacyUnenumerableNamedProperties.cpp [modify] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInheritedLegacyUnenumerableNamedProperties.h [add] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/modules/V8TestNotEnumerableNamedGetter.cpp [add] https://crrev.com/cf8a4164dc2d7dc306b094fadf84052d482d44b7/third_party/WebKit/Source/bindings/tests/results/modules/V8TestNotEnumerableNamedGetter.h
,
Sep 13 2017
Adding the M-63 label due to the commit from comment #26.
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/231585efb676b7d99748e29f9049b29bf1f84f67 commit 231585efb676b7d99748e29f9049b29bf1f84f67 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Tue Sep 19 00:42:47 2017 Make HTMLAllCollection and HTMLCollection's named properties not enumerable. Add the [LegacyUnenumerableNamedProperties] extended attribute to both IDL files so that they follow what the DOM and HTML specs mandate. Since the attribute is inherited, it also applies to both HTMLOptionsCollection and HTMLFormControlsCollection. Both HTMLAllCollection and HTMLCollection already implemented the required enumeration and query callbacks (in other words, they already implemented the specs' definitions of "supported property names" for each interface). The user-visible difference is that all named properties were previously enumerable, while with this CL they are not: they are no longer returned by Object.keys() or iterated by for-in loops, but Object.getOwnPropertyNames() and Object.getOwnPropertyDescriptors() still include them. The value of the "enumerable" property of the property descriptors returned is now false. WebKit did the same thing in r210667 from January 2017 (was already marking the properties as not enumerable before), and Gecko did the same in bug 1270349 in May 2016 (the properties were already not enumerable before that, as explained in the Intent to Implement email). Some tests in fast/dom were removed, as they only served to test the previous behavior and the new, expected behavior is coverted by web-platform-tests. Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/YbsMn7DL1cg/rdkA-It1AgAJ Bug: 703990 , 719703 Change-Id: Id6c899c1715f22a426f24c30f0ad3f527beb14d5 Reviewed-on: https://chromium-review.googlesource.com/654837 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Kent Tamura <tkent@chromium.org> Cr-Commit-Position: refs/heads/master@{#502733} [modify] https://crrev.com/231585efb676b7d99748e29f9049b29bf1f84f67/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object-expected.txt [modify] https://crrev.com/231585efb676b7d99748e29f9049b29bf1f84f67/third_party/WebKit/LayoutTests/external/wpt/dom/collections/HTMLCollection-supported-property-names-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Document-getElementsByTagName-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-children-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-getElementsByTagName-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/external/wpt/html/dom/documents/dom-tree-accessors/document.forms-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlallcollection-enumerated-properties.html [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlcollection-enumerated-properties-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlcollection-enumerated-properties.html [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlformcontrolscollection-enumerated-properties-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmlformcontrolscollection-enumerated-properties.html [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmloptionscollection-enumerated-properties-expected.txt [delete] https://crrev.com/9e319197e596a8b57465591c2ded1afac2ab484d/third_party/WebKit/LayoutTests/fast/dom/htmloptionscollection-enumerated-properties.html [modify] https://crrev.com/231585efb676b7d99748e29f9049b29bf1f84f67/third_party/WebKit/Source/core/html/HTMLAllCollection.idl [modify] https://crrev.com/231585efb676b7d99748e29f9049b29bf1f84f67/third_party/WebKit/Source/core/html/HTMLCollection.idl |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by ligim...@chromium.org
, May 8 2017