servo: Merge #13728 - Setting a devtools timeline marker may fail, due to pipeline lookup failure (from asajeffrey:devtools-set-timeline-marker-may-fail); r=fitzgen
authorAlan Jeffrey <ajeffrey@mozilla.com>
Thu, 13 Oct 2016 03:21:47 -0500
changeset 386550 3270b6fe6ad91f5edfc1d4d2691a0a5e95094238
parent 386549 085c36cd4da4e86ad9c3bfe66c141f805216baf9
child 386551 39c3fc1dcd0f25d3b13cafa7f8a5870f70a3596c
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen
servo: Merge #13728 - Setting a devtools timeline marker may fail, due to pipeline lookup failure (from asajeffrey:devtools-set-timeline-marker-may-fail); r=fitzgen <!-- Please describe your changes on the following line: --> Allow setting a devtools timeline marker to fail, due to pipeline lookup failure. This is part of tidying up pipeline lookup. cc @jdm --- <!-- 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 do not require tests because I'm not sure how to test devtools. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: bb75e2e727eae64477c128ace7bd5cff35e0ea81
servo/components/devtools/actors/timeline.rs
servo/components/devtools_traits/lib.rs
servo/components/script/devtools.rs
servo/components/script/dom/window.rs
servo/components/script/script_thread.rs
--- a/servo/components/devtools/actors/timeline.rs
+++ b/servo/components/devtools/actors/timeline.rs
@@ -138,31 +138,31 @@ impl TimelineActor {
             is_recording: Arc::new(Mutex::new(false)),
             stream: RefCell::new(None),
 
             framerate_actor: RefCell::new(None),
             memory_actor: RefCell::new(None),
         }
     }
 
-    fn pull_timeline_data(&self, receiver: IpcReceiver<TimelineMarker>, mut emitter: Emitter) {
+    fn pull_timeline_data(&self, receiver: IpcReceiver<Option<TimelineMarker>>, mut emitter: Emitter) {
         let is_recording = self.is_recording.clone();
 
         if !*is_recording.lock().unwrap() {
             return;
         }
 
         spawn_named("PullTimelineMarkers".to_owned(), move || {
             loop {
                 if !*is_recording.lock().unwrap() {
                     break;
                 }
 
                 let mut markers = vec![];
-                while let Ok(marker) = receiver.try_recv() {
+                while let Ok(Some(marker)) = receiver.try_recv() {
                     markers.push(emitter.marker(marker));
                 }
                 emitter.send(markers);
 
                 thread::sleep(Duration::from_millis(DEFAULT_TIMELINE_DATA_PULL_TIMEOUT));
             }
         });
     }
@@ -177,17 +177,17 @@ impl Actor for TimelineActor {
                       registry: &ActorRegistry,
                       msg_type: &str,
                       msg: &BTreeMap<String, Value>,
                       stream: &mut TcpStream) -> Result<ActorMessageStatus, ()> {
         Ok(match msg_type {
             "start" => {
                 **self.is_recording.lock().as_mut().unwrap() = true;
 
-                let (tx, rx) = ipc::channel::<TimelineMarker>().unwrap();
+                let (tx, rx) = ipc::channel::<Option<TimelineMarker>>().unwrap();
                 self.script_sender.send(SetTimelineMarkers(self.pipeline,
                                                            self.marker_types.clone(),
                                                            tx)).unwrap();
 
                 *self.stream.borrow_mut() = stream.try_clone().ok();
 
                 // init memory actor
                 if let Some(with_memory) = msg.get("withMemory") {
--- a/servo/components/devtools_traits/lib.rs
+++ b/servo/components/devtools_traits/lib.rs
@@ -185,16 +185,17 @@ pub struct ComputedNodeLayout {
 pub struct AutoMargins {
     pub top: bool,
     pub right: bool,
     pub bottom: bool,
     pub left: bool,
 }
 
 /// Messages to process in a particular script thread, as instructed by a devtools client.
+/// TODO: better error handling, e.g. if pipeline id lookup fails?
 #[derive(Deserialize, Serialize)]
 pub enum DevtoolScriptControlMsg {
     /// Evaluate a JS snippet in the context of the global for the given pipeline.
     EvaluateJS(PipelineId, String, IpcSender<EvaluateJSReply>),
     /// Retrieve the details of the root node (ie. the document) for the given pipeline.
     GetRootNode(PipelineId, IpcSender<Option<NodeInfo>>),
     /// Retrieve the details of the document element for the given pipeline.
     GetDocumentElement(PipelineId, IpcSender<Option<NodeInfo>>),
@@ -204,17 +205,17 @@ pub enum DevtoolScriptControlMsg {
     GetLayout(PipelineId, String, IpcSender<Option<ComputedNodeLayout>>),
     /// Retrieve all stored console messages for the given pipeline.
     GetCachedMessages(PipelineId, CachedConsoleMessageTypes, IpcSender<Vec<CachedConsoleMessage>>),
     /// Update a given node's attributes with a list of modifications.
     ModifyAttribute(PipelineId, String, Vec<Modification>),
     /// Request live console messages for a given pipeline (true if desired, false otherwise).
     WantsLiveNotifications(PipelineId, bool),
     /// Request live notifications for a given set of timeline events for a given pipeline.
-    SetTimelineMarkers(PipelineId, Vec<TimelineMarkerType>, IpcSender<TimelineMarker>),
+    SetTimelineMarkers(PipelineId, Vec<TimelineMarkerType>, IpcSender<Option<TimelineMarker>>),
     /// Withdraw request for live timeline notifications for a given pipeline.
     DropTimelineMarkers(PipelineId, Vec<TimelineMarkerType>),
     /// Request a callback directed at the given actor name from the next animation frame
     /// executed in the given pipeline.
     RequestAnimationFrame(PipelineId, String),
     /// Direct the given pipeline to reload the current page.
     Reload(PipelineId),
 }
--- a/servo/components/script/devtools.rs
+++ b/servo/components/script/devtools.rs
@@ -245,26 +245,31 @@ pub fn handle_modify_attribute(context: 
     }
 }
 
 pub fn handle_wants_live_notifications(global: &GlobalScope, send_notifications: bool) {
     global.set_devtools_wants_updates(send_notifications);
 }
 
 pub fn handle_set_timeline_markers(context: &BrowsingContext,
+                                   pipeline: PipelineId,
                                    marker_types: Vec<TimelineMarkerType>,
-                                   reply: IpcSender<TimelineMarker>) {
-    let window = context.active_window();
-    window.set_devtools_timeline_markers(marker_types, reply);
+                                   reply: IpcSender<Option<TimelineMarker>>) {
+    match context.find(pipeline) {
+        None => reply.send(None).unwrap(),
+        Some(context) => context.active_window().set_devtools_timeline_markers(marker_types, reply),
+    }
 }
 
 pub fn handle_drop_timeline_markers(context: &BrowsingContext,
+                                    pipeline: PipelineId,
                                     marker_types: Vec<TimelineMarkerType>) {
-    let window = context.active_window();
-    window.drop_devtools_timeline_markers(marker_types);
+    if let Some(context) = context.find(pipeline) {
+        context.active_window().drop_devtools_timeline_markers(marker_types);
+    }
 }
 
 pub fn handle_request_animation_frame(context: &BrowsingContext,
                                       id: PipelineId,
                                       actor_name: String) {
     let context = match context.find(id) {
         None => return warn!("context for pipeline id {} is not found", id),
         Some(found_node) => found_node
--- a/servo/components/script/dom/window.rs
+++ b/servo/components/script/dom/window.rs
@@ -167,17 +167,17 @@ pub struct Window {
     session_storage: MutNullableHeap<JS<Storage>>,
     local_storage: MutNullableHeap<JS<Storage>>,
     status: DOMRefCell<DOMString>,
 
     /// For sending timeline markers. Will be ignored if
     /// no devtools server
     devtools_markers: DOMRefCell<HashSet<TimelineMarkerType>>,
     #[ignore_heap_size_of = "channels are hard"]
-    devtools_marker_sender: DOMRefCell<Option<IpcSender<TimelineMarker>>>,
+    devtools_marker_sender: DOMRefCell<Option<IpcSender<Option<TimelineMarker>>>>,
 
     /// Pending resize event, if any.
     resize_event: Cell<Option<(WindowSizeData, WindowSizeType)>>,
 
     /// Parent id associated with this page, if any.
     parent_info: Option<(PipelineId, FrameType)>,
 
     /// Global static data related to the DOM.
@@ -1430,22 +1430,22 @@ impl Window {
     pub fn need_emit_timeline_marker(&self, timeline_type: TimelineMarkerType) -> bool {
         let markers = self.devtools_markers.borrow();
         markers.contains(&timeline_type)
     }
 
     pub fn emit_timeline_marker(&self, marker: TimelineMarker) {
         let sender = self.devtools_marker_sender.borrow();
         let sender = sender.as_ref().expect("There is no marker sender");
-        sender.send(marker).unwrap();
+        sender.send(Some(marker)).unwrap();
     }
 
     pub fn set_devtools_timeline_markers(&self,
                                          markers: Vec<TimelineMarkerType>,
-                                         reply: IpcSender<TimelineMarker>) {
+                                         reply: IpcSender<Option<TimelineMarker>>) {
         *self.devtools_marker_sender.borrow_mut() = Some(reply);
         self.devtools_markers.borrow_mut().extend(markers.into_iter());
     }
 
     pub fn drop_devtools_timeline_markers(&self, markers: Vec<TimelineMarkerType>) {
         let mut devtools_markers = self.devtools_markers.borrow_mut();
         for marker in markers {
             devtools_markers.remove(&marker);
--- a/servo/components/script/script_thread.rs
+++ b/servo/components/script/script_thread.rs
@@ -999,35 +999,35 @@ impl ScriptThread {
             DevtoolScriptControlMsg::GetRootNode(id, reply) =>
                 devtools::handle_get_root_node(&context, id, reply),
             DevtoolScriptControlMsg::GetDocumentElement(id, reply) =>
                 devtools::handle_get_document_element(&context, id, reply),
             DevtoolScriptControlMsg::GetChildren(id, node_id, reply) =>
                 devtools::handle_get_children(&context, id, node_id, reply),
             DevtoolScriptControlMsg::GetLayout(id, node_id, reply) =>
                 devtools::handle_get_layout(&context, id, node_id, reply),
-            DevtoolScriptControlMsg::GetCachedMessages(pipeline_id, message_types, reply) =>
-                devtools::handle_get_cached_messages(pipeline_id, message_types, reply),
+            DevtoolScriptControlMsg::GetCachedMessages(id, message_types, reply) =>
+                devtools::handle_get_cached_messages(id, message_types, reply),
             DevtoolScriptControlMsg::ModifyAttribute(id, node_id, modifications) =>
                 devtools::handle_modify_attribute(&context, id, node_id, modifications),
             DevtoolScriptControlMsg::WantsLiveNotifications(id, to_send) => {
                 let window = match context.find(id) {
                     Some(browsing_context) => browsing_context.active_window(),
                     None => return warn!("Message sent to closed pipeline {}.", id),
                 };
                 devtools::handle_wants_live_notifications(window.upcast(), to_send)
             },
-            DevtoolScriptControlMsg::SetTimelineMarkers(_pipeline_id, marker_types, reply) =>
-                devtools::handle_set_timeline_markers(&context, marker_types, reply),
-            DevtoolScriptControlMsg::DropTimelineMarkers(_pipeline_id, marker_types) =>
-                devtools::handle_drop_timeline_markers(&context, marker_types),
-            DevtoolScriptControlMsg::RequestAnimationFrame(pipeline_id, name) =>
-                devtools::handle_request_animation_frame(&context, pipeline_id, name),
-            DevtoolScriptControlMsg::Reload(pipeline_id) =>
-                devtools::handle_reload(&context, pipeline_id),
+            DevtoolScriptControlMsg::SetTimelineMarkers(id, marker_types, reply) =>
+                devtools::handle_set_timeline_markers(&context, id, marker_types, reply),
+            DevtoolScriptControlMsg::DropTimelineMarkers(id, marker_types) =>
+                devtools::handle_drop_timeline_markers(&context, id, marker_types),
+            DevtoolScriptControlMsg::RequestAnimationFrame(id, name) =>
+                devtools::handle_request_animation_frame(&context, id, name),
+            DevtoolScriptControlMsg::Reload(id) =>
+                devtools::handle_reload(&context, id),
         }
     }
 
     fn handle_msg_from_image_cache(&self, msg: ImageCacheResult) {
         msg.responder.unwrap().respond(msg.image_response);
     }
 
     fn handle_webdriver_msg(&self, pipeline_id: PipelineId, msg: WebDriverScriptCommand) {