Auto merge of #19329 - Manishearth:fetchcanceller, r=jdm

Add RAII guard for cancelling fetch when the consumer no longer cares about it

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

components/constellation/constellation.rs

@@ -1110,9 +1110,9 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
             FromScriptMsg::PipelineExited => {
                 self.handle_pipeline_exited(source_pipeline_id);
             }
-            FromScriptMsg::InitiateNavigateRequest(req_init) => {
+            FromScriptMsg::InitiateNavigateRequest(req_init, cancel_chan) => {
                 debug!("constellation got initiate navigate request message");
-                self.handle_navigate_request(source_pipeline_id, req_init);
+                self.handle_navigate_request(source_pipeline_id, req_init, cancel_chan);
             }
             FromScriptMsg::ScriptLoadedURLInIFrame(load_info) => {
                 debug!("constellation got iframe URL load message {:?} {:?} {:?}",
@@ -1689,14 +1689,15 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
 
     fn handle_navigate_request(&self,
                               id: PipelineId,
-                              req_init: RequestInit) {
+                              req_init: RequestInit,
+                              cancel_chan: IpcReceiver<()>) {
         let listener = NetworkListener::new(
                            req_init,
                            id,
                            self.public_resource_threads.clone(),
                            self.network_listener_sender.clone());
 
-        listener.initiate_fetch();
+        listener.initiate_fetch(Some(cancel_chan));
     }
 
     // The script thread associated with pipeline_id has loaded a URL in an iframe via script. This

components/constellation/network_listener.rs

@@ -41,7 +41,7 @@ impl NetworkListener {
         }
     }
 
-    pub fn initiate_fetch(&self) {
+    pub fn initiate_fetch(&self, cancel_chan: Option<ipc::IpcReceiver<()>>) {
         let (ipc_sender, ipc_receiver) = ipc::channel().expect("Failed to create IPC channel!");
 
         let mut listener = NetworkListener {
@@ -64,7 +64,7 @@ impl NetworkListener {
 
                 CoreResourceMsg::Fetch(
                 listener.req_init.clone(),
-                FetchChannels::ResponseMsg(ipc_sender, None))
+                FetchChannels::ResponseMsg(ipc_sender, cancel_chan))
             }
         };
 
@@ -108,7 +108,11 @@ impl NetworkListener {
                             referrer: metadata.referrer.clone(),
                         });
 
-                        self.initiate_fetch();
+                        // XXXManishearth we don't have the cancel_chan anymore and
+                        // can't use it here.
+                        //
+                        // Ideally the Fetch code would handle manual redirects on its own
+                        self.initiate_fetch(None);
                     },
                     _ => {
                         // Response should be processed by script thread.

components/script/dom/document.rs

@@ -91,6 +91,7 @@ use dom::windowproxy::WindowProxy;
 use dom_struct::dom_struct;
 use encoding_rs::{Encoding, UTF_8};
 use euclid::Point2D;
+use fetch::FetchCanceller;
 use html5ever::{LocalName, Namespace, QualName};
 use hyper::header::{Header, SetCookie};
 use hyper_serde::Serde;
@@ -360,6 +361,8 @@ pub struct Document {
     form_id_listener_map: DomRefCell<HashMap<Atom, HashSet<Dom<Element>>>>,
     interactive_time: DomRefCell<InteractiveMetrics>,
     tti_window: DomRefCell<InteractiveWindow>,
+    /// RAII canceller for Fetch
+    canceller: FetchCanceller,
 }
 
 #[derive(JSTraceable, MallocSizeOf)]
@@ -2165,7 +2168,8 @@ impl Document {
                          source: DocumentSource,
                          doc_loader: DocumentLoader,
                          referrer: Option<String>,
-                         referrer_policy: Option<ReferrerPolicy>)
+                         referrer_policy: Option<ReferrerPolicy>,
+                         canceller: FetchCanceller)
                          -> Document {
         let url = url.unwrap_or_else(|| ServoUrl::parse("about:blank").unwrap());
 
@@ -2270,6 +2274,7 @@ impl Document {
             form_id_listener_map: Default::default(),
             interactive_time: DomRefCell::new(interactive_time),
             tti_window: DomRefCell::new(InteractiveWindow::new()),
+            canceller: canceller,
         }
     }
 
@@ -2288,7 +2293,8 @@ impl Document {
                          DocumentSource::NotFromParser,
                          docloader,
                          None,
-                         None))
+                         None,
+                         Default::default()))
     }
 
     pub fn new(window: &Window,
@@ -2302,7 +2308,8 @@ impl Document {
                source: DocumentSource,
                doc_loader: DocumentLoader,
                referrer: Option<String>,
-               referrer_policy: Option<ReferrerPolicy>)
+               referrer_policy: Option<ReferrerPolicy>,
+               canceller: FetchCanceller)
                -> DomRoot<Document> {
         let document = reflect_dom_object(
             Box::new(Document::new_inherited(
@@ -2317,7 +2324,8 @@ impl Document {
                 source,
                 doc_loader,
                 referrer,
-                referrer_policy
+                referrer_policy,
+                canceller
             )),
             window,
             DocumentBinding::Wrap
@@ -2474,7 +2482,8 @@ impl Document {
                                         DocumentSource::NotFromParser,
                                         DocumentLoader::new(&self.loader()),
                                         None,
-                                        None);
+                                        None,
+                                        Default::default());
             new_doc.appropriate_template_contents_owner_document.set(Some(&new_doc));
             new_doc
         })

components/script/dom/domimplementation.rs

@@ -137,7 +137,8 @@ impl DOMImplementationMethods for DOMImplementation {
                                 DocumentSource::NotFromParser,
                                 loader,
                                 None,
-                                None);
+                                None,
+                                Default::default());
 
         {
             // Step 3.

components/script/dom/domparser.rs

@@ -70,7 +70,8 @@ impl DOMParserMethods for DOMParser {
                                              DocumentSource::FromParser,
                                              loader,
                                              None,
-                                             None);
+                                             None,
+                                             Default::default());
                 ServoParser::parse_html_document(&document, s, url);
                 document.set_ready_state(DocumentReadyState::Complete);
                 Ok(document)
@@ -88,7 +89,8 @@ impl DOMParserMethods for DOMParser {
                                              DocumentSource::NotFromParser,
                                              loader,
                                              None,
-                                             None);
+                                             None,
+                                             Default::default());
                 ServoParser::parse_xml_document(&document, s, url);
                 Ok(document)
             }

components/script/dom/node.rs

@@ -1814,7 +1814,7 @@ impl Node {
                                              is_html_doc, None,
                                              None, DocumentActivity::Inactive,
                                              DocumentSource::NotFromParser, loader,
-                                             None, None);
+                                             None, None, Default::default());
                 DomRoot::upcast::<Node>(document)
             },
             NodeTypeId::Element(..) => {

components/script/dom/servoparser/mod.rs

@@ -138,7 +138,8 @@ impl ServoParser {
                                      DocumentSource::FromParser,
                                      loader,
                                      None,
-                                     None);
+                                     None,
+                                     Default::default());
 
         // Step 2.
         document.set_quirks_mode(context_document.quirks_mode());

components/script/dom/xmldocument.rs

@@ -48,7 +48,8 @@ impl XMLDocument {
                                               source,
                                               doc_loader,
                                               None,
-                                              None),
+                                              None,
+                                              Default::default()),
         }
     }
 

components/script/dom/xmlhttprequest.rs

@@ -38,6 +38,7 @@ use dom::xmlhttprequestupload::XMLHttpRequestUpload;
 use dom_struct::dom_struct;
 use encoding_rs::{Encoding, UTF_8};
 use euclid::Length;
+use fetch::FetchCanceller;
 use html5ever::serialize;
 use html5ever::serialize::SerializeOpts;
 use hyper::header::{ContentLength, ContentType, ContentEncoding};
@@ -154,8 +155,7 @@ pub struct XMLHttpRequest {
     response_status: Cell<Result<(), ()>>,
     referrer_url: Option<ServoUrl>,
     referrer_policy: Option<ReferrerPolicy>,
-    #[ignore_malloc_size_of = "channels are hard"]
-    cancellation_chan: DomRefCell<Option<ipc::IpcSender<()>>>,
+    canceller: DomRefCell<FetchCanceller>,
 }
 
 impl XMLHttpRequest {
@@ -200,7 +200,7 @@ impl XMLHttpRequest {
             response_status: Cell::new(Ok(())),
             referrer_url: referrer_url,
             referrer_policy: referrer_policy,
-            cancellation_chan: DomRefCell::new(None),
+            canceller: DomRefCell::new(Default::default()),
         }
     }
     pub fn new(global: &GlobalScope) -> DomRoot<XMLHttpRequest> {
@@ -978,6 +978,7 @@ impl XMLHttpRequest {
                         self.sync.get());
 
                 self.cancel_timeout();
+                self.canceller.borrow_mut().ignore();
 
                 // Part of step 11, send() (processing response end of file)
                 // XXXManishearth handle errors, if any (substep 2)
@@ -994,6 +995,7 @@ impl XMLHttpRequest {
             },
             XHRProgress::Errored(_, e) => {
                 self.cancel_timeout();
+                self.canceller.borrow_mut().ignore();
 
                 self.discard_subsequent_responses();
                 self.send_flag.set(false);
@@ -1023,12 +1025,7 @@ impl XMLHttpRequest {
     }
 
     fn terminate_ongoing_fetch(&self) {
-        if let Some(ref cancel_chan) = *self.cancellation_chan.borrow() {
-            // The receiver will be destroyed if the request has already completed;
-            // so we throw away the error. Cancellation is a courtesy call,
-            // we don't actually care if the other side heard.
-            let _ = cancel_chan.send(());
-        }
+        self.canceller.borrow_mut().cancel();
         let GenerationId(prev_id) = self.generation_id.get();
         self.generation_id.set(GenerationId(prev_id + 1));
         self.response_status.set(Ok(()));
@@ -1264,7 +1261,8 @@ impl XMLHttpRequest {
                       DocumentSource::FromParser,
                       docloader,
                       None,
-                      None)
+                      None,
+                      Default::default())
     }
 
     fn filter_response_headers(&self) -> Headers {
@@ -1322,8 +1320,7 @@ impl XMLHttpRequest {
             (global.networking_task_source(), None)
         };
 
-        let (cancel_sender, cancel_receiver) = ipc::channel().unwrap();
-        *self.cancellation_chan.borrow_mut() = Some(cancel_sender);
+        let cancel_receiver = self.canceller.borrow_mut().initialize();
 
         XMLHttpRequest::initiate_async_xhr(context.clone(), task_source,
                                            global, init, cancel_receiver);

components/script/fetch.rs

@@ -38,6 +38,58 @@ struct FetchContext {
     body: Vec<u8>,
 }
 
+/// RAII fetch canceller object. By default initialized to not having a canceller
+/// in it, however you can ask it for a cancellation receiver to send to Fetch
+/// in which case it will store the sender. You can manually cancel it
+/// or let it cancel on Drop in that case.
+#[derive(Default, JSTraceable, MallocSizeOf)]
+pub struct FetchCanceller {
+    #[ignore_malloc_size_of = "channels are hard"]
+    cancel_chan: Option<ipc::IpcSender<()>>
+}
+
+impl FetchCanceller {
+    /// Create an empty FetchCanceller
+    pub fn new() -> Self {
+        Default::default()
+    }
+
+    /// Obtain an IpcReceiver to send over to Fetch, and initialize
+    /// the internal sender
+    pub fn initialize(&mut self) -> ipc::IpcReceiver<()> {
+        // cancel previous fetch
+        self.cancel();
+        let (rx, tx) = ipc::channel().unwrap();
+        self.cancel_chan = Some(rx);
+        tx
+    }
+
+    /// Cancel a fetch if it is ongoing
+    pub fn cancel(&mut self) {
+        if let Some(chan) = self.cancel_chan.take() {
+            // stop trying to make fetch happen
+            // it's not going to happen
+
+            // The receiver will be destroyed if the request has already completed;
+            // so we throw away the error. Cancellation is a courtesy call,
+            // we don't actually care if the other side heard.
+            let _ = chan.send(());
+        }
+    }
+
+    /// Use this if you don't want it to send a cancellation request
+    /// on drop (e.g. if the fetch completes)
+    pub fn ignore(&mut self) {
+        let _ = self.cancel_chan.take();
+    }
+}
+
+impl Drop for FetchCanceller {
+    fn drop(&mut self) {
+        self.cancel()
+    }
+}
+
 fn from_referrer_to_referrer_url(request: &NetTraitsRequest) -> Option<ServoUrl> {
     request.referrer.to_url().map(|url| url.clone())
 }

components/script/script_thread.rs

@@ -62,6 +62,7 @@ use dom::worker::TrustedWorkerAddress;
 use dom::worklet::WorkletThreadPool;
 use dom::workletglobalscope::WorkletGlobalScopeInit;
 use euclid::{Point2D, Vector2D, Rect};
+use fetch::FetchCanceller;
 use hyper::header::{ContentType, HttpDate, Headers, LastModified};
 use hyper::header::ReferrerPolicy as ReferrerPolicyHeader;
 use hyper::mime::{Mime, SubLevel, TopLevel};
@@ -170,6 +171,8 @@ struct InProgressLoad {
     navigation_start: u64,
     /// High res timestamp reporting the time when the browser started this load.
     navigation_start_precise: u64,
+    /// For cancelling the fetch
+    canceller: FetchCanceller,
 }
 
 impl InProgressLoad {
@@ -198,6 +201,7 @@ impl InProgressLoad {
             origin: origin,
             navigation_start: (current_time.sec * 1000 + current_time.nsec as i64 / 1000000) as u64,
             navigation_start_precise: navigation_start_precise,
+            canceller: Default::default(),
         }
     }
 }
@@ -2215,7 +2219,8 @@ impl ScriptThread {
                                      DocumentSource::FromParser,
                                      loader,
                                      referrer,
-                                     referrer_policy);
+                                     referrer_policy,
+                                     incomplete.canceller);
         document.set_ready_state(DocumentReadyState::Loading);
 
         self.documents.borrow_mut().insert(incomplete.pipeline_id, &*document);
@@ -2536,7 +2541,7 @@ impl ScriptThread {
 
     /// Instructs the constellation to fetch the document that will be loaded. Stores the InProgressLoad
     /// argument until a notification is received that the fetch is complete.
-    fn pre_page_load(&self, incomplete: InProgressLoad, load_data: LoadData) {
+    fn pre_page_load(&self, mut incomplete: InProgressLoad, load_data: LoadData) {
         let id = incomplete.pipeline_id.clone();
         let req_init = RequestInit {
             url: load_data.url.clone(),
@@ -2557,7 +2562,9 @@ impl ScriptThread {
         let context = ParserContext::new(id, load_data.url);
         self.incomplete_parser_contexts.borrow_mut().push((id, context));
 
-        self.script_sender.send((id, ScriptMsg::InitiateNavigateRequest(req_init))).unwrap();
+        let cancel_chan = incomplete.canceller.initialize();
+
+        self.script_sender.send((id, ScriptMsg::InitiateNavigateRequest(req_init, cancel_chan))).unwrap();
         self.incomplete_loads.borrow_mut().push(incomplete);
     }