Auto merge of #12003 - Manishearth:no-crash-url-parse, r=jdm

Don't crash when <img> fails to parse its src

Fixes #11992

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12003)
<!-- Reviewable:end -->

components/script/dom/htmlimageelement.rs

@@ -31,6 +31,8 @@ use script_thread::Runnable;
 use std::sync::Arc;
 use string_cache::Atom;
 use style::attr::{AttrValue, LengthOrPercentageOrAuto};
+use task_source::TaskSource;
+use task_source::dom_manipulation::DOMManipulationTask;
 use url::Url;
 
 #[derive(JSTraceable, HeapSizeOf)]
@@ -44,7 +46,8 @@ enum State {
 #[derive(JSTraceable, HeapSizeOf)]
 struct ImageRequest {
     state: State,
-    url: Option<Url>,
+    parsed_url: Option<Url>,
+    source_url: Option<DOMString>,
     image: Option<Arc<Image>>,
     metadata: Option<ImageMetadata>,
 }
@@ -56,8 +59,8 @@ pub struct HTMLImageElement {
 }
 
 impl HTMLImageElement {
-    pub fn get_url(&self) -> Option<Url>{
+    pub fn get_url(&self) -> Option<Url> {
-        self.current_request.borrow().url.clone()
+        self.current_request.borrow().parsed_url.clone()
     }
 }
 
@@ -118,33 +121,70 @@ impl HTMLImageElement {
         let image_cache = window.image_cache_thread();
         match value {
             None => {
-                self.current_request.borrow_mut().url = None;
+                self.current_request.borrow_mut().parsed_url = None;
+                self.current_request.borrow_mut().source_url = None;
                 self.current_request.borrow_mut().image = None;
             }
             Some((src, base_url)) => {
                 let img_url = base_url.join(&src);
-                // FIXME: handle URL parse errors more gracefully.
+                if let Ok(img_url) = img_url {
-                let img_url = img_url.unwrap();
+                    self.current_request.borrow_mut().parsed_url = Some(img_url.clone());
-                self.current_request.borrow_mut().url = Some(img_url.clone());
+                    self.current_request.borrow_mut().source_url = Some(src);
 
-                let trusted_node = Trusted::new(self);
+                    let trusted_node = Trusted::new(self);
-                let (responder_sender, responder_receiver) = ipc::channel().unwrap();
+                    let (responder_sender, responder_receiver) = ipc::channel().unwrap();
-                let script_chan = window.networking_task_source();
+                    let script_chan = window.networking_task_source();
-                let wrapper = window.get_runnable_wrapper();
+                    let wrapper = window.get_runnable_wrapper();
-                ROUTER.add_route(responder_receiver.to_opaque(), box move |message| {
+                    ROUTER.add_route(responder_receiver.to_opaque(), box move |message| {
-                    // Return the image via a message to the script thread, which marks the element
+                        // Return the image via a message to the script thread, which marks the element
-                    // as dirty and triggers a reflow.
+                        // as dirty and triggers a reflow.
-                    let image_response = message.to().unwrap();
+                        let image_response = message.to().unwrap();
-                    let runnable = ImageResponseHandlerRunnable::new(
+                        let runnable = ImageResponseHandlerRunnable::new(
-                        trusted_node.clone(), image_response);
+                            trusted_node.clone(), image_response);
-                    let runnable = wrapper.wrap_runnable(runnable);
+                        let runnable = wrapper.wrap_runnable(runnable);
-                    let _ = script_chan.send(CommonScriptMsg::RunnableMsg(
+                        let _ = script_chan.send(CommonScriptMsg::RunnableMsg(
-                        UpdateReplacedElement, runnable));
+                            UpdateReplacedElement, runnable));
-                });
+                    });
 
-                image_cache.request_image_and_metadata(img_url,
+                    image_cache.request_image_and_metadata(img_url,
-                                          window.image_cache_chan(),
+                                              window.image_cache_chan(),
-                                          Some(ImageResponder::new(responder_sender)));
+                                              Some(ImageResponder::new(responder_sender)));
+                } else {
+                    // https://html.spec.whatwg.org/multipage/#update-the-image-data
+                    // Step 11 (error substeps)
+                    debug!("Failed to parse URL {} with base {}", src, base_url);
+                    let mut req = self.current_request.borrow_mut();
+
+                    // Substeps 1,2
+                    req.image = None;
+                    req.parsed_url = None;
+                    req.state = State::Broken;
+                    // todo: set pending request to null
+                    // (pending requests aren't being used yet)
+
+
+                    struct ImgParseErrorRunnable {
+                        img: Trusted<HTMLImageElement>,
+                        src: String,
+                    }
+                    impl Runnable for ImgParseErrorRunnable {
+                        fn handler(self: Box<Self>) {
+                            // https://html.spec.whatwg.org/multipage/#update-the-image-data
+                            // Step 11, substep 5
+                            let img = self.img.root();
+                            img.current_request.borrow_mut().source_url = Some(self.src.into());
+                            img.upcast::<EventTarget>().fire_simple_event("error");
+                            img.upcast::<EventTarget>().fire_simple_event("loadend");
+                        }
+                    }
+
+                    let runnable = Box::new(ImgParseErrorRunnable {
+                        img: Trusted::new(self),
+                        src: src.into(),
+                    });
+                    let task = window.dom_manipulation_task_source();
+                    let _ = task.queue(DOMManipulationTask::Miscellaneous(runnable));
+                }
             }
         }
     }
@@ -153,13 +193,15 @@ impl HTMLImageElement {
             htmlelement: HTMLElement::new_inherited(localName, prefix, document),
             current_request: DOMRefCell::new(ImageRequest {
                 state: State::Unavailable,
-                url: None,
+                parsed_url: None,
+                source_url: None,
                 image: None,
                 metadata: None
             }),
             pending_request: DOMRefCell::new(ImageRequest {
                 state: State::Unavailable,
-                url: None,
+                parsed_url: None,
+                source_url: None,
                 image: None,
                 metadata: None
             }),
@@ -209,7 +251,7 @@ impl LayoutHTMLImageElementHelpers for LayoutJS<HTMLImageElement> {
 
     #[allow(unsafe_code)]
     unsafe fn image_url(&self) -> Option<Url> {
-        (*self.unsafe_get()).current_request.borrow_for_layout().url.clone()
+        (*self.unsafe_get()).current_request.borrow_for_layout().parsed_url.clone()
     }
 
     #[allow(unsafe_code)]
@@ -313,9 +355,9 @@ impl HTMLImageElementMethods for HTMLImageElement {
 
     // https://html.spec.whatwg.org/multipage/#dom-img-currentsrc
     fn CurrentSrc(&self) -> DOMString {
-        let ref url = self.current_request.borrow().url;
+        let ref url = self.current_request.borrow().source_url;
         match *url {
-            Some(ref url) => DOMString::from(url.as_str()),
+            Some(ref url) => url.clone(),
             None => DOMString::from(""),
         }
     }

components/script/task_source/dom_manipulation.rs

@@ -52,7 +52,8 @@ pub enum DOMManipulationTask {
     // https://html.spec.whatwg.org/multipage/#planned-navigation
     PlannedNavigation(Box<Runnable + Send>),
     // https://html.spec.whatwg.org/multipage/#send-a-storage-notification
-    SendStorageNotification(Box<MainThreadRunnable + Send>)
+    SendStorageNotification(Box<MainThreadRunnable + Send>),
+    Miscellaneous(Box<Runnable + Send>),
 }
 
 impl DOMManipulationTask {
@@ -72,7 +73,8 @@ impl DOMManipulationTask {
             FireToggleEvent(runnable) => runnable.handler(),
             MediaTask(runnable) => runnable.handler(),
             PlannedNavigation(runnable) => runnable.handler(),
-            SendStorageNotification(runnable) => runnable.handler(script_thread)
+            SendStorageNotification(runnable) => runnable.handler(script_thread),
+            Miscellaneous(runnable) => runnable.handler(),
         }
     }
 }

tests/wpt/metadata/MANIFEST.json

@@ -36276,6 +36276,12 @@
             "path": "html/semantics/document-metadata/the-link-element/document-without-browsing-context.html",
             "url": "/html/semantics/document-metadata/the-link-element/document-without-browsing-context.html"
           }
+        ],
+        "html/semantics/embedded-content/the-img-element/invalid-src.html": [
+          {
+            "path": "html/semantics/embedded-content/the-img-element/invalid-src.html",
+            "url": "/html/semantics/embedded-content/the-img-element/invalid-src.html"
+          }
         ]
       }
     },

tests/wpt/metadata/html/semantics/embedded-content/the-img-element/update-the-image-data/fail-to-resolve.html.ini

@@ -1,3 +1,11 @@
 [fail-to-resolve.html]
   type: testharness
-  expected: CRASH
+  [<img srcset="//[">]
+    expected: FAIL
+
+  [<img srcset="//[" src="/images/red.png">]
+    expected: FAIL
+
+  [<img srcset="//[, /images/red.png">]
+    expected: FAIL
+

tests/wpt/metadata/html/semantics/embedded-content/the-img-element/update-the-source-set.html.ini

@@ -1,8 +1,5 @@
 [update-the-source-set.html]
   type: testharness
-  [<img src="" data-expect="">]
-    expected: FAIL
-
   [<img srcset="data:,b" src="data:,a" data-expect="data:,b">]
     expected: FAIL
 

tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/invalid-src.html

@@ -0,0 +1,21 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Loading a non-parsing URL as an image should silently fail; triggering appropriate events</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<img id=myimg />
+<script>
+async_test(function(t) {
+    var img = document.getElementById("myimg");
+    img.src = "http://also a broken url";
+    var errorevent = false;
+
+    // The errors should be queued in the event loop, so they should only trigger
+    // after this block of code finishes, not during the img.src setter itself
+    img.addEventListener('error', t.step_func(function(){errorevent = true;}));
+    img.addEventListener('loadend', t.step_func_done(function() {
+        assert_true(errorevent, "error event fired");
+    }));
+});
+
+</script>