Auto merge of #11791 - craftytrickster:11712/pipeline-lookup, r=asajeffrey

Pipeline lookup in webdriver

Fixes #11712

<!-- Please describe your changes on the following line: -->

Removed a method that seemed to duplicate already existing functionality, and returned BrowsingContextErrors in the web_handler file where panics were previously occurring.

I am not sure if I like all the unwrapping that occurs in the script thread, but the current methods are not set up to return Option/Result.

Also, should line the method on line 37 `find_node_by_unique_id` of components/script/webdriver_handlers.rs return None if the context is not found like I have it currently? Or should it return a `Result<Option...>` instead?

<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11712 .

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because I simply removed a method that duplicated already existing functionality. On the other part, I added better error sending instead of forcing a panic, which does not require testing to my knowledge.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/11791)
<!-- Reviewable:end -->

3 files changed, 74 insertions, 44 deletions

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<Eva
     reply.send(result).unwrap();
 }
 
-pub fn handle_get_root_node(context: &BrowsingContext, pipeline: PipelineId, reply: IpcSender<NodeInfo>) {
-    let context = get_browsing_context(context, pipeline);
+pub fn handle_get_root_node(context: &BrowsingContext, pipeline: PipelineId, reply: IpcSender<Option<NodeInfo>>) {
+    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::<Node>();
-    reply.send(node.summarize()).unwrap();
+    reply.send(Some(node.summarize())).unwrap();
 }
 
 pub fn handle_get_document_element(context: &BrowsingContext,
                                    pipeline: PipelineId,
-                                   reply: IpcSender<NodeInfo>) {
-    let context = get_browsing_context(context, pipeline);
+                                   reply: IpcSender<Option<NodeInfo>>) {
+    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::<Node>();
-    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<Node> {
-    let context = get_browsing_context(context, pipeline);
+                          node_id: &str)
+                          -> Option<Root<Node>> {
+    let context = match context.find(pipeline) {
+        Some(found_context) => found_context,
+        None => return None
+    };
+
     let document = context.active_document();
     let node = document.upcast::<Node>();
 
-    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<Vec<NodeInfo>>) {
-    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<Option<Vec<NodeInfo>>>) {
+    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<ComputedNodeLayout>) {
-    let node = find_node_by_unique_id(context, pipeline, node_id);
+                         reply: IpcSender<Option<ComputedNodeLayout>>) {
+    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::<Element>().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::<Element>().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<Modification>) {
-    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::<Element>().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 1ec367e9b75..d5d23578c7a 100644
--- a/components/script/script_thread.rs
+++ b/components/script/script_thread.rs
@@ -1073,7 +1073,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;
         }
@@ -2211,15 +2214,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<BrowsingContext> {
-    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<Root<Node>> {
-    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::<Node>().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<WebDriverJSResult>) {
-    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<WebDriverJSResult>) {
-    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));