Auto merge of #9067 - nox:unenumerable-named-properties, r=jdm

Make NamedNodeMap's named properties unenumerable

https://dom.spec.whatwg.org/#dom-htmlcollection-item

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9067)
<!-- Reviewable:end -->

components/script/dom/bindings/codegen/CodegenRust.py

@@ -2602,13 +2602,25 @@ class CGDefineProxyHandler(CGAbstractMethod):
         if self.descriptor.operations['NamedDeleter']:
             customDelete = 'delete'
 
-        body = """\
+        getOwnEnumerablePropertyKeys = "own_property_keys"
+        if self.descriptor.interface.getExtendedAttribute("LegacyUnenumerableNamedProperties"):
+            getOwnEnumerablePropertyKeys = "getOwnEnumerablePropertyKeys"
+
+        args = {
+            "defineProperty": customDefineProperty,
+            "delete": customDelete,
+            "getOwnEnumerablePropertyKeys": getOwnEnumerablePropertyKeys,
+            "trace": TRACE_HOOK_NAME,
+            "finalize": FINALIZE_HOOK_NAME,
+        }
+
+        return CGGeneric("""\
 let traps = ProxyTraps {
     enter: None,
     getOwnPropertyDescriptor: Some(getOwnPropertyDescriptor),
-    defineProperty: Some(%s),
+    defineProperty: Some(%(defineProperty)s),
     ownPropertyKeys: Some(own_property_keys),
-    delete_: Some(%s),
+    delete_: Some(%(delete)s),
     enumerate: None,
     preventExtensions: Some(proxyhandler::prevent_extensions),
     isExtensible: Some(proxyhandler::is_extensible),
@@ -2619,7 +2631,7 @@ let traps = ProxyTraps {
     construct: None,
     getPropertyDescriptor: Some(get_property_descriptor),
     hasOwn: Some(hasOwn),
-    getOwnEnumerablePropertyKeys: None,
+    getOwnEnumerablePropertyKeys: Some(%(getOwnEnumerablePropertyKeys)s),
     nativeCall: None,
     hasInstance: None,
     objectClassIs: None,
@@ -2627,16 +2639,15 @@ let traps = ProxyTraps {
     fun_toString: None,
     boxedValue_unbox: None,
     defaultValue: None,
-    trace: Some(%s),
+    trace: Some(%(trace)s),
-    finalize: Some(%s),
+    finalize: Some(%(finalize)s),
     objectMoved: None,
     isCallable: None,
     isConstructor: None,
 };
 
 CreateProxyHandler(&traps, &Class as *const _ as *const _)\
-""" % (customDefineProperty, customDelete, TRACE_HOOK_NAME, FINALIZE_HOOK_NAME)
+""" % args)
-        return CGGeneric(body)
 
 
 class CGDefineDOMInterfaceMethod(CGAbstractMethod):
@@ -4321,10 +4332,12 @@ class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod):
             get = "let index = get_array_index_from_id(cx, id);\n"
 
         if indexedGetter:
-            readonly = toStringBool(self.descriptor.operations['IndexedSetter'] is None)
+            attrs = "JSPROP_ENUMERATE"
+            if self.descriptor.operations['IndexedSetter'] is None:
+                attrs += " | JSPROP_READONLY"
             fillDescriptor = ("desc.get().value = result_root.ptr;\n"
                               "fill_property_descriptor(&mut *desc.ptr, *proxy.ptr, %s);\n"
-                              "return true;" % readonly)
+                              "return true;" % attrs)
             templateValues = {
                 'jsvalRef': 'result_root.handle_mut()',
                 'successCode': fillDescriptor,
@@ -4338,10 +4351,18 @@ class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod):
 
         namedGetter = self.descriptor.operations['NamedGetter']
         if namedGetter:
-            readonly = toStringBool(self.descriptor.operations['NamedSetter'] is None)
+            attrs = []
+            if not self.descriptor.interface.getExtendedAttribute("LegacyUnenumerableNamedProperties"):
+                attrs.append("JSPROP_ENUMERATE")
+            if self.descriptor.operations['NamedSetter'] is None:
+                attrs.append("JSPROP_READONLY")
+            if attrs:
+                attrs = " | ".join(attrs)
+            else:
+                attrs = "0"
             fillDescriptor = ("desc.get().value = result_root.ptr;\n"
                               "fill_property_descriptor(&mut *desc.ptr, *proxy.ptr, %s);\n"
-                              "return true;" % readonly)
+                              "return true;" % attrs)
             templateValues = {
                 'jsvalRef': 'result_root.handle_mut()',
                 'successCode': fillDescriptor,
@@ -4514,6 +4535,49 @@ class CGDOMJSProxyHandler_ownPropertyKeys(CGAbstractExternMethod):
         return CGGeneric(self.getBody())
 
 
+class CGDOMJSProxyHandler_getOwnEnumerablePropertyKeys(CGAbstractExternMethod):
+    def __init__(self, descriptor):
+        assert (descriptor.operations["IndexedGetter"] and
+                descriptor.interface.getExtendedAttribute("LegacyUnenumerableNamedProperties"))
+        args = [Argument('*mut JSContext', 'cx'),
+                Argument('HandleObject', 'proxy'),
+                Argument('*mut AutoIdVector', 'props')]
+        CGAbstractExternMethod.__init__(self, descriptor,
+                                        "getOwnEnumerablePropertyKeys", "bool", args)
+        self.descriptor = descriptor
+
+    def getBody(self):
+        body = dedent(
+            """
+            let unwrapped_proxy = UnwrapProxy(proxy);
+            """)
+
+        if self.descriptor.operations['IndexedGetter']:
+            body += dedent(
+                """
+                for i in 0..(*unwrapped_proxy).Length() {
+                    let rooted_jsid = RootedId::new(cx, int_to_jsid(i as i32));
+                    AppendToAutoIdVector(props, rooted_jsid.handle().get());
+                }
+                """)
+
+        body += dedent(
+            """
+            let expando = get_expando_object(proxy);
+            if !expando.is_null() {
+                let rooted_expando = RootedObject::new(cx, expando);
+                GetPropertyKeys(cx, rooted_expando.handle(), JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, props);
+            }
+
+            return true;
+            """)
+
+        return body
+
+    def definition_body(self):
+        return CGGeneric(self.getBody())
+
+
 class CGDOMJSProxyHandler_hasOwn(CGAbstractExternMethod):
     def __init__(self, descriptor):
         args = [Argument('*mut JSContext', 'cx'), Argument('HandleObject', 'proxy'),
@@ -4963,6 +5027,8 @@ class CGDescriptor(CGThing):
                 cgThings.append(CGProxyUnwrap(descriptor))
                 cgThings.append(CGDOMJSProxyHandlerDOMClass(descriptor))
                 cgThings.append(CGDOMJSProxyHandler_ownPropertyKeys(descriptor))
+                if descriptor.interface.getExtendedAttribute("LegacyUnenumerableNamedProperties"):
+                    cgThings.append(CGDOMJSProxyHandler_getOwnEnumerablePropertyKeys(descriptor))
                 cgThings.append(CGDOMJSProxyHandler_getOwnPropertyDescriptor(descriptor))
                 cgThings.append(CGDOMJSProxyHandler_className(descriptor))
                 cgThings.append(CGDOMJSProxyHandler_get(descriptor))

components/script/dom/bindings/codegen/parser/WebIDL.py

@@ -1069,6 +1069,12 @@ class IDLInterface(IDLObjectWithScope, IDLExposureMixins):
 
             specialMembersSeen[memberType] = member
 
+        if (self.getExtendedAttribute("LegacyUnenumerableNamedProperties") and
+            "named getters" not in specialMembersSeen):
+            raise WebIDLError("[LegacyUnenumerableNamedProperties] used on an interface "
+                              "without a named getter",
+                              [self.location])
+
         if self._isOnGlobalProtoChain:
             # Make sure we have no named setters, creators, or deleters
             for memberType in ["setter", "creator", "deleter"]:
@@ -1417,7 +1423,8 @@ class IDLInterface(IDLObjectWithScope, IDLExposureMixins):
                   identifier == "UnsafeInPrerendering" or
                   identifier == "LegacyEventInit" or
                   identifier == "ProbablyShortLivingObject" or
-                  identifier == "Abstract"):
+                  identifier == "Abstract" or
+                  identifier == "LegacyUnenumerableNamedProperties"):
                 # Known extended attributes that do not take values
                 if not attr.noArguments():
                     raise WebIDLError("[%s] must take no arguments" % identifier,

components/script/dom/bindings/codegen/parser/legacy-unenumerable-named-properties.patch

@@ -0,0 +1,25 @@
+--- WebIDL.py
++++ WebIDL.py
+@@ -1069,6 +1069,12 @@ class IDLInterface(IDLObjectWithScope, IDLExposureMixins):
+
+             specialMembersSeen[memberType] = member
+
++        if (self.getExtendedAttribute("LegacyUnenumerableNamedProperties") and
++            "named getters" not in specialMembersSeen):
++            raise WebIDLError("[LegacyUnenumerableNamedProperties] used on an interface "
++                              "without a named getter",
++                              [self.location])
++
+         if self._isOnGlobalProtoChain:
+             # Make sure we have no named setters, creators, or deleters
+             for memberType in ["setter", "creator", "deleter"]:
+@@ -1417,7 +1423,8 @@ class IDLInterface(IDLObjectWithScope, IDLExposureMixins):
+                   identifier == "UnsafeInPrerendering" or
+                   identifier == "LegacyEventInit" or
+                   identifier == "ProbablyShortLivingObject" or
+-                  identifier == "Abstract"):
++                  identifier == "Abstract" or
++                  identifier == "LegacyUnenumerableNamedProperties"):
+                 # Known extended attributes that do not take values
+                 if not attr.noArguments():
+                     raise WebIDLError("[%s] must take no arguments" % identifier,

components/script/dom/bindings/codegen/parser/update.sh

@@ -1,4 +1,5 @@
 wget https://mxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.py?raw=1 -O WebIDL.py
 patch < abstract.patch
+patch < legacy-unenumerable-named-properties.patch
 
 # TODO: update test files from https://dxr.mozilla.org/mozilla-central/source/dom/bindings/parser/tests

components/script/dom/bindings/proxyhandler.rs

@@ -8,6 +8,7 @@
 
 use dom::bindings::conversions::is_dom_proxy;
 use dom::bindings::utils::delete_property_by_id;
+use js::JSPROP_GETTER;
 use js::glue::GetProxyExtra;
 use js::glue::InvokeGetOwnPropertyDescriptor;
 use js::glue::{GetProxyHandler, SetProxyExtra};
@@ -18,7 +19,6 @@ use js::jsapi::{JSContext, JSObject, JSPropertyDescriptor};
 use js::jsapi::{JSErrNum, JS_StrictPropertyStub};
 use js::jsapi::{JS_DefinePropertyById6, JS_NewObjectWithGivenProto};
 use js::jsval::ObjectValue;
-use js::{JSPROP_ENUMERATE, JSPROP_GETTER, JSPROP_READONLY};
 use libc;
 use std::{mem, ptr};
 
@@ -135,9 +135,9 @@ pub fn ensure_expando_object(cx: *mut JSContext, obj: HandleObject) -> *mut JSOb
 /// and writable if `readonly` is true.
 pub fn fill_property_descriptor(desc: &mut JSPropertyDescriptor,
                                 obj: *mut JSObject,
-                                readonly: bool) {
+                                attrs: u32) {
     desc.obj = obj;
-    desc.attrs = if readonly { JSPROP_READONLY } else { 0 } | JSPROP_ENUMERATE;
+    desc.attrs = attrs;
     desc.getter = None;
     desc.setter = None;
 }

components/script/dom/browsingcontext.rs

@@ -12,15 +12,15 @@ use dom::bindings::utils::get_array_index_from_id;
 use dom::document::Document;
 use dom::element::Element;
 use dom::window::Window;
-use js::JSCLASS_IS_GLOBAL;
+use js::glue::{CreateWrapperProxyHandler, ProxyTraps, NewWindowProxy};
-use js::glue::{CreateWrapperProxyHandler, GetProxyPrivate, NewWindowProxy};
+use js::glue::{GetProxyPrivate, SetProxyExtra};
-use js::glue::{ProxyTraps, SetProxyExtra};
 use js::jsapi::{Handle, HandleId, HandleObject, JSAutoCompartment, JSAutoRequest, JSContext};
 use js::jsapi::{JSErrNum, JSObject, JSPropertyDescriptor, JS_DefinePropertyById6};
 use js::jsapi::{JS_ForwardGetPropertyTo, JS_ForwardSetPropertyTo, JS_GetClass};
 use js::jsapi::{JS_GetOwnPropertyDescriptorById, JS_HasPropertyById, MutableHandle};
 use js::jsapi::{MutableHandleValue, ObjectOpResult, RootedObject, RootedValue};
-use js::jsval::{ObjectValue, PrivateValue, UndefinedValue};
+use js::jsval::{ObjectValue, UndefinedValue, PrivateValue};
+use js::{JSCLASS_IS_GLOBAL, JSPROP_READONLY};
 
 #[dom_struct]
 pub struct BrowsingContext {
@@ -138,7 +138,7 @@ unsafe extern "C" fn getOwnPropertyDescriptor(cx: *mut JSContext,
         let mut val = RootedValue::new(cx, UndefinedValue());
         window.to_jsval(cx, val.handle_mut());
         (*desc.ptr).value = val.ptr;
-        fill_property_descriptor(&mut *desc.ptr, *proxy.ptr, true);
+        fill_property_descriptor(&mut *desc.ptr, *proxy.ptr, JSPROP_READONLY);
         return true;
     }
 

components/script/dom/htmlcollection.rs

@@ -337,8 +337,8 @@ impl HTMLCollectionMethods for HTMLCollection {
 
         // Step 2.
         self.elements_iter().find(|elem| {
-            elem.get_string_attribute(&atom!("name")) == key ||
+            elem.get_string_attribute(&atom!("id")) == key ||
-            elem.get_string_attribute(&atom!("id")) == key
+            (elem.namespace() == &ns!(html) && elem.get_string_attribute(&atom!("name")) == key)
         })
     }
 

components/script/dom/webidls/HTMLCollection.webidl

@@ -5,6 +5,7 @@
 
 // https://dom.spec.whatwg.org/#interface-htmlcollection
 
+[LegacyUnenumerableNamedProperties]
 interface HTMLCollection {
   [Pure]
   readonly attribute unsigned long length;

components/script/dom/webidls/HTMLFormControlsCollection.webidl

@@ -4,6 +4,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // https://html.spec.whatwg.org/multipage/#htmlformcontrolscollection
+[LegacyUnenumerableNamedProperties]
 interface HTMLFormControlsCollection : HTMLCollection {
   // inherits length and item()
   getter (RadioNodeList or Element)? namedItem(DOMString name); // shadows inherited namedItem()

components/script/dom/webidls/NamedNodeMap.webidl

@@ -4,6 +4,7 @@
 
 // https://dom.spec.whatwg.org/#interface-namednodemap
 
+[LegacyUnenumerableNamedProperties]
 interface NamedNodeMap {
   [Pure]
   readonly attribute unsigned long length;

tests/wpt/metadata/dom/nodes/Document-getElementsByTagName.html.ini

@@ -3,6 +3,3 @@
   [Shouldn't be able to set unsigned properties on a HTMLCollection (strict mode)]
     expected: FAIL
 
-  [hasOwnProperty, getOwnPropertyDescriptor, getOwnPropertyNames]
-    expected: FAIL
-

tests/wpt/metadata/dom/nodes/Element-children.html.ini

@@ -1,5 +0,0 @@
-[Element-children.html]
-  type: testharness
-  [HTMLCollection edge cases 1]
-    expected: FAIL
-

tests/wpt/metadata/dom/nodes/Element-getElementsByTagName.html.ini

@@ -3,6 +3,3 @@
   [Shouldn't be able to set unsigned properties on a HTMLCollection (strict mode)]
     expected: FAIL
 
-  [hasOwnProperty, getOwnPropertyDescriptor, getOwnPropertyNames]
-    expected: FAIL
-

tests/wpt/metadata/dom/nodes/attributes.html.ini

@@ -6,18 +6,6 @@
   [getAttributeNames tests]
     expected: FAIL
 
-  [Own property correctness with basic attributes]
-    expected: FAIL
-
-  [Own property correctness with non-namespaced attribute before same-name namespaced one]
-    expected: FAIL
-
-  [Own property correctness with namespaced attribute before same-name non-namespaced one]
-    expected: FAIL
-
-  [Own property correctness with two namespaced attributes with the same name-with-prefix]
-    expected: FAIL
-
   [Own property names should only include all-lowercase qualified names for an HTML element in an HTML document]
     expected: FAIL
 

tests/wpt/metadata/html/dom/documents/dom-tree-accessors/document.forms.html.ini

@@ -1,5 +0,0 @@
-[document.forms.html]
-  type: testharness
-  [document.forms iteration]
-    expected: FAIL
-