Auto merge of #16295 - jdm:transition-safety, r=nox

Root nodes for the duration of their CSS transitions

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14972
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)
<!-- Reviewable:end -->

components/layout/animation.rs

@@ -9,7 +9,9 @@ use flow::{self, Flow};
 use gfx::display_list::OpaqueNode;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::PipelineId;
+use opaque_node::OpaqueNodeMethods;
 use script_traits::{AnimationState, ConstellationControlMsg, LayoutMsg as ConstellationMsg};
+use script_traits::UntrustedNodeAddress;
 use std::collections::HashMap;
 use std::sync::mpsc::Receiver;
 use style::animation::{Animation, update_style_for_animation};
@@ -24,6 +26,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender<ConstellationMsg>,
                               script_chan: &IpcSender<ConstellationControlMsg>,
                               running_animations: &mut HashMap<OpaqueNode, Vec<Animation>>,
                               expired_animations: &mut HashMap<OpaqueNode, Vec<Animation>>,
+                              mut newly_transitioning_nodes: Option<&mut Vec<UntrustedNodeAddress>>,
                               new_animations_receiver: &Receiver<Animation>,
                               pipeline_id: PipelineId,
                               timer: &Timer) {
@@ -71,7 +74,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender<ConstellationMsg>,
         let mut animations_still_running = vec![];
         for mut running_animation in running_animations.drain(..) {
             let still_running = !running_animation.is_expired() && match running_animation {
-                Animation::Transition(_, _, started_at, ref frame, _expired) => {
+                Animation::Transition(_, started_at, ref frame, _expired) => {
                     now < started_at + frame.duration
                 }
                 Animation::Keyframes(_, _, ref mut state) => {
@@ -86,8 +89,8 @@ pub fn update_animation_state(constellation_chan: &IpcSender<ConstellationMsg>,
                 continue
             }
 
-            if let Animation::Transition(_, unsafe_node, _, ref frame, _) = running_animation {
-                script_chan.send(ConstellationControlMsg::TransitionEnd(unsafe_node,
+            if let Animation::Transition(node, _, ref frame, _) = running_animation {
+                script_chan.send(ConstellationControlMsg::TransitionEnd(node.to_untrusted_node_address(),
                                                                         frame.property_animation
                                                                              .property_name().into(),
                                                                         frame.duration))
@@ -112,6 +115,17 @@ pub fn update_animation_state(constellation_chan: &IpcSender<ConstellationMsg>,
 
     // Add new running animations.
     for new_running_animation in new_running_animations {
+        if new_running_animation.is_transition() {
+            match newly_transitioning_nodes {
+                Some(ref mut nodes) => {
+                    nodes.push(new_running_animation.node().to_untrusted_node_address());
+                }
+                None => {
+                    warn!("New transition encountered from compositor-initiated layout.");
+                }
+            }
+        }
+
         running_animations.entry(*new_running_animation.node())
                           .or_insert_with(Vec::new)
                           .push(new_running_animation)

components/layout/context.rs

@@ -15,6 +15,7 @@ use net_traits::image_cache::{ImageOrMetadataAvailable, UsePlaceholder};
 use opaque_node::OpaqueNodeMethods;
 use parking_lot::RwLock;
 use script_layout_interface::{PendingImage, PendingImageState};
+use script_traits::UntrustedNodeAddress;
 use servo_url::ServoUrl;
 use std::borrow::{Borrow, BorrowMut};
 use std::cell::{RefCell, RefMut};
@@ -96,7 +97,11 @@ pub struct LayoutContext<'a> {
 
     /// A list of in-progress image loads to be shared with the script thread.
     /// A None value means that this layout was not initiated by the script thread.
-    pub pending_images: Option<Mutex<Vec<PendingImage>>>
+    pub pending_images: Option<Mutex<Vec<PendingImage>>>,
+
+    /// A list of nodes that have just initiated a CSS transition.
+    /// A None value means that this layout was not initiated by the script thread.
+    pub newly_transitioning_nodes: Option<Mutex<Vec<UntrustedNodeAddress>>>,
 }
 
 impl<'a> Drop for LayoutContext<'a> {

components/layout/query.rs

@@ -17,7 +17,6 @@ use inline::LAST_FRAGMENT_OF_ELEMENT;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::PipelineId;
 use opaque_node::OpaqueNodeMethods;
-use script_layout_interface::PendingImage;
 use script_layout_interface::rpc::{ContentBoxResponse, ContentBoxesResponse};
 use script_layout_interface::rpc::{HitTestResponse, LayoutRPC};
 use script_layout_interface::rpc::{MarginStyleResponse, NodeGeometryResponse};
@@ -28,7 +27,6 @@ use script_traits::LayoutMsg as ConstellationMsg;
 use script_traits::UntrustedNodeAddress;
 use sequential;
 use std::cmp::{min, max};
-use std::mem;
 use std::ops::Deref;
 use std::sync::{Arc, Mutex};
 use style::computed_values;
@@ -89,9 +87,6 @@ pub struct LayoutThreadData {
     /// Index in a text fragment. We need this do determine the insertion point.
     pub text_index_response: TextIndexResponse,
 
-    /// A list of images requests that need to be initiated.
-    pub pending_images: Vec<PendingImage>,
-
     /// A queued response for the list of nodes at a given point.
     pub nodes_from_point_response: Vec<UntrustedNodeAddress>,
 }
@@ -198,12 +193,6 @@ impl LayoutRPC for LayoutRPCImpl {
         let rw_data = rw_data.lock().unwrap();
         rw_data.text_index_response.clone()
     }
-
-    fn pending_images(&self) -> Vec<PendingImage> {
-        let &LayoutRPCImpl(ref rw_data) = self;
-        let mut rw_data = rw_data.lock().unwrap();
-        mem::replace(&mut rw_data.pending_images, vec![])
-    }
 }
 
 struct UnioningFragmentBorderBoxIterator {

components/layout_thread/lib.rs

@@ -81,7 +81,8 @@ use profile_traits::mem::{self, Report, ReportKind, ReportsChan};
 use profile_traits::time::{self, TimerMetadata, profile};
 use profile_traits::time::{TimerMetadataFrameType, TimerMetadataReflowType};
 use script::layout_wrapper::{ServoLayoutElement, ServoLayoutDocument, ServoLayoutNode};
-use script_layout_interface::message::{Msg, NewLayoutThreadInfo, Reflow, ReflowQueryType, ScriptReflow};
+use script_layout_interface::message::{Msg, NewLayoutThreadInfo, Reflow, ReflowQueryType};
+use script_layout_interface::message::{ScriptReflow, ReflowComplete};
 use script_layout_interface::reporter::CSSErrorReporter;
 use script_layout_interface::rpc::{LayoutRPC, MarginStyleResponse, NodeOverflowResponse, OffsetParentResponse};
 use script_layout_interface::rpc::TextIndexResponse;
@@ -292,6 +293,37 @@ impl LayoutThreadFactory for LayoutThread {
     }
 }
 
+struct ScriptReflowResult {
+    script_reflow: ScriptReflow,
+    result: RefCell<Option<ReflowComplete>>,
+}
+
+impl Deref for ScriptReflowResult {
+    type Target = ScriptReflow;
+    fn deref(&self) -> &ScriptReflow {
+        &self.script_reflow
+    }
+}
+
+impl ScriptReflowResult {
+    fn new(script_reflow: ScriptReflow) -> ScriptReflowResult {
+        ScriptReflowResult {
+            script_reflow: script_reflow,
+            result: RefCell::new(Some(Default::default())),
+        }
+    }
+}
+
+impl Drop for ScriptReflowResult {
+    fn drop(&mut self) {
+        self.script_reflow.script_join_chan.send(
+            self.result
+                .borrow_mut()
+                .take()
+                .unwrap()).unwrap();
+    }
+}
+
 /// The `LayoutThread` `rw_data` lock must remain locked until the first reflow,
 /// as RPC calls don't make sense until then. Use this in combination with
 /// `LayoutThread::lock_rw_data` and `LayoutThread::return_rw_data`.
@@ -476,7 +508,6 @@ impl LayoutThread {
                     margin_style_response: MarginStyleResponse::empty(),
                     stacking_context_scroll_offsets: HashMap::new(),
                     text_index_response: TextIndexResponse(None),
-                    pending_images: vec![],
                     nodes_from_point_response: vec![],
                 })),
             error_reporter: CSSErrorReporter {
@@ -513,7 +544,7 @@ impl LayoutThread {
     // Create a layout context for use in building display lists, hit testing, &c.
     fn build_layout_context<'a>(&'a self,
                                 guards: StylesheetGuards<'a>,
-                                request_images: bool,
+                                script_initiated_layout: bool,
                                 snapshot_map: &'a SnapshotMap)
                                 -> LayoutContext<'a> {
         let thread_local_style_context_creation_data =
@@ -537,7 +568,8 @@ impl LayoutThread {
             image_cache: self.image_cache.clone(),
             font_cache_thread: Mutex::new(self.font_cache_thread.clone()),
             webrender_image_cache: self.webrender_image_cache.clone(),
-            pending_images: if request_images { Some(Mutex::new(vec![])) } else { None },
+            pending_images: if script_initiated_layout { Some(Mutex::new(vec![])) } else { None },
+            newly_transitioning_nodes: if script_initiated_layout { Some(Mutex::new(vec![])) } else { None },
         }
     }
 
@@ -614,10 +646,11 @@ impl LayoutThread {
                                    Box<LayoutRPC + Send>).unwrap();
             },
             Msg::Reflow(data) => {
+                let mut data = ScriptReflowResult::new(data);
                 profile(time::ProfilerCategory::LayoutPerform,
                         self.profiler_metadata(),
                         self.time_profiler_chan.clone(),
-                        || self.handle_reflow(&data, possibly_locked_rw_data));
+                        || self.handle_reflow(&mut data, possibly_locked_rw_data));
             },
             Msg::TickAnimations => self.tick_all_animations(possibly_locked_rw_data),
             Msg::SetStackingContextScrollStates(new_scroll_states) => {
@@ -953,7 +986,7 @@ impl LayoutThread {
 
     /// The high-level routine that performs layout threads.
     fn handle_reflow<'a, 'b>(&mut self,
-                             data: &ScriptReflow,
+                             data: &mut ScriptReflowResult,
                              possibly_locked_rw_data: &mut RwData<'a, 'b>) {
         let document = unsafe { ServoLayoutNode::new(&data.document) };
         let document = document.as_document().unwrap();
@@ -1238,18 +1271,26 @@ impl LayoutThread {
 
         self.respond_to_query_if_necessary(&data.query_type,
                                            &mut *rw_data,
-                                           &mut layout_context);
+                                           &mut layout_context,
+                                           data.result.borrow_mut().as_mut().unwrap());
     }
 
     fn respond_to_query_if_necessary(&self,
                                      query_type: &ReflowQueryType,
                                      rw_data: &mut LayoutThreadData,
-                                     context: &mut LayoutContext) {
+                                     context: &mut LayoutContext,
+                                     reflow_result: &mut ReflowComplete) {
         let pending_images = match context.pending_images {
             Some(ref pending) => std_mem::replace(&mut *pending.lock().unwrap(), vec![]),
             None => vec![],
         };
-        rw_data.pending_images = pending_images;
+        reflow_result.pending_images = pending_images;
+
+        let newly_transitioning_nodes = match context.newly_transitioning_nodes {
+            Some(ref nodes) => std_mem::replace(&mut *nodes.lock().unwrap(), vec![]),
+            None => vec![],
+        };
+        reflow_result.newly_transitioning_nodes = newly_transitioning_nodes;
 
         let mut root_flow = match self.root_flow.borrow().clone() {
             Some(root_flow) => root_flow,
@@ -1426,6 +1467,7 @@ impl LayoutThread {
                                                          &mut *rw_data,
                                                          &mut layout_context);
             assert!(layout_context.pending_images.is_none());
+            assert!(layout_context.newly_transitioning_nodes.is_none());
         }
     }
 
@@ -1436,14 +1478,24 @@ impl LayoutThread {
                                                document: Option<&ServoLayoutDocument>,
                                                rw_data: &mut LayoutThreadData,
                                                context: &mut LayoutContext) {
-        // Kick off animations if any were triggered, expire completed ones.
-        animation::update_animation_state(&self.constellation_chan,
-                                          &self.script_chan,
-                                          &mut *self.running_animations.write(),
-                                          &mut *self.expired_animations.write(),
-                                          &self.new_animations_receiver,
-                                          self.id,
-                                          &self.timer);
+        {
+            let mut newly_transitioning_nodes = context
+                .newly_transitioning_nodes
+                .as_ref()
+                .map(|nodes| nodes.lock().unwrap());
+            let newly_transitioning_nodes = newly_transitioning_nodes
+                .as_mut()
+                .map(|nodes| &mut **nodes);
+            // Kick off animations if any were triggered, expire completed ones.
+            animation::update_animation_state(&self.constellation_chan,
+                                              &self.script_chan,
+                                              &mut *self.running_animations.write(),
+                                              &mut *self.expired_animations.write(),
+                                              newly_transitioning_nodes,
+                                              &self.new_animations_receiver,
+                                              self.id,
+                                              &self.timer);
+        }
 
         profile(time::ProfilerCategory::LayoutRestyleDamagePropagation,
                 self.profiler_metadata(),

components/script/dom/document.rs

@@ -826,6 +826,7 @@ impl Document {
         }
     }
 
+    #[allow(unsafe_code)]
     pub fn handle_mouse_event(&self,
                               js_runtime: *mut JSRuntime,
                               button: MouseButton,
@@ -841,7 +842,9 @@ impl Document {
         let node = match self.window.hit_test_query(client_point, false) {
             Some(node_address) => {
                 debug!("node address is {:?}", node_address);
-                node::from_untrusted_node_address(js_runtime, node_address)
+                unsafe {
+                    node::from_untrusted_node_address(js_runtime, node_address)
+                }
             },
             None => return,
         };
@@ -988,13 +991,16 @@ impl Document {
         *self.last_click_info.borrow_mut() = Some((now, click_pos));
     }
 
+    #[allow(unsafe_code)]
     pub fn handle_touchpad_pressure_event(&self,
                                           js_runtime: *mut JSRuntime,
                                           client_point: Point2D<f32>,
                                           pressure: f32,
                                           phase_now: TouchpadPressurePhase) {
         let node = match self.window.hit_test_query(client_point, false) {
-            Some(node_address) => node::from_untrusted_node_address(js_runtime, node_address),
+            Some(node_address) => unsafe {
+                node::from_untrusted_node_address(js_runtime, node_address)
+            },
             None => return
         };
 
@@ -1089,6 +1095,7 @@ impl Document {
         event.fire(target);
     }
 
+    #[allow(unsafe_code)]
     pub fn handle_mouse_move_event(&self,
                                    js_runtime: *mut JSRuntime,
                                    client_point: Option<Point2D<f32>>,
@@ -1104,7 +1111,7 @@ impl Document {
         };
 
         let maybe_new_target = self.window.hit_test_query(client_point, true).and_then(|address| {
-            let node = node::from_untrusted_node_address(js_runtime, address);
+            let node = unsafe { node::from_untrusted_node_address(js_runtime, address) };
             node.inclusive_ancestors()
                 .filter_map(Root::downcast::<Element>)
                 .next()
@@ -1186,6 +1193,7 @@ impl Document {
                            ReflowReason::MouseEvent);
     }
 
+    #[allow(unsafe_code)]
     pub fn handle_touch_event(&self,
                               js_runtime: *mut JSRuntime,
                               event_type: TouchEventType,
@@ -1202,7 +1210,9 @@ impl Document {
         };
 
         let node = match self.window.hit_test_query(point, false) {
-            Some(node_address) => node::from_untrusted_node_address(js_runtime, node_address),
+            Some(node_address) => unsafe {
+                node::from_untrusted_node_address(js_runtime, node_address)
+            },
             None => return TouchEventResult::Processed(false),
         };
         let el = match node.downcast::<Element>() {
@@ -3480,7 +3490,9 @@ impl DocumentMethods for Document {
             Some(untrusted_node_address) => {
                 let js_runtime = unsafe { JS_GetRuntime(window.get_cx()) };
 
-                let node = node::from_untrusted_node_address(js_runtime, untrusted_node_address);
+                let node = unsafe {
+                    node::from_untrusted_node_address(js_runtime, untrusted_node_address)
+                };
                 let parent_node = node.GetParentNode().unwrap();
                 let element_ref = node.downcast::<Element>().unwrap_or_else(|| {
                     parent_node.downcast::<Element>().unwrap()
@@ -3515,7 +3527,9 @@ impl DocumentMethods for Document {
         // Step 1 and Step 3
         let mut elements: Vec<Root<Element>> = self.nodes_from_point(point).iter()
             .flat_map(|&untrusted_node_address| {
-                let node = node::from_untrusted_node_address(js_runtime, untrusted_node_address);
+                let node = unsafe {
+                    node::from_untrusted_node_address(js_runtime, untrusted_node_address)
+                };
                 Root::downcast::<Element>(node)
         }).collect();