Auto merge of #6131 - glennw:jquery-exit-fix, r=jdm

This fixes a hang found while testing the jQuery test suite.

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

7 files changed, 74 insertions, 21 deletions

diff --git a/components/compositing/compositor_task.rs b/components/compositing/compositor_task.rs
index 5a3c6bdd5fa..10a49fa3928 100644
--- a/components/compositing/compositor_task.rs
+++ b/components/compositing/compositor_task.rs
@@ -94,7 +94,13 @@ impl PaintListener for Box<CompositorProxy+'static+Send> {
     fn get_graphics_metadata(&mut self) -> Option<NativeGraphicsMetadata> {
         let (chan, port) = channel();
         self.send(Msg::GetGraphicsMetadata(chan));
-        port.recv().unwrap()
+        // If the compositor is shutting down when a paint task
+        // is being created, the compositor won't respond to
+        // this message, resulting in an eventual panic. Instead,
+        // just return None in this case, since the paint task
+        // will exit shortly and never actually be requested
+        // to paint buffers by the compositor.
+        port.recv().unwrap_or(None)
     }
 
     fn assign_painted_buffers(&mut self,
diff --git a/components/gfx/paint_task.rs b/components/gfx/paint_task.rs
index 62a368b18d9..04570f14c80 100644
--- a/components/gfx/paint_task.rs
+++ b/components/gfx/paint_task.rs
@@ -145,7 +145,7 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                   time_profiler_chan: time::ProfilerChan,
                   shutdown_chan: Sender<()>) {
         let ConstellationChan(c) = constellation_chan.clone();
-        spawn_named_with_send_on_failure("PaintTask", task_state::PAINT, move || {
+        spawn_named_with_send_on_failure(format!("PaintTask {:?}", id), task_state::PAINT, move || {
             {
                 // Ensures that the paint task and graphics context are destroyed before the
                 // shutdown message.
diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs
index 0bf583627d1..702c28f887d 100644
--- a/components/layout/layout_task.rs
+++ b/components/layout/layout_task.rs
@@ -220,7 +220,7 @@ impl LayoutTaskFactory for LayoutTask {
               memory_profiler_chan: mem::ProfilerChan,
               shutdown_chan: Sender<()>) {
         let ConstellationChan(con_chan) = constellation_chan.clone();
-        spawn_named_with_send_on_failure("LayoutTask", task_state::LAYOUT, move || {
+        spawn_named_with_send_on_failure(format!("LayoutTask {:?}", id), task_state::LAYOUT, move || {
             { // Ensures layout task is destroyed before we send shutdown message
                 let sender = chan.sender();
                 let layout = LayoutTask::new(id,
diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs
index cba1b2c6d86..09bf676405b 100644
--- a/components/msg/constellation_msg.rs
+++ b/components/msg/constellation_msg.rs
@@ -366,7 +366,7 @@ pub struct SubpageId(pub u32);
 
 // The type of pipeline exit. During complete shutdowns, pipelines do not have to
 // release resources automatically released on process termination.
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug)]
 pub enum PipelineExitType {
     PipelineOnly,
     Complete,
diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs
index 2621a9498a3..f791464366d 100644
--- a/components/script/dom/window.rs
+++ b/components/script/dom/window.rs
@@ -584,6 +584,17 @@ impl<'a> WindowHelpers for JSRef<'a, Window> {
         let document = self.Document().root();
         NodeCast::from_ref(document.r()).teardown();
 
+        // The above code may not catch all DOM objects
+        // (e.g. DOM objects removed from the tree that haven't
+        // been collected yet). Forcing a GC here means that
+        // those DOM objects will be able to call dispose()
+        // to free their layout data before the layout task
+        // exits. Without this, those remaining objects try to
+        // send a message to free their layout data to the
+        // layout task when the script task is dropped,
+        // which causes a panic!
+        self.Gc();
+
         *self.js_runtime.borrow_mut() = None;
         *self.browser_context.borrow_mut() = None;
     }
diff --git a/components/script/script_task.rs b/components/script/script_task.rs
index b114819c613..014e32f2f97 100644
--- a/components/script/script_task.rs
+++ b/components/script/script_task.rs
@@ -317,6 +317,9 @@ pub struct ScriptTask {
     js_runtime: Rc<Runtime>,
 
     mouse_over_targets: DOMRefCell<Vec<JS<Node>>>,
+
+    /// List of pipelines that have been owned and closed by this script task.
+    closed_pipelines: RefCell<HashSet<PipelineId>>,
 }
 
 /// In the event of task failure, all data on the stack runs its destructor. However, there
@@ -386,7 +389,7 @@ impl ScriptTaskFactory for ScriptTask {
         let ConstellationChan(const_chan) = constellation_chan.clone();
         let (script_chan, script_port) = channel();
         let layout_chan = LayoutChan(layout_chan.sender());
-        spawn_named_with_send_on_failure("ScriptTask", task_state::SCRIPT, move || {
+        spawn_named_with_send_on_failure(format!("ScriptTask {:?}", id), task_state::SCRIPT, move || {
             let script_task = ScriptTask::new(box compositor as Box<ScriptListener>,
                                               script_port,
                                               NonWorkerScriptChan(script_chan),
@@ -494,7 +497,8 @@ impl ScriptTask {
             devtools_marker_sender: RefCell::new(None),
 
             js_runtime: Rc::new(runtime),
-            mouse_over_targets: DOMRefCell::new(vec!())
+            mouse_over_targets: DOMRefCell::new(vec!()),
+            closed_pipelines: RefCell::new(HashSet::new()),
         }
     }
 
@@ -1027,11 +1031,15 @@ impl ScriptTask {
     fn handle_reflow_complete_msg(&self, pipeline_id: PipelineId, reflow_id: u32) {
         debug!("Script: Reflow {:?} complete for {:?}", reflow_id, pipeline_id);
         let page = self.root_page();
-        let page = page.find(pipeline_id).expect(
-            "ScriptTask: received a load message for a layout channel that is not associated \
-             with this script task. This is a bug.");
-        let window = page.window().root();
-        window.r().handle_reflow_complete_msg(reflow_id);
+        match page.find(pipeline_id) {
+            Some(page) => {
+                let window = page.window().root();
+                window.r().handle_reflow_complete_msg(reflow_id);
+            }
+            None => {
+                assert!(self.closed_pipelines.borrow().contains(&pipeline_id));
+            }
+        }
     }
 
     /// Window was resized, but this script was not active, so don't reflow yet
@@ -1062,12 +1070,20 @@ impl ScriptTask {
     /// Kick off the document and frame tree creation process using the result.
     fn handle_page_fetch_complete(&self, id: PipelineId, subpage: Option<SubpageId>,
                                   response: LoadResponse) {
-        // Any notification received should refer to an existing, in-progress load that is tracked.
         let idx = self.incomplete_loads.borrow().iter().position(|load| {
             load.pipeline_id == id && load.parent_info.map(|info| info.1) == subpage
-        }).unwrap();
-        let load = self.incomplete_loads.borrow_mut().remove(idx);
-        self.load(response, load);
+        });
+        // The matching in progress load structure may not exist if
+        // the pipeline exited before the page load completed.
+        match idx {
+            Some(idx) => {
+                let load = self.incomplete_loads.borrow_mut().remove(idx);
+                self.load(response, load);
+            }
+            None => {
+                assert!(self.closed_pipelines.borrow().contains(&id));
+            }
+        }
     }
 
     /// Handles a request for the window title.
@@ -1080,11 +1096,31 @@ impl ScriptTask {
     /// Handles a request to exit the script task and shut down layout.
     /// Returns true if the script task should shut down and false otherwise.
     fn handle_exit_pipeline_msg(&self, id: PipelineId, exit_type: PipelineExitType) -> bool {
-        if self.page.borrow().is_none() {
-            // The root page doesn't even exist yet, so it
-            // is safe to exit this script task.
-            // TODO(gw): This probably leaks resources!
-            return true;
+        self.closed_pipelines.borrow_mut().insert(id);
+
+        // Check if the exit message is for an in progress load.
+        let idx = self.incomplete_loads.borrow().iter().position(|load| {
+            load.pipeline_id == id
+        });
+
+        if let Some(idx) = idx {
+            let load = self.incomplete_loads.borrow_mut().remove(idx);
+
+            // Tell the layout task to begin shutting down, and wait until it
+            // processed this message.
+            let (response_chan, response_port) = channel();
+            let LayoutChan(chan) = load.layout_chan;
+            if chan.send(layout_interface::Msg::PrepareToExit(response_chan)).is_ok() {
+                debug!("shutting down layout for page {:?}", id);
+                response_port.recv().unwrap();
+                chan.send(layout_interface::Msg::ExitNow(exit_type)).ok();
+            }
+
+            let has_pending_loads = self.incomplete_loads.borrow().len() > 0;
+            let has_root_page = self.page.borrow().is_some();
+
+            // Exit if no pending loads and no root page
+            return !has_pending_loads && !has_root_page;
         }
 
         // If root is being exited, shut down all pages
diff --git a/components/util/task.rs b/components/util/task.rs
index de206526f44..d00f29c1c67 100644
--- a/components/util/task.rs
+++ b/components/util/task.rs
@@ -18,7 +18,7 @@ pub fn spawn_named<F>(name: String, f: F)
 }
 
 /// Arrange to send a particular message to a channel if the task fails.
-pub fn spawn_named_with_send_on_failure<F, T>(name: &'static str,
+pub fn spawn_named_with_send_on_failure<F, T>(name: String,
                                               state: task_state::TaskState,
                                               f: F,
                                               msg: T,