From f5d16fc069533b810a76a2fa611882e1b9c8743c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 17 Sep 2017 19:25:58 +0200 Subject: [PATCH 1/3] script: Fix integer-JSID handling in named getters. Fixes #10686 --- .../dom/bindings/codegen/CodegenRust.py | 14 ++-- .../webstorage/storage_setitem.html.ini | 64 +------------------ 2 files changed, 11 insertions(+), 67 deletions(-) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index 0ad87747a12..c42ec783716 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -4750,7 +4750,7 @@ class CGProxyNamedOperation(CGProxySpecialOperation): def define(self): # Our first argument is the id we're getting. argName = self.arguments[0].identifier.name - return ("let %s = string_jsid_to_string(cx, id);\n" + return ("let %s = jsid_to_string(cx, id).expect(\"Not a string-convertible JSID?\");\n" "let this = UnwrapProxy(proxy);\n" "let this = &*this;\n" % argName + CGProxySpecialOperation.define(self)) @@ -4868,7 +4868,7 @@ class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod): # ResolveOwnProperty or EnumerateOwnProperties filter out named # properties that shadow prototype properties. namedGet = """ -if RUST_JSID_IS_STRING(id) { +if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { let mut has_on_proto = false; if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) { return false; @@ -4935,12 +4935,12 @@ class CGDOMJSProxyHandler_defineProperty(CGAbstractExternMethod): if self.descriptor.hasUnforgeableMembers: raise TypeError("Can't handle a named setter on an interface that has " "unforgeables. Figure out how that should work!") - set += ("if RUST_JSID_IS_STRING(id) {\n" + + set += ("if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {\n" + CGIndenter(CGProxyNamedSetter(self.descriptor)).define() + " return (*opresult).succeed();\n" + "}\n") else: - set += ("if RUST_JSID_IS_STRING(id) {\n" + + set += ("if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {\n" + CGIndenter(CGProxyNamedGetter(self.descriptor)).define() + " if result.is_some() {\n" " return (*opresult).failNoNamedSetter();\n" @@ -5095,7 +5095,7 @@ class CGDOMJSProxyHandler_hasOwn(CGAbstractExternMethod): namedGetter = self.descriptor.operations['NamedGetter'] if namedGetter: named = """\ -if RUST_JSID_IS_STRING(id) { +if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { let mut has_on_proto = false; if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) { return false; @@ -5175,7 +5175,7 @@ if !expando.is_null() { namedGetter = self.descriptor.operations['NamedGetter'] if namedGetter: - getNamed = ("if RUST_JSID_IS_STRING(id) {\n" + + getNamed = ("if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {\n" + CGIndenter(CGProxyNamedGetter(self.descriptor, templateValues)).define() + "}\n") else: @@ -5633,6 +5633,7 @@ def generate_imports(config, cgthings, descriptors, callbacks=None, dictionaries 'js::glue::GetProxyPrivate', 'js::glue::NewProxyObject', 'js::glue::ProxyTraps', + 'js::glue::RUST_JSID_IS_INT', 'js::glue::RUST_JSID_IS_STRING', 'js::glue::RUST_SYMBOL_TO_JSID', 'js::glue::int_to_jsid', @@ -5720,6 +5721,7 @@ def generate_imports(config, cgthings, descriptors, callbacks=None, dictionaries 'dom::bindings::conversions::root_from_handlevalue', 'dom::bindings::conversions::root_from_object', 'dom::bindings::conversions::string_jsid_to_string', + 'dom::bindings::conversions::jsid_to_string', 'dom::bindings::codegen::PrototypeList', 'dom::bindings::codegen::RegisterBindings', 'dom::bindings::codegen::UnionTypes', diff --git a/tests/wpt/metadata/webstorage/storage_setitem.html.ini b/tests/wpt/metadata/webstorage/storage_setitem.html.ini index ca08ab7a750..0f95dbcbb45 100644 --- a/tests/wpt/metadata/webstorage/storage_setitem.html.ini +++ b/tests/wpt/metadata/webstorage/storage_setitem.html.ini @@ -12,73 +12,15 @@ expected: FAIL bug: https://github.com/servo/servo/issues/6564 - [localStorage["0"\]] - expected: FAIL - bug: https://github.com/servo/servo/issues/10686 - - [localStorage["1"\]] - expected: FAIL - - [localStorage["2"\]] - expected: FAIL - - [localStorage["3"\]] - expected: FAIL - - [localStorage["4"\]] - expected: FAIL - - [localStorage["5"\]] - expected: FAIL - - [localStorage["6"\]] - expected: FAIL - - [localStorage["7"\]] - expected: FAIL - - [localStorage["8"\]] - expected: FAIL - - [localStorage["9"\]] - expected: FAIL - [sessionStorage[\] = "�"] expected: FAIL + bug: https://github.com/servo/servo/issues/6564 [sessionStorage[\] = "�a"] expected: FAIL + bug: https://github.com/servo/servo/issues/6564 [sessionStorage[\] = "a�"] expected: FAIL - - [sessionStorage["0"\]] - expected: FAIL - - [sessionStorage["1"\]] - expected: FAIL - - [sessionStorage["2"\]] - expected: FAIL - - [sessionStorage["3"\]] - expected: FAIL - - [sessionStorage["4"\]] - expected: FAIL - - [sessionStorage["5"\]] - expected: FAIL - - [sessionStorage["6"\]] - expected: FAIL - - [sessionStorage["7"\]] - expected: FAIL - - [sessionStorage["8"\]] - expected: FAIL - - [sessionStorage["9"\]] - expected: FAIL + bug: https://github.com/servo/servo/issues/6564 From 6edefb829c2417b763478cde3fbbc483ed196b04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 17 Sep 2017 19:33:11 +0200 Subject: [PATCH 2/3] script: remove unused function. --- .../script/dom/bindings/codegen/CodegenRust.py | 1 - components/script/dom/bindings/conversions.rs | 14 -------------- 2 files changed, 15 deletions(-) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index c42ec783716..afb189ec795 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -5720,7 +5720,6 @@ def generate_imports(config, cgthings, descriptors, callbacks=None, dictionaries 'dom::bindings::conversions::root_from_handleobject', 'dom::bindings::conversions::root_from_handlevalue', 'dom::bindings::conversions::root_from_object', - 'dom::bindings::conversions::string_jsid_to_string', 'dom::bindings::conversions::jsid_to_string', 'dom::bindings::codegen::PrototypeList', 'dom::bindings::codegen::RegisterBindings', diff --git a/components/script/dom/bindings/conversions.rs b/components/script/dom/bindings/conversions.rs index 6ae233aa034..10accbcc62b 100644 --- a/components/script/dom/bindings/conversions.rs +++ b/components/script/dom/bindings/conversions.rs @@ -132,20 +132,6 @@ impl FromJSValConvertible for RootedTrac } } -/// Convert `id` to a `DOMString`, assuming it is string-valued. -/// -/// Handling of invalid UTF-16 in strings depends on the relevant option. -/// -/// # Panics -/// -/// Panics if `id` is not string-valued. -pub fn string_jsid_to_string(cx: *mut JSContext, id: HandleId) -> DOMString { - unsafe { - assert!(RUST_JSID_IS_STRING(id)); - jsstring_to_str(cx, RUST_JSID_TO_STRING(id)) - } -} - /// Convert `id` to a `DOMString`. Returns `None` if `id` is not a string or /// integer. /// From 90fb2847207678e7feee06eaf17d75ac359d6a07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 18 Sep 2017 06:55:08 +0200 Subject: [PATCH 3/3] script: Properly implement LegacyPlatformGetOwnProperty in WebIDL. We were missing the "ignoreNamedProperties" bit, which my previous patch uncovers. --- .../dom/bindings/codegen/CodegenRust.py | 33 ++++++++++++++----- .../webstorage/storage_indexing.html.ini | 8 ----- 2 files changed, 24 insertions(+), 17 deletions(-) delete mode 100644 tests/wpt/metadata/webstorage/storage_indexing.html.ini diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index afb189ec795..174bda2b0ca 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -4817,15 +4817,14 @@ class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod): "bool", args) self.descriptor = descriptor + # https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty def getBody(self): indexedGetter = self.descriptor.operations['IndexedGetter'] - indexedSetter = self.descriptor.operations['IndexedSetter'] get = "" - if indexedGetter or indexedSetter: + if indexedGetter: get = "let index = get_array_index_from_id(cx, id);\n" - if indexedGetter: attrs = "JSPROP_ENUMERATE" if self.descriptor.operations['IndexedSetter'] is None: attrs += " | JSPROP_READONLY" @@ -4864,11 +4863,16 @@ class CGDOMJSProxyHandler_getOwnPropertyDescriptor(CGAbstractExternMethod): '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 RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { +if %s { let mut has_on_proto = false; if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) { return false; @@ -4877,7 +4881,7 @@ if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { %s } } -""" % CGIndenter(CGProxyNamedGetter(self.descriptor, templateValues), 8).define() +""" % (condition, CGIndenter(CGProxyNamedGetter(self.descriptor, templateValues), 8).define()) else: namedGet = "" @@ -5093,9 +5097,12 @@ class CGDOMJSProxyHandler_hasOwn(CGAbstractExternMethod): indexed = "" namedGetter = self.descriptor.operations['NamedGetter'] + condition = "RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id)" + if indexedGetter: + condition = "index.is_none() && (%s)" % condition if namedGetter: named = """\ -if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { +if %s { let mut has_on_proto = false; if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) { return false; @@ -5107,7 +5114,7 @@ if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) { } } -""" % CGIndenter(CGProxyNamedGetter(self.descriptor), 8).define() +""" % (condition, CGIndenter(CGProxyNamedGetter(self.descriptor), 8).define()) else: named = "" @@ -5136,6 +5143,7 @@ class CGDOMJSProxyHandler_get(CGAbstractExternMethod): CGAbstractExternMethod.__init__(self, descriptor, "get", "bool", args) self.descriptor = descriptor + # https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty def getBody(self): getFromExpando = """\ rooted!(in(cx) let mut expando = ptr::null_mut()); @@ -5175,9 +5183,16 @@ if !expando.is_null() { namedGetter = self.descriptor.operations['NamedGetter'] if namedGetter: - getNamed = ("if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {\n" + + condition = "RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id)" + # From step 1: + # If O supports indexed properties and P is an array index, then: + # + # 3. Set ignoreNamedProps to true. + if indexedGetter: + condition = "index.is_none() && (%s)" % condition + getNamed = ("if %s {\n" + CGIndenter(CGProxyNamedGetter(self.descriptor, templateValues)).define() + - "}\n") + "}\n") % condition else: getNamed = "" diff --git a/tests/wpt/metadata/webstorage/storage_indexing.html.ini b/tests/wpt/metadata/webstorage/storage_indexing.html.ini deleted file mode 100644 index 36eb6983c9f..00000000000 --- a/tests/wpt/metadata/webstorage/storage_indexing.html.ini +++ /dev/null @@ -1,8 +0,0 @@ -[storage_indexing.html] - type: testharness - [Getting existing number-valued properties on localStorage] - expected: FAIL - - [Getting existing number-valued properties on sessionStorage] - expected: FAIL -