From 35a570ab66227fb640e64bd50373f1e1a9f51b22 Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Tue, 19 May 2015 16:14:25 +1000 Subject: Fix several hangs / panics during pipeline cleanup of in progress loads. This fixes a hang found while testing the jQuery test suite. --- components/script/script_task.rs | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) (limited to 'components/script/script_task.rs') diff --git a/components/script/script_task.rs b/components/script/script_task.rs index b114819c613..e291d98e5ee 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -386,7 +386,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, script_port, NonWorkerScriptChan(script_chan), @@ -1080,11 +1080,30 @@ 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. + // 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 { // TODO(gw): This probably leaks resources! - return true; + 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 root 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 -- cgit v1.2.3 From decfb0da6edec9857c304932f3c6fa5ca23b8f07 Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Wed, 20 May 2015 07:10:55 +1000 Subject: Address review comments --- components/script/script_task.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'components/script/script_task.rs') diff --git a/components/script/script_task.rs b/components/script/script_task.rs index e291d98e5ee..fcc0615213a 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -1086,7 +1086,6 @@ impl ScriptTask { }); if let Some(idx) = idx { - // TODO(gw): This probably leaks resources! let load = self.incomplete_loads.borrow_mut().remove(idx); // Tell the layout task to begin shutting down, and wait until it @@ -1094,7 +1093,7 @@ impl ScriptTask { 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 root page {:?}", id); + debug!("shutting down layout for page {:?}", id); response_port.recv().unwrap(); chan.send(layout_interface::Msg::ExitNow(exit_type)).ok(); } -- cgit v1.2.3 From 23b18a841714a4dc8931fda0f73014699d2cbbbe Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Wed, 20 May 2015 07:55:22 +1000 Subject: Handle case where a page fetch completes after pipeline exits. --- components/script/script_task.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'components/script/script_task.rs') diff --git a/components/script/script_task.rs b/components/script/script_task.rs index fcc0615213a..0a9d97b4540 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -1062,12 +1062,15 @@ impl ScriptTask { /// Kick off the document and frame tree creation process using the result. fn handle_page_fetch_complete(&self, id: PipelineId, subpage: Option, 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. + if let Some(idx) = idx { + let load = self.incomplete_loads.borrow_mut().remove(idx); + self.load(response, load); + } } /// Handles a request for the window title. -- cgit v1.2.3 From 41c243e853a1a19bac512fd26b6c9bae3402c4df Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Wed, 20 May 2015 09:22:08 +1000 Subject: Add closed pipelines record as a debugging aid --- components/script/script_task.rs | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) (limited to 'components/script/script_task.rs') diff --git a/components/script/script_task.rs b/components/script/script_task.rs index 0a9d97b4540..014e32f2f97 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -317,6 +317,9 @@ pub struct ScriptTask { js_runtime: Rc, mouse_over_targets: DOMRefCell>>, + + /// List of pipelines that have been owned and closed by this script task. + closed_pipelines: RefCell>, } /// In the event of task failure, all data on the stack runs its destructor. However, there @@ -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 @@ -1067,9 +1075,14 @@ impl ScriptTask { }); // The matching in progress load structure may not exist if // the pipeline exited before the page load completed. - if let Some(idx) = idx { - let load = self.incomplete_loads.borrow_mut().remove(idx); - self.load(response, load); + 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)); + } } } @@ -1083,6 +1096,8 @@ 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 { + 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 -- cgit v1.2.3