Auto merge of #8430 - jdm:errorhandler, r=Ms2ger

Invoke specialized callback behaviour for OnErrorEventHandler.

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

components/script/dom/document.rs

@@ -9,6 +9,7 @@ use dom::bindings::codegen::Bindings::DocumentBinding;
 use dom::bindings::codegen::Bindings::DocumentBinding::{DocumentMethods, DocumentReadyState};
 use dom::bindings::codegen::Bindings::EventBinding::EventMethods;
 use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
+use dom::bindings::codegen::Bindings::EventHandlerBinding::OnErrorEventHandlerNonNull;
 use dom::bindings::codegen::Bindings::EventTargetBinding::EventTargetMethods;
 use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
 use dom::bindings::codegen::Bindings::NodeFilterBinding::NodeFilter;

components/script/dom/eventtarget.rs

@@ -2,19 +2,26 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use dom::bindings::callback::{CallbackContainer, ExceptionHandling};
+use dom::bindings::callback::{CallbackContainer, ExceptionHandling, CallbackFunction};
 use dom::bindings::cell::DOMRefCell;
+use dom::bindings::codegen::Bindings::ErrorEventBinding::ErrorEventMethods;
 use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
+use dom::bindings::codegen::Bindings::EventHandlerBinding::OnErrorEventHandlerNonNull;
 use dom::bindings::codegen::Bindings::EventListenerBinding::EventListener;
 use dom::bindings::codegen::Bindings::EventTargetBinding::EventTargetMethods;
+use dom::bindings::codegen::UnionTypes::EventOrString;
 use dom::bindings::error::{Error, Fallible, report_pending_exception};
-use dom::bindings::inheritance::EventTargetTypeId;
+use dom::bindings::global::global_root_from_reflector;
+use dom::bindings::inheritance::{Castable, EventTargetTypeId};
+use dom::bindings::js::Root;
 use dom::bindings::reflector::{Reflectable, Reflector};
+use dom::errorevent::ErrorEvent;
 use dom::event::Event;
 use dom::eventdispatcher::dispatch_event;
 use dom::virtualmethods::VirtualMethods;
+use dom::window::Window;
 use fnv::FnvHasher;
-use js::jsapi::{CompileFunction, JS_GetFunctionObject};
+use js::jsapi::{CompileFunction, JS_GetFunctionObject, RootedValue};
 use js::jsapi::{HandleObject, JSContext, RootedFunction};
 use js::jsapi::{JSAutoCompartment, JSAutoRequest};
 use js::rust::{AutoObjectVectorWrapper, CompileOptionsWrapper};
@@ -31,7 +38,20 @@ use url::Url;
 use util::mem::HeapSizeOf;
 use util::str::DOMString;
 
-pub type EventHandler = EventHandlerNonNull;
+#[derive(PartialEq, Clone, JSTraceable)]
+pub enum CommonEventHandler {
+    EventHandler(Rc<EventHandlerNonNull>),
+    ErrorEventHandler(Rc<OnErrorEventHandlerNonNull>),
+}
+
+impl CommonEventHandler {
+    fn parent(&self) -> &CallbackFunction {
+        match *self {
+            CommonEventHandler::EventHandler(ref handler) => &handler.parent,
+            CommonEventHandler::ErrorEventHandler(ref handler) => &handler.parent,
+        }
+    }
+}
 
 #[derive(JSTraceable, Copy, Clone, PartialEq, HeapSizeOf)]
 pub enum ListenerPhase {
@@ -78,7 +98,7 @@ impl EventTargetTypeId {
 #[derive(JSTraceable, Clone, PartialEq)]
 pub enum EventListenerType {
     Additive(Rc<EventListener>),
-    Inline(Rc<EventHandler>),
+    Inline(CommonEventHandler),
 }
 
 impl HeapSizeOf for EventListenerType {
@@ -89,18 +109,45 @@ impl HeapSizeOf for EventListenerType {
 }
 
 impl EventListenerType {
+    // https://html.spec.whatwg.org/multipage/#the-event-handler-processing-algorithm
     pub fn call_or_handle_event<T: Reflectable>(&self,
                                                 object: &T,
                                                 event: &Event,
                                                 exception_handle: ExceptionHandling) {
+        // Step 3
         match *self {
             EventListenerType::Additive(ref listener) => {
                 let _ = listener.HandleEvent_(object, event, exception_handle);
             },
             EventListenerType::Inline(ref handler) => {
-                let _ = handler.Call_(object, event, exception_handle);
+                match *handler {
+                    CommonEventHandler::ErrorEventHandler(ref handler) => {
+                        if let Some(event) = event.downcast::<ErrorEvent>() {
+                            let global = global_root_from_reflector(object);
+                            let cx = global.r().get_cx();
+                            let error = RootedValue::new(cx, event.Error(cx));
+                            let _ = handler.Call_(object,
+                                                  EventOrString::eString(event.Message()),
+                                                  Some(event.Filename()),
+                                                  Some(event.Lineno()),
+                                                  Some(event.Colno()),
+                                                  Some(error.handle()),
+                                                  exception_handle);
+                            return;
+                        }
+
+                        let _ = handler.Call_(object, EventOrString::eEvent(Root::from_ref(event)),
+                                              None, None, None, None, exception_handle);
+                    }
+
+                    CommonEventHandler::EventHandler(ref handler) => {
+                        let _ = handler.Call_(object, event, exception_handle);
+                    }
+                }
             },
         }
+
+        // TODO(#8490): step 4 (cancel event based on return value)
     }
 }
 
@@ -151,7 +198,7 @@ impl EventTarget {
 
     pub fn set_inline_event_listener(&self,
                                      ty: Atom,
-                                     listener: Option<Rc<EventHandler>>) {
+                                     listener: Option<CommonEventHandler>) {
         let mut handlers = self.handlers.borrow_mut();
         let entries = match handlers.entry(ty) {
             Occupied(entry) => entry.into_mut(),
@@ -185,7 +232,7 @@ impl EventTarget {
         }
     }
 
-    pub fn get_inline_event_listener(&self, ty: &Atom) -> Option<Rc<EventHandler>> {
+    pub fn get_inline_event_listener(&self, ty: &Atom) -> Option<CommonEventHandler> {
         let handlers = self.handlers.borrow();
         let entries = handlers.get(ty);
         entries.and_then(|entries| entries.iter().filter_map(|entry| {
@@ -197,6 +244,7 @@ impl EventTarget {
     }
 
     #[allow(unsafe_code)]
+    // https://html.spec.whatwg.org/multipage/#getting-the-current-value-of-the-event-handler
     pub fn set_event_handler_uncompiled(&self,
                                     cx: *mut JSContext,
                                     url: Url,
@@ -207,8 +255,21 @@ impl EventTarget {
         let name = CString::new(ty).unwrap();
         let lineno = 0; //XXXjdm need to get a real number here
 
-        let nargs = 1; //XXXjdm not true for onerror
         static mut ARG_NAMES: [*const c_char; 1] = [b"event\0" as *const u8 as *const c_char];
+        static mut ERROR_ARG_NAMES: [*const c_char; 5] = [b"event\0" as *const u8 as *const c_char,
+                                                          b"source\0" as *const u8 as *const c_char,
+                                                          b"lineno\0" as *const u8 as *const c_char,
+                                                          b"colno\0" as *const u8 as *const c_char,
+                                                          b"error\0" as *const u8 as *const c_char];
+        // step 10
+        let is_error = ty == "error" && self.is::<Window>();
+        let args = unsafe {
+            if is_error {
+                &ERROR_ARG_NAMES[..]
+            } else {
+                &ARG_NAMES[..]
+            }
+        };
 
         let source: Vec<u16> = source.utf16_units().collect();
         let options = CompileOptionsWrapper::new(cx, url.as_ptr(), lineno);
@@ -222,8 +283,8 @@ impl EventTarget {
                             scopechain.ptr,
                             options.ptr,
                             name.as_ptr(),
-                            nargs,
+                            args.len() as u32,
-                            ARG_NAMES.as_mut_ptr(),
+                            args.as_ptr(),
                             source.as_ptr(),
                             source.len() as size_t,
                             handler.handle_mut())
@@ -235,20 +296,34 @@ impl EventTarget {
 
         let funobj = unsafe { JS_GetFunctionObject(handler.ptr) };
         assert!(!funobj.is_null());
-        self.set_event_handler_common(ty, Some(EventHandlerNonNull::new(funobj)));
+        if is_error {
+            self.set_error_event_handler(ty, Some(OnErrorEventHandlerNonNull::new(funobj)));
+        } else {
+            self.set_event_handler_common(ty, Some(EventHandlerNonNull::new(funobj)));
+        }
     }
 
     pub fn set_event_handler_common<T: CallbackContainer>(
         &self, ty: &str, listener: Option<Rc<T>>)
     {
         let event_listener = listener.map(|listener|
-                                          EventHandlerNonNull::new(listener.callback()));
+                                          CommonEventHandler::EventHandler(
+                                              EventHandlerNonNull::new(listener.callback())));
+        self.set_inline_event_listener(Atom::from_slice(ty), event_listener);
+    }
+
+    pub fn set_error_event_handler<T: CallbackContainer>(
+        &self, ty: &str, listener: Option<Rc<T>>)
+    {
+        let event_listener = listener.map(|listener|
+                                          CommonEventHandler::ErrorEventHandler(
+                                              OnErrorEventHandlerNonNull::new(listener.callback())));
         self.set_inline_event_listener(Atom::from_slice(ty), event_listener);
     }
 
     pub fn get_event_handler_common<T: CallbackContainer>(&self, ty: &str) -> Option<Rc<T>> {
         let listener = self.get_inline_event_listener(&Atom::from_slice(ty));
-        listener.map(|listener| CallbackContainer::new(listener.parent.callback()))
+        listener.map(|listener| CallbackContainer::new(listener.parent().callback()))
     }
 
     pub fn has_handlers(&self) -> bool {

components/script/dom/htmlbodyelement.rs

@@ -153,34 +153,43 @@ impl VirtualMethods for HTMLBodyElement {
     }
 
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
-        self.super_type().unwrap().attribute_mutated(attr, mutation);
+        let do_super_mutate = match (attr.local_name(), mutation) {
-        match (attr.local_name(), mutation) {
             (&atom!(background), _) => {
                 *self.background.borrow_mut() = mutation.new_value(attr).and_then(|value| {
                     let document = document_from_node(self);
                     let base = document.url();
                     UrlParser::new().base_url(&base).parse(&value).ok()
                 });
+                true
             },
             (name, AttributeMutation::Set(_)) if name.starts_with("on") => {
                 let window = window_from_node(self);
                 let (cx, url, reflector) = (window.get_cx(),
                                             window.get_url(),
                                             window.reflector().get_jsobject());
-                let evtarget = match name {
+                // https://html.spec.whatwg.org/multipage/
+                // #event-handlers-on-elements,-document-objects,-and-window-objects:event-handlers-3
+                match name {
                     &atom!(onfocus) | &atom!(onload) | &atom!(onscroll) | &atom!(onafterprint) |
                     &atom!(onbeforeprint) | &atom!(onbeforeunload) | &atom!(onhashchange) |
                     &atom!(onlanguagechange) | &atom!(onmessage) | &atom!(onoffline) | &atom!(ononline) |
                     &atom!(onpagehide) | &atom!(onpageshow) | &atom!(onpopstate) | &atom!(onstorage) |
                     &atom!(onresize) | &atom!(onunload) | &atom!(onerror)
-                      => window.upcast::<EventTarget>(), // forwarded event
+                      => {
-                    _ => self.upcast::<EventTarget>(),
+                          let evtarget = window.upcast::<EventTarget>(); // forwarded event
-                };
+                          evtarget.set_event_handler_uncompiled(cx, url, reflector,
-                evtarget.set_event_handler_uncompiled(cx, url, reflector,
+                                                                &name[2..],
-                                                      &name[2..],
+                                                                DOMString((**attr.value()).to_owned()));
-                                                      DOMString((**attr.value()).to_owned()));
+                          false
+                    }
+                    _ => true, // HTMLElement::attribute_mutated will take care of this.
+                }
             },
-            _ => {}
+            _ => true,
+        };
+
+        if do_super_mutate {
+            self.super_type().unwrap().attribute_mutated(attr, mutation);
         }
     }
 }

components/script/dom/htmlelement.rs

@@ -6,6 +6,7 @@ use dom::attr::Attr;
 use dom::attr::AttrValue;
 use dom::bindings::codegen::Bindings::ElementBinding::ElementMethods;
 use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
+use dom::bindings::codegen::Bindings::EventHandlerBinding::OnErrorEventHandlerNonNull;
 use dom::bindings::codegen::Bindings::HTMLElementBinding;
 use dom::bindings::codegen::Bindings::HTMLElementBinding::HTMLElementMethods;
 use dom::bindings::codegen::Bindings::HTMLInputElementBinding::HTMLInputElementMethods;

components/script/dom/macros.rs

@@ -279,7 +279,7 @@ macro_rules! no_jsmanaged_fields(
 
 /// These are used to generate a event handler which has no special case.
 macro_rules! define_event_handler(
-    ($handler: ident, $event_type: ident, $getter: ident, $setter: ident) => (
+    ($handler: ident, $event_type: ident, $getter: ident, $setter: ident, $setter_fn: ident) => (
         fn $getter(&self) -> Option<::std::rc::Rc<$handler>> {
             use dom::bindings::inheritance::Castable;
             use dom::eventtarget::EventTarget;
@@ -291,20 +291,22 @@ macro_rules! define_event_handler(
             use dom::bindings::inheritance::Castable;
             use dom::eventtarget::EventTarget;
             let eventtarget = self.upcast::<EventTarget>();
-            eventtarget.set_event_handler_common(stringify!($event_type), listener)
+            eventtarget.$setter_fn(stringify!($event_type), listener)
         }
     )
 );
 
 macro_rules! event_handler(
     ($event_type: ident, $getter: ident, $setter: ident) => (
-        define_event_handler!(EventHandlerNonNull, $event_type, $getter, $setter);
+        define_event_handler!(EventHandlerNonNull, $event_type, $getter, $setter,
+                              set_event_handler_common);
     )
 );
 
 macro_rules! error_event_handler(
     ($event_type: ident, $getter: ident, $setter: ident) => (
-        define_event_handler!(OnErrorEventHandlerNonNull, $event_type, $getter, $setter);
+        define_event_handler!(OnErrorEventHandlerNonNull, $event_type, $getter, $setter,
+                              set_error_event_handler);
     )
 );
 
@@ -319,6 +321,7 @@ macro_rules! global_event_handlers(
     );
     (NoOnload) => (
         event_handler!(click, GetOnclick, SetOnclick);
+        error_event_handler!(error, GetOnerror, SetOnerror);
         event_handler!(keydown, GetOnkeydown, SetOnkeydown);
         event_handler!(keypress, GetOnkeypress, SetOnkeypress);
         event_handler!(keyup, GetOnkeyup, SetOnkeyup);

components/script/dom/webidls/EventHandler.webidl

@@ -24,6 +24,7 @@ typedef OnErrorEventHandlerNonNull? OnErrorEventHandler;
 [NoInterfaceObject]
 interface GlobalEventHandlers {
            attribute EventHandler onclick;
+           attribute OnErrorEventHandler onerror;
            attribute EventHandler onload;
            attribute EventHandler oninput;
            attribute EventHandler onkeydown;
@@ -38,18 +39,3 @@ interface WindowEventHandlers {
            attribute EventHandler onunload;
            attribute EventHandler onstorage;
 };
-
-// The spec has |attribute OnErrorEventHandler onerror;| on
-// GlobalEventHandlers, and calls the handler differently depending on
-// whether an ErrorEvent was fired. We don't do that, and until we do we'll
-// need to distinguish between onerror on Window or on nodes.
-
-/*[NoInterfaceObject]
-interface OnErrorEventHandlerForNodes {
-           attribute EventHandler onerror;
-};*/
-
-[NoInterfaceObject]
-interface OnErrorEventHandlerForWindow {
-           attribute OnErrorEventHandler onerror;
-};

components/script/dom/webidls/Window.webidl

@@ -161,7 +161,6 @@ partial interface Window {
   void gc();
   void trap();
 };
-Window implements OnErrorEventHandlerForWindow;
 
 // WebDriver extensions
 partial interface Window {

components/script/dom/window.rs

@@ -511,9 +511,6 @@ impl WindowMethods for Window {
     // https://html.spec.whatwg.org/multipage/#handler-window-onstorage
     event_handler!(storage, GetOnstorage, SetOnstorage);
 
-    // https://html.spec.whatwg.org/multipage/#handler-onerror
-    error_event_handler!(error, GetOnerror, SetOnerror);
-
     // https://developer.mozilla.org/en-US/docs/Web/API/Window/screen
     fn Screen(&self) -> Root<Screen> {
         self.screen.or_init(|| Screen::new(self))

tests/wpt/metadata/MANIFEST.json

@@ -29826,6 +29826,12 @@
             "path": "html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html",
             "url": "/html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html"
           }
+        ],
+        "html/webappapis/scripting/events/onerroreventhandler.html": [
+          {
+            "path": "html/webappapis/scripting/events/onerroreventhandler.html",
+            "url": "/html/webappapis/scripting/events/onerroreventhandler.html"
+          }
         ]
       }
     },

tests/wpt/metadata/html/dom/interfaces.html.ini

@@ -135,9 +135,6 @@
   [Document interface: attribute onended]
     expected: FAIL
 
-  [Document interface: attribute onerror]
-    expected: FAIL
-
   [Document interface: attribute onfocus]
     expected: FAIL
 
@@ -1722,9 +1719,6 @@
   [HTMLElement interface: attribute onended]
     expected: FAIL
 
-  [HTMLElement interface: attribute onerror]
-    expected: FAIL
-
   [HTMLElement interface: attribute onfocus]
     expected: FAIL
 
@@ -1962,9 +1956,6 @@
   [HTMLElement interface: document.createElement("noscript") must inherit property "onended" with the proper type (55)]
     expected: FAIL
 
-  [HTMLElement interface: document.createElement("noscript") must inherit property "onerror" with the proper type (56)]
-    expected: FAIL
-
   [HTMLElement interface: document.createElement("noscript") must inherit property "onfocus" with the proper type (57)]
     expected: FAIL
 
@@ -9165,9 +9156,6 @@
   [Document interface: document.implementation.createDocument(null, "", null) must inherit property "onended" with the proper type (117)]
     expected: FAIL
 
-  [Document interface: document.implementation.createDocument(null, "", null) must inherit property "onerror" with the proper type (118)]
-    expected: FAIL
-
   [Document interface: document.implementation.createDocument(null, "", null) must inherit property "onfocus" with the proper type (119)]
     expected: FAIL
 

tests/wpt/web-platform-tests/html/webappapis/scripting/events/onerroreventhandler-frame.html

@@ -0,0 +1,56 @@
+<body></body>
+<script>
+function check1(args, callee) {
+  parent.t.step(function() {
+    parent.assert_equals(callee.length, 5);
+    parent.assert_equals(args.length, 5);
+    parent.assert_equals(args[0], reference_error.message);
+    parent.assert_equals(args[1], reference_error.filename);
+    parent.assert_equals(args[2], reference_error.lineno);
+    parent.assert_equals(args[3], reference_error.colno);
+    parent.assert_equals(args[4], reference_error.error);
+    parent.t.done();
+  });
+}
+
+var reference_error = new ErrorEvent("error", {
+  filename: "error_file.js",
+  lineno: 333,
+  colno: 999,
+  message: "there was an error",
+  error: {nondefault: 'some unusual object'},
+});
+
+parent.t.step(function() {
+  document.body.outerHTML = "<body onerror='check1(arguments, arguments.callee)'></body>"
+  window.dispatchEvent(reference_error);
+});
+
+function check2(args, callee) {
+  parent.t2.step(function() {
+    parent.assert_equals(callee.length, 5);
+    parent.assert_equals(args.length, 1);
+    parent.assert_false(args[0] instanceof ErrorEvent);
+    parent.t2.done()
+  });
+}
+
+parent.t2.step(function() {
+  document.body.outerHTML = "<body onerror='check2(arguments, arguments.callee)'></body>"
+  window.dispatchEvent(new Event("error"));
+});
+
+function check3(args, callee) {
+  parent.t3.step(function() {
+    parent.assert_equals(args.length, 1);
+    parent.assert_equals(callee.length, 1);
+  });
+}
+
+parent.t3.step(function() {
+  document.body.outerHTML = "<body><span onerror='check3(arguments, arguments.callee)'></span></body>"
+  document.body.firstChild.dispatchEvent(reference_error);
+  document.body.firstChild.dispatchEvent(new Event("error"));
+  parent.t3.done();
+});
+</script>

tests/wpt/web-platform-tests/html/webappapis/scripting/events/onerroreventhandler.html

@@ -0,0 +1,11 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>OnErrorEventHandler + ErrorEvent is treated differently</title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<script>
+var t = async_test("onerror + ErrorEvent + Window");
+var t2 = async_test("onerror + !ErrorEvent + Window");
+var t3 = async_test("onerror + Document");
+</script>
+<iframe src="onerroreventhandler-frame.html"></iframe>