"Legacy platform object" getOwnPropertyDescriptor returns undefined when it should be finding dynamic properties

[pshaughn]

WebIDL/ecmascript-binding/legacy-platform-object.html tests out various things "legacy platform objects" (https://heycam.github.io/webidl/#es-legacy-platform-objects) are expected to do, most of which involving pretending to be arrays or open-ended hashmaps in one way or another. Mutliple tests fail on getOwnPropertyDescriptor, which is not seeing the dynamic properties.

HTMLSelectElement insufficiently pretending to be an array is also in #25003; there is possibly overlap, if the reason getOwnPropertyDescriptor isn't working is that the property isn't even there to get.

Other failures are DOMTokenList with numeric properties and DOMStringMap with named ones.

[jdm]

Relevant to getOwnPropertyDescriptor for legacy objects:

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

Lines 5130 to 5228 in 54d88fd

	class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod):
	def __init__(self, descriptor):
	args = [Argument('*mut JSContext', 'cx'), Argument('RawHandleObject', 'proxy'),
	Argument('RawHandleId', 'id'),
	Argument('RawMutableHandle<PropertyDescriptor>', 'mut desc')]
	CGAbstractExternMethod.__init__(self, descriptor, "getOwnPropertyDescriptor",
	"bool", args)
	self.descriptor = descriptor
	# https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty
	def getBody(self):
	indexedGetter = self.descriptor.operations['IndexedGetter']
	get = "let cx = SafeJSContext::from_ptr(cx);\n"
	if indexedGetter:
	get += "let index = get_array_index_from_id(*cx, Handle::from_raw(id));\n"
	attrs = "JSPROP_ENUMERATE"
	if self.descriptor.operations['IndexedSetter'] is None:
	attrs += " | JSPROP_READONLY"
	# FIXME(#11868) Should assign to desc.value, desc.get() is a copy.
	fillDescriptor = ("desc.get().value = result_root.get();\n"
	"fill_property_descriptor(MutableHandle::from_raw(desc), proxy.get(), (%s) as u32);\n"
	"return true;" % attrs)
	templateValues = {
	'jsvalRef': 'result_root.handle_mut()',
	'successCode': fillDescriptor,
	'pre': 'rooted!(in(*cx) let mut result_root = UndefinedValue());'
	}
	get += ("if let Some(index) = index {\n" +
	" let this = UnwrapProxy(proxy);\n" +
	" let this = &*this;\n" +
	CGIndenter(CGProxyIndexedGetter(self.descriptor, templateValues)).define() + "\n" +
	"}\n")
	namedGetter = self.descriptor.operations['NamedGetter']
	if namedGetter:
	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"
	# FIXME(#11868) Should assign to desc.value, desc.get() is a copy.
	fillDescriptor = ("desc.get().value = result_root.get();\n"
	"fill_property_descriptor(MutableHandle::from_raw(desc), proxy.get(), (%s) as u32);\n"
	"return true;" % attrs)
	templateValues = {
	'jsvalRef': 'result_root.handle_mut()',
	'successCode': fillDescriptor,
	'pre': 'rooted!(in(*cx) let mut result_root = UndefinedValue());'
	}
	# See the similar-looking in CGDOMJSProxyHandler_get for the spec quote.
	condition = "RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id)"
	if indexedGetter:
	condition = "index.is_none() && (%s)" % condition
	# Once we start supporting OverrideBuiltins we need to make
	# ResolveOwnProperty or EnumerateOwnProperties filter out named
	# properties that shadow prototype properties.
	namedGet = """
	if %s {
	let mut has_on_proto = false;
	if !has_property_on_prototype(*cx, proxy_lt, id_lt, &mut has_on_proto) {
	return false;
	}
	if !has_on_proto {
	%s
	}
	}
	""" % (condition, CGIndenter(CGProxyNamedGetter(self.descriptor, templateValues), 8).define())
	else:
	namedGet = ""
	return get + """\
	rooted!(in(*cx) let mut expando = ptr::null_mut::<JSObject>());
	get_expando_object(proxy, expando.handle_mut());
	//if (!xpc::WrapperFactory::IsXrayWrapper(proxy) && (expando = GetExpandoObject(proxy))) {
	let proxy_lt = Handle::from_raw(proxy);
	let id_lt = Handle::from_raw(id);
	if !expando.is_null() {
	if !JS_GetPropertyDescriptorById(*cx, expando.handle().into(), id, desc) {
	return false;
	}
	if !desc.obj.is_null() {
	// Pretend the property lives on the wrapper.
	desc.obj = proxy.get();
	return true;
	}
	}
	""" + namedGet + """\
	desc.get().obj = ptr::null_mut();
	return true;"""
	def definition_body(self):
	return CGGeneric(self.getBody())

[pshaughn]

I don't understand that code very well. In the named getter case, where is it actually looking up the name?

[pshaughn]

servo/tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-nameditem.html

Lines 112 to 118 in 5c7a4db

	test(function() {
	var form = document.getElementsByTagName("form")[0]
	var button = document.getElementsByTagName("input")[0]
	assert_equals(button.type, "button")
	assert_equals(form.button, button)
	var desc = Object.getOwnPropertyDescriptor(form, "button");
	assert_equals(desc.value, button);

this test seems to be getting back a descriptor that exists, but that has .value=undefined. Could that be related to the FIXME(#11868) not setting the real value?

[jdm]

That seems entirely plausible, yes.

[pshaughn]

Removing the .get() gets us three asserts further down in this particular test (failing on desc.enumerable); if it doesn't break anything else, it's an improvement.

[jdm]

What's the enumerable failure? The descriptor returned should be unconditionally enumerable based on the this line.

[pshaughn]

The test wants it to not be enumerable:

servo/tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-nameditem.html

Line 121 in 5c7a4db

assert_false(desc.enumerable);

[jdm]

Aha, that comes from step 2.8 of https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty, since HTMLFormElement is listed in https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties.

[pshaughn]

I see how that happened. The build is set up to fail if that extended attribute's specified without a named getter, and HTMLFormElement didn't have one yet when the extended attribute was being handed out. Window will also need it once it gets a named getter, and HTMLAllCollection once it exists.

[pshaughn]

Updated #25446 now that I know it will fix this and not break anything else.

[pshaughn]

remaining WebIDL/ecmascript-binding/legacy-platform-object.html failures are HTMLSelectElement-specific and probably just its lack of an indexed setter, #25003