From 2475dc1d21343e7cdda8b77be87be4484ee0f15a Mon Sep 17 00:00:00 2001 From: David Raifaizen Date: Mon, 25 Jul 2016 20:34:47 -0400 Subject: Removed some sources of panic from script thread and devtools, using Option values instead to indicate when a pipeline context is missing where appropriate. Additionally, removed erroneous method get_browsing_context. --- components/script/devtools.rs | 85 +++++++++++++++++++++------------ components/script/script_thread.rs | 14 ++---- components/script/webdriver_handlers.rs | 19 ++++++-- 3 files changed, 74 insertions(+), 44 deletions(-) (limited to 'components/script') diff --git a/components/script/devtools.rs b/components/script/devtools.rs index 20394bf367a..024307bc696 100644 --- a/components/script/devtools.rs +++ b/components/script/devtools.rs @@ -25,7 +25,6 @@ use ipc_channel::ipc::IpcSender; use js::jsapi::{JSAutoCompartment, ObjectClassName}; use js::jsval::UndefinedValue; use msg::constellation_msg::PipelineId; -use script_thread::get_browsing_context; use std::ffi::CStr; use std::str; use style::properties::longhands::{margin_top, margin_right, margin_bottom, margin_left}; @@ -68,58 +67,72 @@ pub fn handle_evaluate_js(global: &GlobalRef, eval: String, reply: IpcSender) { - let context = get_browsing_context(context, pipeline); +pub fn handle_get_root_node(context: &BrowsingContext, pipeline: PipelineId, reply: IpcSender>) { + let context = match context.find(pipeline) { + Some(found_context) => found_context, + None => return reply.send(None).unwrap() + }; + let document = context.active_document(); let node = document.upcast::(); - reply.send(node.summarize()).unwrap(); + reply.send(Some(node.summarize())).unwrap(); } pub fn handle_get_document_element(context: &BrowsingContext, pipeline: PipelineId, - reply: IpcSender) { - let context = get_browsing_context(context, pipeline); + reply: IpcSender>) { + let context = match context.find(pipeline) { + Some(found_context) => found_context, + None => return reply.send(None).unwrap() + }; + let document = context.active_document(); let document_element = document.GetDocumentElement().unwrap(); let node = document_element.upcast::(); - reply.send(node.summarize()).unwrap(); + reply.send(Some(node.summarize())).unwrap(); } fn find_node_by_unique_id(context: &BrowsingContext, pipeline: PipelineId, - node_id: String) - -> Root { - let context = get_browsing_context(context, pipeline); + node_id: &str) + -> Option> { + let context = match context.find(pipeline) { + Some(found_context) => found_context, + None => return None + }; + let document = context.active_document(); let node = document.upcast::(); - for candidate in node.traverse_preorder() { - if candidate.unique_id() == node_id { - return candidate; - } - } - - panic!("couldn't find node with unique id {}", node_id) + node.traverse_preorder().find(|candidate| candidate.unique_id() == node_id) } pub fn handle_get_children(context: &BrowsingContext, pipeline: PipelineId, node_id: String, - reply: IpcSender>) { - let parent = find_node_by_unique_id(context, pipeline, node_id); - let children = parent.children() - .map(|child| child.summarize()) - .collect(); - reply.send(children).unwrap(); + reply: IpcSender>>) { + match find_node_by_unique_id(context, pipeline, &*node_id) { + None => return reply.send(None).unwrap(), + Some(parent) => { + let children = parent.children() + .map(|child| child.summarize()) + .collect(); + + reply.send(Some(children)).unwrap(); + } + }; } pub fn handle_get_layout(context: &BrowsingContext, pipeline: PipelineId, node_id: String, - reply: IpcSender) { - let node = find_node_by_unique_id(context, pipeline, node_id); + reply: IpcSender>) { + let node = match find_node_by_unique_id(context, pipeline, &*node_id) { + None => return reply.send(None).unwrap(), + Some(found_node) => found_node + }; let elem = node.downcast::().expect("should be getting layout of element"); let rect = elem.GetBoundingClientRect(); @@ -130,7 +143,7 @@ pub fn handle_get_layout(context: &BrowsingContext, let elem = node.downcast::().expect("should be getting layout of element"); let computed_style = window.r().GetComputedStyle(elem, None); - reply.send(ComputedNodeLayout { + reply.send(Some(ComputedNodeLayout { display: String::from(computed_style.Display()), position: String::from(computed_style.Position()), zIndex: String::from(computed_style.ZIndex()), @@ -150,7 +163,7 @@ pub fn handle_get_layout(context: &BrowsingContext, paddingLeft: String::from(computed_style.PaddingLeft()), width: width, height: height, - }).unwrap(); + })).unwrap(); } fn determine_auto_margins(window: &Window, node: &Node) -> AutoMargins { @@ -209,7 +222,11 @@ pub fn handle_modify_attribute(context: &BrowsingContext, pipeline: PipelineId, node_id: String, modifications: Vec) { - let node = find_node_by_unique_id(context, pipeline, node_id); + let node = match find_node_by_unique_id(context, pipeline, &*node_id) { + None => return warn!("node id {} for pipeline id {} is not found", &node_id, &pipeline), + Some(found_node) => found_node + }; + let elem = node.downcast::().expect("should be getting layout of element"); for modification in modifications { @@ -243,7 +260,11 @@ pub fn handle_drop_timeline_markers(context: &BrowsingContext, pub fn handle_request_animation_frame(context: &BrowsingContext, id: PipelineId, actor_name: String) { - let context = context.find(id).expect("There is no such context"); + let context = match context.find(id) { + None => return warn!("context for pipeline id {} is not found", id), + Some(found_node) => found_node + }; + let doc = context.active_document(); let devtools_sender = context.active_window().devtools_chan().unwrap(); doc.request_animation_frame(box move |time| { @@ -254,7 +275,11 @@ pub fn handle_request_animation_frame(context: &BrowsingContext, pub fn handle_reload(context: &BrowsingContext, id: PipelineId) { - let context = context.find(id).expect("There is no such context"); + let context = match context.find(id) { + None => return warn!("context for pipeline id {} is not found", id), + Some(found_node) => found_node + }; + let win = context.active_window(); let location = win.Location(); location.Reload(); diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index aeed396fbb1..a7d9135b068 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1108,7 +1108,10 @@ impl ScriptThread { fn handle_resize(&self, id: PipelineId, size: WindowSizeData, size_type: WindowSizeType) { if let Some(ref context) = self.find_child_context(id) { - let window = context.active_window(); + let window = match context.find(id) { + Some(browsing_context) => browsing_context.active_window(), + None => return warn!("Message sent to closed pipeline {}.", id), + }; window.set_resize_event(size, size_type); return; } @@ -2204,15 +2207,6 @@ fn shut_down_layout(context_tree: &BrowsingContext) { } } -// TODO: remove this function, as it's a source of panic. -pub fn get_browsing_context(context: &BrowsingContext, - pipeline_id: PipelineId) - -> Root { - context.find(pipeline_id).expect("ScriptThread: received an event \ - message for a layout channel that is not associated with this script thread.\ - This is a bug.") -} - fn dom_last_modified(tm: &Tm) -> String { tm.to_local().strftime("%m/%d/%Y %H:%M:%S").unwrap().to_string() } diff --git a/components/script/webdriver_handlers.rs b/components/script/webdriver_handlers.rs index 2711cfb1225..74a754c36ce 100644 --- a/components/script/webdriver_handlers.rs +++ b/components/script/webdriver_handlers.rs @@ -34,7 +34,6 @@ use msg::constellation_msg::PipelineId; use net_traits::CookieSource::{HTTP, NonHTTP}; use net_traits::CoreResourceMsg::{GetCookiesDataForUrl, SetCookiesForUrlWithData}; use net_traits::IpcSend; -use script_thread::get_browsing_context; use script_traits::webdriver_msg::WebDriverCookieError; use script_traits::webdriver_msg::{WebDriverFrameId, WebDriverJSError, WebDriverJSResult, WebDriverJSValue}; use url::Url; @@ -43,7 +42,11 @@ fn find_node_by_unique_id(context: &BrowsingContext, pipeline: PipelineId, node_id: String) -> Option> { - let context = get_browsing_context(&context, pipeline); + let context = match context.find(pipeline) { + Some(context) => context, + None => return None + }; + let document = context.active_document(); document.upcast::().traverse_preorder().find(|candidate| candidate.unique_id() == node_id) } @@ -72,7 +75,11 @@ pub fn handle_execute_script(context: &BrowsingContext, pipeline: PipelineId, eval: String, reply: IpcSender) { - let context = get_browsing_context(&context, pipeline); + let context = match context.find(pipeline) { + Some(context) => context, + None => return reply.send(Err(WebDriverJSError::BrowsingContextNotFound)).unwrap() + }; + let window = context.active_window(); let result = unsafe { let cx = window.get_cx(); @@ -87,7 +94,11 @@ pub fn handle_execute_async_script(context: &BrowsingContext, pipeline: PipelineId, eval: String, reply: IpcSender) { - let context = get_browsing_context(&context, pipeline); + let context = match context.find(pipeline) { + Some(context) => context, + None => return reply.send(Err(WebDriverJSError::BrowsingContextNotFound)).unwrap() + }; + let window = context.active_window(); let cx = window.get_cx(); window.set_webdriver_script_chan(Some(reply)); -- cgit v1.2.3