From 0607cd3fb54250f7b6e3b4028879a3eda1e3688a Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 14 Jun 2015 16:01:11 +0200 Subject: [PATCH 1/3] Return Fallible from get_callable_property. --- components/script/dom/bindings/callback.rs | 10 +++++----- components/script/dom/bindings/codegen/CodegenRust.py | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/components/script/dom/bindings/callback.rs b/components/script/dom/bindings/callback.rs index 975e3cb99d9..7a1c12f64d3 100644 --- a/components/script/dom/bindings/callback.rs +++ b/components/script/dom/bindings/callback.rs @@ -4,6 +4,7 @@ //! Base classes to work with IDL callbacks. +use dom::bindings::error::{Fallible, Error}; use dom::bindings::global::global_object_for_js_object; use dom::bindings::js::JSRef; use dom::bindings::utils::Reflectable; @@ -93,22 +94,21 @@ impl CallbackInterface { } /// Returns the property with the given `name`, if it is a callable object, - /// or `Err(())` otherwise. If it returns `Err(())`, a JSAPI exception is - /// pending. + /// or an error otherwise. pub fn get_callable_property(&self, cx: *mut JSContext, name: &str) - -> Result { + -> Fallible { let mut callable = UndefinedValue(); unsafe { let name = CString::new(name).unwrap(); if JS_GetProperty(cx, self.callback(), name.as_ptr(), &mut callable) == 0 { - return Err(()); + return Err(Error::JSFailed); } if !callable.is_object() || JS_ObjectIsCallable(cx, callable.to_object()) == 0 { // FIXME(#347) //ThrowErrorMessage(cx, MSG_NOT_CALLABLE, description.get()); - return Err(()); + return Err(Error::JSFailed); } } Ok(callable) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index cf7fcfd8e4c..78d8b2f7f6f 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -5304,10 +5304,8 @@ class CallbackOperationBase(CallbackMethod): "methodName": self.methodName } getCallableFromProp = string.Template( - 'match self.parent.get_callable_property(cx, "${methodName}") {\n' - ' Err(_) => return Err(JSFailed),\n' - ' Ok(callable) => callable,\n' - '}').substitute(replacements) + 'try!(self.parent.get_callable_property(cx, "${methodName}"))' + ).substitute(replacements) if not self.singleOperation: return 'JS::Rooted callable(cx);\n' + getCallableFromProp return ( From 90a7ef15718d8786771c1631de7739d00c97f7d0 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 14 Jun 2015 16:45:28 +0200 Subject: [PATCH 2/3] Throw a TypeError when get_callable_property encounters a value that isn't callable. --- components/script/dom/bindings/callback.rs | 9 ++++----- .../traversal/TreeWalker-acceptNode-filter.html.ini | 3 --- .../dom/traversal/TreeWalker-acceptNode-filter.html | 10 ++++------ 3 files changed, 8 insertions(+), 14 deletions(-) delete mode 100644 tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini diff --git a/components/script/dom/bindings/callback.rs b/components/script/dom/bindings/callback.rs index 7a1c12f64d3..4baccae098e 100644 --- a/components/script/dom/bindings/callback.rs +++ b/components/script/dom/bindings/callback.rs @@ -99,16 +99,15 @@ impl CallbackInterface { -> Fallible { let mut callable = UndefinedValue(); unsafe { - let name = CString::new(name).unwrap(); - if JS_GetProperty(cx, self.callback(), name.as_ptr(), &mut callable) == 0 { + let c_name = CString::new(name).unwrap(); + if JS_GetProperty(cx, self.callback(), c_name.as_ptr(), &mut callable) == 0 { return Err(Error::JSFailed); } if !callable.is_object() || JS_ObjectIsCallable(cx, callable.to_object()) == 0 { - // FIXME(#347) - //ThrowErrorMessage(cx, MSG_NOT_CALLABLE, description.get()); - return Err(Error::JSFailed); + return Err(Error::Type( + format!("The value of the {} property is not callable", name))); } } Ok(callable) diff --git a/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini b/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini deleted file mode 100644 index a0c22ee211b..00000000000 --- a/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[TreeWalker-acceptNode-filter.html] - type: testharness - expected: CRASH diff --git a/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html b/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html index 1bd28f3b885..b7f89eeea89 100644 --- a/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html +++ b/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html @@ -85,23 +85,21 @@ test(function() assert_node(walker.currentNode, { type: Element, id: 'B1' }); }, 'Testing with undefined filter'); -// XXX Servo breaks the test when a callback isn't callable test(function() { var walker = document.createTreeWalker(testElement, /*NodeFilter.*/SHOW_ELEMENT, {}); - assert_throws(null, function () { walker.firstChild(); }); + assert_throws(new TypeError(), function () { walker.firstChild(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); - assert_throws(null, function () { walker.nextNode(); }); + assert_throws(new TypeError(), function () { walker.nextNode(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); }, 'Testing with object lacking acceptNode property'); -// XXX Servo breaks the test when a callback isn't callable test(function() { var walker = document.createTreeWalker(testElement, /*NodeFilter.*/SHOW_ELEMENT, { acceptNode: "foo" }); - assert_throws(null, function () { walker.firstChild(); }); + assert_throws(new TypeError(), function () { walker.firstChild(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); - assert_throws(null, function () { walker.nextNode(); }); + assert_throws(new TypeError(), function () { walker.nextNode(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); }, 'Testing with object with non-function acceptNode property'); From 5cc130727faf65d1da0261ee0ddfadb49c151869 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 14 Jun 2015 17:33:05 +0200 Subject: [PATCH 3/3] Test that the exception doesn't get lost in acceptNode. And it turns out that it does get lost. I have no idea where, but I suspect it is in SpiderMonkey somewhere. I hope the SpiderMonkey upgrade will fix it. --- .../TreeWalker-acceptNode-filter.html.ini | 8 ++++++++ .../TreeWalker-acceptNode-filter.html | 18 ++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) create mode 100644 tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini diff --git a/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini b/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini new file mode 100644 index 00000000000..e67e45df785 --- /dev/null +++ b/tests/wpt/metadata/dom/traversal/TreeWalker-acceptNode-filter.html.ini @@ -0,0 +1,8 @@ +[TreeWalker-acceptNode-filter.html] + type: testharness + [Testing with filter function that throws] + expected: FAIL + + [Testing with filter object that throws] + expected: FAIL + diff --git a/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html b/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html index b7f89eeea89..769d9c2e248 100644 --- a/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html +++ b/tests/wpt/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html @@ -123,33 +123,31 @@ test(function() assert_node(walker.firstChild(), { type: Element, id: 'A1' }); }, 'Testing acceptNode callee'); -// XXX Looks like Servo is doing something wrong when a callback function throws test(function() { + var test_error = { name: "test" }; var walker = document.createTreeWalker(testElement, /*NodeFilter.*/SHOW_ELEMENT, function(node) { - throw('filter exception'); - return /*NodeFilter.*/FILTER_ACCEPT; + throw test_error; }); - assert_throws(null, function () { walker.firstChild(); }); + assert_throws(test_error, function () { walker.firstChild(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); - assert_throws(null, function () { walker.nextNode(); }); + assert_throws(test_error, function () { walker.nextNode(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); }, 'Testing with filter function that throws'); -// XXX Looks like Servo is doing something wrong when a callback function throws test(function() { + var test_error = { name: "test" }; var walker = document.createTreeWalker(testElement, /*NodeFilter.*/SHOW_ELEMENT, { acceptNode : function(node) { - throw('filter exception'); - return /*NodeFilter.*/FILTER_ACCEPT; + throw test_error; } }); - assert_throws(null, function () { walker.firstChild(); }); + assert_throws(test_error, function () { walker.firstChild(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); - assert_throws(null, function () { walker.nextNode(); }); + assert_throws(test_error, function () { walker.nextNode(); }); assert_node(walker.currentNode, { type: Element, id: 'root' }); }, 'Testing with filter object that throws');