From dabebdfbf5e502e3eef7cb0159b20f0fa5e74f65 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 7 Apr 2017 06:57:08 +0900 Subject: [PATCH 1/5] 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. --- components/layout/animation.rs | 13 +++-- components/layout/query.rs | 9 ++++ components/layout_thread/lib.rs | 4 ++ components/script/dom/window.rs | 6 ++- components/script/script_thread.rs | 47 +++++++++++++++---- components/script_layout_interface/rpc.rs | 2 + components/script_traits/lib.rs | 4 +- components/style/animation.rs | 26 ++++++---- components/style/matching.rs | 3 +- tests/wpt/mozilla/meta/MANIFEST.json | 10 ++++ .../tests/mozilla/transitionend_safety.html | 27 +++++++++++ 11 files changed, 126 insertions(+), 25 deletions(-) create mode 100644 tests/wpt/mozilla/tests/mozilla/transitionend_safety.html diff --git a/components/layout/animation.rs b/components/layout/animation.rs index be6d5baf77c..ebde01323e5 100644 --- a/components/layout/animation.rs +++ b/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, script_chan: &IpcSender, running_animations: &mut HashMap>, expired_animations: &mut HashMap>, + newly_transitioning_nodes: &mut Vec, new_animations_receiver: &Receiver, pipeline_id: PipelineId, timer: &Timer) { @@ -71,7 +74,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender, 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, 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,10 @@ pub fn update_animation_state(constellation_chan: &IpcSender, // Add new running animations. for new_running_animation in new_running_animations { + if new_running_animation.is_transition() { + newly_transitioning_nodes.push(new_running_animation.node().to_untrusted_node_address()); + } + running_animations.entry(*new_running_animation.node()) .or_insert_with(Vec::new) .push(new_running_animation) diff --git a/components/layout/query.rs b/components/layout/query.rs index c12a2b7f63d..a8d5913e3db 100644 --- a/components/layout/query.rs +++ b/components/layout/query.rs @@ -94,6 +94,9 @@ pub struct LayoutThreadData { /// A queued response for the list of nodes at a given point. pub nodes_from_point_response: Vec, + + /// A list of nodes that have just started a CSS transition. + pub newly_transitioning_nodes: Vec, } pub struct LayoutRPCImpl(pub Arc>); @@ -204,6 +207,12 @@ impl LayoutRPC for LayoutRPCImpl { let mut rw_data = rw_data.lock().unwrap(); mem::replace(&mut rw_data.pending_images, vec![]) } + + fn newly_transitioning_nodes(&self) -> Vec { + let &LayoutRPCImpl(ref rw_data) = self; + let mut rw_data = rw_data.lock().unwrap(); + mem::replace(&mut rw_data.newly_transitioning_nodes, vec![]) + } } struct UnioningFragmentBorderBoxIterator { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 9e8cc9bc165..2bef4bc1c21 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -478,6 +478,7 @@ impl LayoutThread { text_index_response: TextIndexResponse(None), pending_images: vec![], nodes_from_point_response: vec![], + newly_transitioning_nodes: vec![], })), error_reporter: CSSErrorReporter { pipelineid: id, @@ -1436,11 +1437,14 @@ impl LayoutThread { document: Option<&ServoLayoutDocument>, rw_data: &mut LayoutThreadData, context: &mut LayoutContext) { + assert!(rw_data.newly_transitioning_nodes.is_empty()); + // 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(), + &mut rw_data.newly_transitioning_nodes, &self.new_animations_receiver, self.id, &self.timer); diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 993749b00c2..a783c15531b 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -76,7 +76,7 @@ use script_layout_interface::rpc::{MarginStyleResponse, NodeScrollRootIdResponse use script_layout_interface::rpc::{ResolvedStyleResponse, TextIndexResponse}; use script_runtime::{CommonScriptMsg, ScriptChan, ScriptPort, ScriptThreadEventCategory}; use script_thread::{MainThreadScriptChan, MainThreadScriptMsg, Runnable, RunnableWrapper}; -use script_thread::{SendableMainThreadScriptChan, ImageCacheMsg}; +use script_thread::{SendableMainThreadScriptChan, ImageCacheMsg, ScriptThread}; use script_traits::{ConstellationControlMsg, LoadData, MozBrowserEvent, UntrustedNodeAddress}; use script_traits::{DocumentState, TimerEvent, TimerEventId}; use script_traits::{ScriptMsg as ConstellationMsg, TimerSchedulerMsg, WindowSizeData, WindowSizeType}; @@ -1150,6 +1150,7 @@ impl Window { /// off-main-thread layout. /// /// Returns true if layout actually happened, false otherwise. + #[allow(unsafe_code)] pub fn force_reflow(&self, goal: ReflowGoal, query_type: ReflowQueryType, @@ -1261,6 +1262,9 @@ impl Window { } } + let newly_transitioning_nodes = self.layout_rpc.newly_transitioning_nodes(); + ScriptThread::note_newly_transitioning_nodes(newly_transitioning_nodes); + true } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 8a861e92570..805b19a24b2 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -47,7 +47,7 @@ use dom::globalscope::GlobalScope; use dom::htmlanchorelement::HTMLAnchorElement; use dom::htmliframeelement::{HTMLIFrameElement, NavigationType}; use dom::mutationobserver::MutationObserver; -use dom::node::{Node, NodeDamage, window_from_node}; +use dom::node::{Node, NodeDamage, window_from_node, from_untrusted_node_address}; use dom::serviceworker::TrustedServiceWorkerAddress; use dom::serviceworkerregistration::ServiceWorkerRegistration; use dom::servoparser::{ParserContext, ServoParser}; @@ -69,7 +69,6 @@ use js::jsapi::{JSAutoCompartment, JSContext, JS_SetWrapObjectCallbacks}; use js::jsapi::{JSTracer, SetWindowProxyClass}; use js::jsval::UndefinedValue; use js::rust::Runtime; -use layout_wrapper::ServoLayoutNode; use mem::heap_size_of_self_and_children; use microtask::{MicrotaskQueue, Microtask}; use msg::constellation_msg::{FrameId, FrameType, PipelineId, PipelineNamespace}; @@ -109,7 +108,6 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{Receiver, Select, Sender, channel}; use std::thread; use style::context::ReflowGoal; -use style::dom::{TNode, UnsafeNode}; use style::thread_state; use task_source::dom_manipulation::{DOMManipulationTask, DOMManipulationTaskSource}; use task_source::file_reading::FileReadingTaskSource; @@ -490,6 +488,10 @@ pub struct ScriptThread { /// A list of pipelines containing documents that finished loading all their blocking /// resources during a turn of the event loop. docs_with_no_blocking_loads: DOMRefCell>>, + + /// A list of nodes with in-progress CSS transitions, which roots them for the duration + /// of the transition. + transitioning_nodes: DOMRefCell>>, } /// In the event of thread panic, all data on the stack runs its destructor. However, there @@ -574,6 +576,17 @@ impl ScriptThreadFactory for ScriptThread { } impl ScriptThread { + pub fn note_newly_transitioning_nodes(nodes: Vec) { + SCRIPT_THREAD_ROOT.with(|root| { + let script_thread = unsafe { &*root.get().unwrap() }; + let js_runtime = script_thread.js_runtime.rt(); + let new_nodes = nodes + .into_iter() + .map(|n| JS::from_ref(&*from_untrusted_node_address(js_runtime, n))); + script_thread.transitioning_nodes.borrow_mut().extend(new_nodes); + }) + } + pub fn add_mutation_observer(observer: &MutationObserver) { SCRIPT_THREAD_ROOT.with(|root| { let script_thread = unsafe { &*root.get().unwrap() }; @@ -742,6 +755,8 @@ impl ScriptThread { webvr_thread: state.webvr_thread, docs_with_no_blocking_loads: Default::default(), + + transitioning_nodes: Default::default(), } } @@ -1602,11 +1617,27 @@ impl ScriptThread { } /// Handles firing of transition events. - #[allow(unsafe_code)] - fn handle_transition_event(&self, unsafe_node: UnsafeNode, name: String, duration: f64) { - let node = unsafe { ServoLayoutNode::from_unsafe(&unsafe_node) }; - let node = unsafe { node.get_jsmanaged().get_for_script() }; - let window = window_from_node(node); + fn handle_transition_event(&self, unsafe_node: UntrustedNodeAddress, name: String, duration: f64) { + let js_runtime = self.js_runtime.rt(); + let node = from_untrusted_node_address(js_runtime, unsafe_node); + + let idx = self.transitioning_nodes + .borrow() + .iter() + .position(|n| &**n as *const _ == &*node as *const _); + match idx { + Some(idx) => { + self.transitioning_nodes.borrow_mut().remove(idx); + } + None => { + // If no index is found, we can't know whether this node is safe to use. + // It's better not to fire a DOM event than crash. + warn!("Ignoring transition end notification for unknown node."); + return; + } + } + + let window = window_from_node(&*node); // Not quite the right thing - see #13865. node.dirty(NodeDamage::NodeStyleDamaged); diff --git a/components/script_layout_interface/rpc.rs b/components/script_layout_interface/rpc.rs index 82dd9b9ff08..1c694a7ff8c 100644 --- a/components/script_layout_interface/rpc.rs +++ b/components/script_layout_interface/rpc.rs @@ -42,6 +42,8 @@ pub trait LayoutRPC { fn pending_images(&self) -> Vec; /// Requests the list of nodes from the given point. fn nodes_from_point_response(&self) -> Vec; + /// Requests the list of nodes that have just started CSS transitions in the last reflow. + fn newly_transitioning_nodes(&self) -> Vec; fn text_index(&self) -> TextIndexResponse; } diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 29baf8821e6..4f3cfca76a5 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -69,7 +69,7 @@ use std::collections::HashMap; use std::fmt; use std::sync::Arc; use std::sync::mpsc::{Receiver, Sender}; -use style_traits::{CSSPixel, UnsafeNode}; +use style_traits::CSSPixel; use webdriver_msg::{LoadStatus, WebDriverScriptCommand}; use webrender_traits::ClipId; use webvr_traits::{WebVREvent, WebVRMsg}; @@ -274,7 +274,7 @@ pub enum ConstellationControlMsg { /// Notifies script thread that all animations are done TickAllAnimations(PipelineId), /// Notifies the script thread of a transition end - TransitionEnd(UnsafeNode, String, f64), + TransitionEnd(UntrustedNodeAddress, String, f64), /// Notifies the script thread that a new Web font has been loaded, and thus the page should be /// reflowed. WebFontLoaded(PipelineId), diff --git a/components/style/animation.rs b/components/style/animation.rs index 632588988c2..5f53508ba4e 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -8,7 +8,7 @@ use Atom; use bezier::Bezier; use context::SharedStyleContext; -use dom::{OpaqueNode, UnsafeNode}; +use dom::OpaqueNode; use euclid::point::Point2D; use font_metrics::FontMetricsProvider; use keyframes::{KeyframesStep, KeyframesStepValue}; @@ -188,7 +188,7 @@ pub enum Animation { /// the f64 field is the start time as returned by `time::precise_time_s()`. /// /// The `bool` field is werther this animation should no longer run. - Transition(OpaqueNode, UnsafeNode, f64, AnimationFrame, bool), + Transition(OpaqueNode, f64, AnimationFrame, bool), /// A keyframes animation is identified by a name, and can have a /// node-dependent state (i.e. iteration count, etc.). Keyframes(OpaqueNode, Atom, KeyframesAnimationState), @@ -200,7 +200,7 @@ impl Animation { pub fn mark_as_expired(&mut self) { debug_assert!(!self.is_expired()); match *self { - Animation::Transition(_, _, _, _, ref mut expired) => *expired = true, + Animation::Transition(_, _, _, ref mut expired) => *expired = true, Animation::Keyframes(_, _, ref mut state) => state.expired = true, } } @@ -209,7 +209,7 @@ impl Animation { #[inline] pub fn is_expired(&self) -> bool { match *self { - Animation::Transition(_, _, _, _, expired) => expired, + Animation::Transition(_, _, _, expired) => expired, Animation::Keyframes(_, _, ref state) => state.expired, } } @@ -218,7 +218,7 @@ impl Animation { #[inline] pub fn node(&self) -> &OpaqueNode { match *self { - Animation::Transition(ref node, _, _, _, _) => node, + Animation::Transition(ref node, _, _, _) => node, Animation::Keyframes(ref node, _, _) => node, } } @@ -231,6 +231,15 @@ impl Animation { Animation::Keyframes(_, _, ref state) => state.is_paused(), } } + + /// Whether this animation is a transition. + #[inline] + pub fn is_transition(&self) -> bool { + match *self { + Animation::Transition(..) => true, + Animation::Keyframes(..) => false, + } + } } @@ -402,7 +411,6 @@ impl PropertyAnimation { #[cfg(feature = "servo")] pub fn start_transitions_if_applicable(new_animations_sender: &Sender, opaque_node: OpaqueNode, - unsafe_node: UnsafeNode, old_style: &ComputedValues, new_style: &mut Arc, timer: &Timer, @@ -436,7 +444,7 @@ pub fn start_transitions_if_applicable(new_animations_sender: &Sender let start_time = now + (box_style.transition_delay_mod(i).seconds() as f64); new_animations_sender - .send(Animation::Transition(opaque_node, unsafe_node, start_time, AnimationFrame { + .send(Animation::Transition(opaque_node, start_time, AnimationFrame { duration: box_style.transition_duration_mod(i).seconds() as f64, property_animation: property_animation, }, /* is_expired = */ false)).unwrap(); @@ -589,7 +597,7 @@ pub fn update_style_for_animation(context: &SharedStyleContext, debug_assert!(!animation.is_expired()); match *animation { - Animation::Transition(_, _, start_time, ref frame, _) => { + Animation::Transition(_, start_time, ref frame, _) => { debug!("update_style_for_animation: transition found"); let now = context.timer.seconds(); let mut new_style = (*style).clone(); @@ -767,7 +775,7 @@ pub fn complete_expired_transitions(node: OpaqueNode, style: &mut Arc + +Asynchronous transitionend event is not a GC hazard + + + + + From c3b9714ab79a8230162605236f7c2f5e247707a7 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 10 Apr 2017 17:02:25 +1000 Subject: [PATCH 2/5] Accumulate transitioning nodes inside the layout context. --- components/layout/animation.rs | 9 +++++--- components/layout/context.rs | 7 +++++- components/layout_thread/lib.rs | 41 ++++++++++++++++++++++----------- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/components/layout/animation.rs b/components/layout/animation.rs index ebde01323e5..9b1d0671b9a 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -26,7 +26,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender, script_chan: &IpcSender, running_animations: &mut HashMap>, expired_animations: &mut HashMap>, - newly_transitioning_nodes: &mut Vec, + mut newly_transitioning_nodes: Option<&mut Vec>, new_animations_receiver: &Receiver, pipeline_id: PipelineId, timer: &Timer) { @@ -115,8 +115,11 @@ pub fn update_animation_state(constellation_chan: &IpcSender, // Add new running animations. for new_running_animation in new_running_animations { - if new_running_animation.is_transition() { - newly_transitioning_nodes.push(new_running_animation.node().to_untrusted_node_address()); + match newly_transitioning_nodes { + Some(ref mut nodes) if new_running_animation.is_transition() => { + nodes.push(new_running_animation.node().to_untrusted_node_address()); + } + _ => () } running_animations.entry(*new_running_animation.node()) diff --git a/components/layout/context.rs b/components/layout/context.rs index 214061e276f..8acbc3b254a 100644 --- a/components/layout/context.rs +++ b/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>> + pub pending_images: Option>>, + + /// 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>>, } impl<'a> Drop for LayoutContext<'a> { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 2bef4bc1c21..32b47d57a22 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -514,7 +514,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 = @@ -538,7 +538,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 }, } } @@ -1252,6 +1253,12 @@ impl LayoutThread { }; rw_data.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![], + }; + rw_data.newly_transitioning_nodes = newly_transitioning_nodes; + let mut root_flow = match self.root_flow.borrow().clone() { Some(root_flow) => root_flow, None => return, @@ -1427,6 +1434,7 @@ impl LayoutThread { &mut *rw_data, &mut layout_context); assert!(layout_context.pending_images.is_none()); + assert!(layout_context.newly_transitioning_nodes.is_none()); } } @@ -1437,17 +1445,24 @@ impl LayoutThread { document: Option<&ServoLayoutDocument>, rw_data: &mut LayoutThreadData, context: &mut LayoutContext) { - assert!(rw_data.newly_transitioning_nodes.is_empty()); - - // 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(), - &mut rw_data.newly_transitioning_nodes, - &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(), From 913491884431b9030db9a8a0a10011fa03aa2db6 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 10 Apr 2017 17:47:20 +1000 Subject: [PATCH 3/5] Send information to script as part of finishing layout. This avoids the need for multiple layout RPC operations immediately following return of control to script. This means that layout and script can continue to operate in parallel at this point, rather than one potentially waiting on the shared mutex to be unlocked. --- components/layout/query.rs | 20 -------- components/layout_thread/lib.rs | 51 +++++++++++++++---- components/script/dom/window.rs | 14 +++-- components/script_layout_interface/message.rs | 21 ++++---- components/script_layout_interface/rpc.rs | 5 -- 5 files changed, 60 insertions(+), 51 deletions(-) diff --git a/components/layout/query.rs b/components/layout/query.rs index a8d5913e3db..476a68afafe 100644 --- a/components/layout/query.rs +++ b/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,14 +87,8 @@ 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, - /// A queued response for the list of nodes at a given point. pub nodes_from_point_response: Vec, - - /// A list of nodes that have just started a CSS transition. - pub newly_transitioning_nodes: Vec, } pub struct LayoutRPCImpl(pub Arc>); @@ -201,18 +193,6 @@ impl LayoutRPC for LayoutRPCImpl { let rw_data = rw_data.lock().unwrap(); rw_data.text_index_response.clone() } - - fn pending_images(&self) -> Vec { - let &LayoutRPCImpl(ref rw_data) = self; - let mut rw_data = rw_data.lock().unwrap(); - mem::replace(&mut rw_data.pending_images, vec![]) - } - - fn newly_transitioning_nodes(&self) -> Vec { - let &LayoutRPCImpl(ref rw_data) = self; - let mut rw_data = rw_data.lock().unwrap(); - mem::replace(&mut rw_data.newly_transitioning_nodes, vec![]) - } } struct UnioningFragmentBorderBoxIterator { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 32b47d57a22..7a3bf5cb9ab 100644 --- a/components/layout_thread/lib.rs +++ b/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>, +} + +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,9 +508,7 @@ 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![], - newly_transitioning_nodes: vec![], })), error_reporter: CSSErrorReporter { pipelineid: id, @@ -616,10 +646,11 @@ impl LayoutThread { Box).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) => { @@ -955,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(); @@ -1240,24 +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![], }; - rw_data.newly_transitioning_nodes = newly_transitioning_nodes; + reflow_result.newly_transitioning_nodes = newly_transitioning_nodes; let mut root_flow = match self.root_flow.borrow().clone() { Some(root_flow) => root_flow, diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index a783c15531b..3d55a0e82aa 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -1214,16 +1214,16 @@ impl Window { debug!("script: layout forked"); - match join_port.try_recv() { + let complete = match join_port.try_recv() { Err(Empty) => { info!("script: waiting on layout"); - join_port.recv().unwrap(); + join_port.recv().unwrap() } - Ok(_) => {} + Ok(reflow_complete) => reflow_complete, Err(Disconnected) => { panic!("Layout thread failed while script was waiting for a result."); } - } + }; debug!("script: layout joined"); @@ -1237,8 +1237,7 @@ impl Window { self.emit_timeline_marker(marker.end()); } - let pending_images = self.layout_rpc.pending_images(); - for image in pending_images { + for image in complete.pending_images { let id = image.id; let js_runtime = self.js_runtime.borrow(); let js_runtime = js_runtime.as_ref().unwrap(); @@ -1262,8 +1261,7 @@ impl Window { } } - let newly_transitioning_nodes = self.layout_rpc.newly_transitioning_nodes(); - ScriptThread::note_newly_transitioning_nodes(newly_transitioning_nodes); + ScriptThread::note_newly_transitioning_nodes(complete.newly_transitioning_nodes); true } diff --git a/components/script_layout_interface/message.rs b/components/script_layout_interface/message.rs index 8b04a131404..9c2f94b0dc0 100644 --- a/components/script_layout_interface/message.rs +++ b/components/script_layout_interface/message.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use {OpaqueStyleAndLayoutData, TrustedNodeAddress}; +use {OpaqueStyleAndLayoutData, TrustedNodeAddress, PendingImage}; use app_units::Au; use euclid::point::Point2D; use euclid::rect::Rect; @@ -12,7 +12,7 @@ use msg::constellation_msg::PipelineId; use net_traits::image_cache::ImageCache; use profile_traits::mem::ReportsChan; use rpc::LayoutRPC; -use script_traits::{ConstellationControlMsg, LayoutControlMsg}; +use script_traits::{ConstellationControlMsg, LayoutControlMsg, UntrustedNodeAddress}; use script_traits::{LayoutMsg as ConstellationMsg, StackingContextScrollState, WindowSizeData}; use servo_url::ServoUrl; use std::sync::Arc; @@ -109,6 +109,15 @@ pub struct Reflow { pub page_clip_rect: Rect, } +/// Information derived from a layout pass that needs to be returned to the script thread. +#[derive(Default)] +pub struct ReflowComplete { + /// The list of images that were encountered that are in progress. + pub pending_images: Vec, + /// The list of nodes that initiated a CSS transition. + pub newly_transitioning_nodes: Vec, +} + /// Information needed for a script-initiated reflow. pub struct ScriptReflow { /// General reflow data. @@ -122,19 +131,13 @@ pub struct ScriptReflow { /// The current window size. pub window_size: WindowSizeData, /// The channel that we send a notification to. - pub script_join_chan: Sender<()>, + pub script_join_chan: Sender, /// The type of query if any to perform during this reflow. pub query_type: ReflowQueryType, /// The number of objects in the dom #10110 pub dom_count: u32, } -impl Drop for ScriptReflow { - fn drop(&mut self) { - self.script_join_chan.send(()).unwrap(); - } -} - pub struct NewLayoutThreadInfo { pub id: PipelineId, pub url: ServoUrl, diff --git a/components/script_layout_interface/rpc.rs b/components/script_layout_interface/rpc.rs index 1c694a7ff8c..a39e51d3633 100644 --- a/components/script_layout_interface/rpc.rs +++ b/components/script_layout_interface/rpc.rs @@ -2,7 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use PendingImage; use app_units::Au; use euclid::point::Point2D; use euclid::rect::Rect; @@ -38,12 +37,8 @@ pub trait LayoutRPC { fn offset_parent(&self) -> OffsetParentResponse; /// Query layout for the resolve values of the margin properties for an element. fn margin_style(&self) -> MarginStyleResponse; - /// Requests the list of not-yet-loaded images that were encountered in the last reflow. - fn pending_images(&self) -> Vec; /// Requests the list of nodes from the given point. fn nodes_from_point_response(&self) -> Vec; - /// Requests the list of nodes that have just started CSS transitions in the last reflow. - fn newly_transitioning_nodes(&self) -> Vec; fn text_index(&self) -> TextIndexResponse; } From 2ca80a800f8c1f85135bbdb605f69641ae9aa7d0 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 10 Apr 2017 17:53:46 +1000 Subject: [PATCH 4/5] Warn about ignored transitions which can't be rooted. --- components/layout/animation.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/components/layout/animation.rs b/components/layout/animation.rs index 9b1d0671b9a..d1673ac44a1 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -115,11 +115,15 @@ pub fn update_animation_state(constellation_chan: &IpcSender, // Add new running animations. for new_running_animation in new_running_animations { - match newly_transitioning_nodes { - Some(ref mut nodes) if new_running_animation.is_transition() => { - nodes.push(new_running_animation.node().to_untrusted_node_address()); + 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()) From b0bf2b4bad636acfba66d55571b417ebae795408 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 10 May 2017 18:20:31 -0400 Subject: [PATCH 5/5] Make methods storing layout-originating nodes unsafe. --- components/script/dom/document.rs | 26 ++++++++++++++++++++------ components/script/dom/node.rs | 18 ++++++++---------- components/script/dom/window.rs | 9 ++++++--- components/script/script_thread.rs | 8 +++++--- 4 files changed, 39 insertions(+), 22 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 2f0d9dcfde3..bce898e6347 100644 --- a/components/script/dom/document.rs +++ b/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, 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>, @@ -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::) .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::() { @@ -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::().unwrap_or_else(|| { parent_node.downcast::().unwrap() @@ -3515,7 +3527,9 @@ impl DocumentMethods for Document { // Step 1 and Step 3 let mut elements: Vec> = 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::(node) }).collect(); diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 81a06850757..6d16cac6731 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -927,20 +927,18 @@ fn first_node_not_in(mut nodes: I, not_in: &[NodeOrString]) -> Option Root { - unsafe { - // https://github.com/servo/servo/issues/6383 - let candidate: uintptr_t = mem::transmute(candidate.0); + // https://github.com/servo/servo/issues/6383 + let candidate: uintptr_t = mem::transmute(candidate.0); // let object: *mut JSObject = jsfriendapi::bindgen::JS_GetAddressableObject(runtime, // candidate); - let object: *mut JSObject = mem::transmute(candidate); - if object.is_null() { - panic!("Attempted to create a `JS` from an invalid pointer!") - } - let boxed_node = conversions::private_from_object(object) as *const Node; - Root::from_ref(&*boxed_node) + let object: *mut JSObject = mem::transmute(candidate); + if object.is_null() { + panic!("Attempted to create a `JS` from an invalid pointer!") } + let boxed_node = conversions::private_from_object(object) as *const Node; + Root::from_ref(&*boxed_node) } #[allow(unsafe_code)] diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 3d55a0e82aa..4317d2890b7 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -1241,7 +1241,7 @@ impl Window { let id = image.id; let js_runtime = self.js_runtime.borrow(); let js_runtime = js_runtime.as_ref().unwrap(); - let node = from_untrusted_node_address(js_runtime.rt(), image.node); + let node = unsafe { from_untrusted_node_address(js_runtime.rt(), image.node) }; if let PendingImageState::Unrequested(ref url) = image.state { fetch_image_for_layout(url.clone(), &*node, id, self.image_cache.clone()); @@ -1261,7 +1261,9 @@ impl Window { } } - ScriptThread::note_newly_transitioning_nodes(complete.newly_transitioning_nodes); + unsafe { + ScriptThread::note_newly_transitioning_nodes(complete.newly_transitioning_nodes); + } true } @@ -1457,6 +1459,7 @@ impl Window { DOMString::from(resolved) } + #[allow(unsafe_code)] pub fn offset_parent_query(&self, node: TrustedNodeAddress) -> (Option>, Rect) { if !self.reflow(ReflowGoal::ForScriptQuery, ReflowQueryType::OffsetParentQuery(node), @@ -1468,7 +1471,7 @@ impl Window { let js_runtime = self.js_runtime.borrow(); let js_runtime = js_runtime.as_ref().unwrap(); let element = response.node_address.and_then(|parent_node_address| { - let node = from_untrusted_node_address(js_runtime.rt(), parent_node_address); + let node = unsafe { from_untrusted_node_address(js_runtime.rt(), parent_node_address) }; Root::downcast(node) }); (element, response.rect) diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 805b19a24b2..b817fc77d1d 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -576,9 +576,9 @@ impl ScriptThreadFactory for ScriptThread { } impl ScriptThread { - pub fn note_newly_transitioning_nodes(nodes: Vec) { + pub unsafe fn note_newly_transitioning_nodes(nodes: Vec) { SCRIPT_THREAD_ROOT.with(|root| { - let script_thread = unsafe { &*root.get().unwrap() }; + let script_thread = &*root.get().unwrap(); let js_runtime = script_thread.js_runtime.rt(); let new_nodes = nodes .into_iter() @@ -1619,7 +1619,9 @@ impl ScriptThread { /// Handles firing of transition events. fn handle_transition_event(&self, unsafe_node: UntrustedNodeAddress, name: String, duration: f64) { let js_runtime = self.js_runtime.rt(); - let node = from_untrusted_node_address(js_runtime, unsafe_node); + let node = unsafe { + from_untrusted_node_address(js_runtime, unsafe_node) + }; let idx = self.transitioning_nodes .borrow()