More lock acquire in callers

6 files changed, 46 insertions, 26 deletions

diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs
index c86c0221603..566d6c7ca33 100644
--- a/components/layout_thread/lib.rs
+++ b/components/layout_thread/lib.rs
@@ -115,7 +115,7 @@ use style::logical_geometry::LogicalPoint;
 use style::media_queries::{Device, MediaType};
 use style::parser::ParserContextExtraData;
 use style::servo::restyle_damage::{REFLOW, REFLOW_OUT_OF_FLOW, REPAINT, REPOSITION, STORE_OVERFLOW};
-use style::shared_lock::SharedRwLock;
+use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard};
 use style::stylesheets::{Origin, Stylesheet, UserAgentStylesheets};
 use style::stylist::Stylist;
 use style::thread_state;
@@ -345,13 +345,14 @@ impl<'a, 'b: 'a> RwData<'a, 'b> {
 }
 
 fn add_font_face_rules(stylesheet: &Stylesheet,
+                       guard: &SharedRwLockReadGuard,
                        device: &Device,
                        font_cache_thread: &FontCacheThread,
                        font_cache_sender: &IpcSender<()>,
                        outstanding_web_fonts_counter: &Arc<AtomicUsize>) {
     if opts::get().load_webfonts_synchronously {
         let (sender, receiver) = ipc::channel().unwrap();
-        stylesheet.effective_font_face_rules(&device, |font_face| {
+        stylesheet.effective_font_face_rules(&device, guard, |font_face| {
             let effective_sources = font_face.effective_sources();
             font_cache_thread.add_web_font(font_face.family.clone(),
                                            effective_sources,
@@ -359,7 +360,7 @@ fn add_font_face_rules(stylesheet: &Stylesheet,
             receiver.recv().unwrap();
         })
     } else {
-        stylesheet.effective_font_face_rules(&device, |font_face| {
+        stylesheet.effective_font_face_rules(&device, guard, |font_face| {
             let effective_sources = font_face.effective_sources();
             outstanding_web_fonts_counter.fetch_add(1, Ordering::SeqCst);
             font_cache_thread.add_web_font(font_face.family.clone(),
@@ -407,8 +408,11 @@ impl LayoutThread {
 
         let stylist = Arc::new(Stylist::new(device));
         let outstanding_web_fonts_counter = Arc::new(AtomicUsize::new(0));
-        for stylesheet in &*UA_STYLESHEETS.user_or_user_agent_stylesheets {
+        let ua_stylesheets = &*UA_STYLESHEETS;
+        let guard = ua_stylesheets.shared_lock.read();
+        for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets {
             add_font_face_rules(stylesheet,
+                                &guard,
                                 &stylist.device,
                                 &font_cache_thread,
                                 &ipc_font_cache_sender,
@@ -734,6 +738,7 @@ impl LayoutThread {
         let guard = stylesheet.shared_lock.read();
         if stylesheet.is_effective_for_device(&rw_data.stylist.device, &guard) {
             add_font_face_rules(&*stylesheet,
+                                &guard,
                                 &rw_data.stylist.device,
                                 &self.font_cache_thread,
                                 &self.font_cache_sender,
diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs
index 092bbdf8a93..6c6862fef5e 100644
--- a/components/style/stylesheets.rs
+++ b/components/style/stylesheets.rs
@@ -658,9 +658,9 @@ impl Stylesheet {
     /// nested rules will be skipped. Use `rules` if all rules need to be
     /// examined.
     #[inline]
-    pub fn effective_rules<F>(&self, device: &Device, mut f: F) where F: FnMut(&CssRule) {
-        let guard = self.shared_lock.read();  // FIXME: have the caller pass this?
-        effective_rules(&self.rules.read().0, device, &guard, &mut f);
+    pub fn effective_rules<F>(&self, device: &Device, guard: &SharedRwLockReadGuard, mut f: F)
+    where F: FnMut(&CssRule) {
+        effective_rules(&self.rules.read().0, device, guard, &mut f);
     }
 
     /// Returns whether the stylesheet has been explicitly disabled through the
@@ -701,8 +701,9 @@ macro_rules! rule_filter {
         impl Stylesheet {
             $(
                 #[allow(missing_docs)]
-                pub fn $method<F>(&self, device: &Device, mut f: F) where F: FnMut(&$rule_type) {
-                    self.effective_rules(device, |rule| {
+                pub fn $method<F>(&self, device: &Device, guard: &SharedRwLockReadGuard, mut f: F)
+                where F: FnMut(&$rule_type) {
+                    self.effective_rules(device, guard, |rule| {
                         if let CssRule::$variant(ref lock) = *rule {
                             let rule = lock.read();
                             f(&rule)
diff --git a/components/style/stylist.rs b/components/style/stylist.rs
index 27adfdd451a..0ff9d20d883 100644
--- a/components/style/stylist.rs
+++ b/components/style/stylist.rs
@@ -167,7 +167,9 @@ impl Stylist {
         }
 
         let cascaded_rule = ViewportRule {
-            declarations: viewport::Cascade::from_stylesheets(doc_stylesheets, &self.device).finish(),
+            declarations: viewport::Cascade::from_stylesheets(
+                doc_stylesheets, doc_guard, &self.device
+            ).finish(),
         };
 
         self.viewport_constraints =
@@ -237,7 +239,7 @@ impl Stylist {
         // Cheap `Arc` clone so that the closure below can borrow `&mut Stylist`.
         let device = self.device.clone();
 
-        stylesheet.effective_rules(&device, |rule| {
+        stylesheet.effective_rules(&device, guard, |rule| {
             match *rule {
                 CssRule::Style(ref style_rule) => {
                     let guard = style_rule.read();
@@ -467,7 +469,7 @@ impl Stylist {
     pub fn set_device(&mut self, mut device: Device, guard: &SharedRwLockReadGuard,
                       stylesheets: &[Arc<Stylesheet>]) {
         let cascaded_rule = ViewportRule {
-            declarations: viewport::Cascade::from_stylesheets(stylesheets, &device).finish(),
+            declarations: viewport::Cascade::from_stylesheets(stylesheets, guard, &device).finish(),
         };
 
         self.viewport_constraints =
diff --git a/components/style/viewport.rs b/components/style/viewport.rs
index 9c364dc7249..196216423f0 100644
--- a/components/style/viewport.rs
+++ b/components/style/viewport.rs
@@ -15,6 +15,7 @@ use cssparser::ToCss as ParserToCss;
 use euclid::size::TypedSize2D;
 use media_queries::Device;
 use parser::{ParserContext, log_css_error};
+use shared_lock::SharedRwLockReadGuard;
 use std::ascii::AsciiExt;
 use std::borrow::Cow;
 use std::fmt;
@@ -555,13 +556,14 @@ impl Cascade {
         }
     }
 
-    pub fn from_stylesheets<'a, I>(stylesheets: I, device: &Device) -> Self
+    pub fn from_stylesheets<'a, I>(stylesheets: I, guard: &SharedRwLockReadGuard,
+                                   device: &Device) -> Self
         where I: IntoIterator,
               I::Item: AsRef<Stylesheet>,
     {
         let mut cascade = Self::new();
         for stylesheet in stylesheets {
-            stylesheet.as_ref().effective_viewport_rules(device, |rule| {
+            stylesheet.as_ref().effective_viewport_rules(device, guard, |rule| {
                 for declaration in &rule.declarations {
                     cascade.add(Cow::Borrowed(declaration))
                 }
diff --git a/tests/unit/style/media_queries.rs b/tests/unit/style/media_queries.rs
index 485ccd1d486..652d68ce953 100644
--- a/tests/unit/style/media_queries.rs
+++ b/tests/unit/style/media_queries.rs
@@ -62,7 +62,7 @@ fn media_query_test(device: &Device, css: &str, expected_rule_count: usize) {
         None, &CSSErrorReporterTest,
         ParserContextExtraData::default());
     let mut rule_count = 0;
-    ss.effective_style_rules(device, |_| rule_count += 1);
+    ss.effective_style_rules(device, &ss.shared_lock.read(), |_| rule_count += 1);
     assert!(rule_count == expected_rule_count, css.to_owned());
 }
 
diff --git a/tests/unit/style/viewport.rs b/tests/unit/style/viewport.rs
index 9fe53ebb40c..7423286227d 100644
--- a/tests/unit/style/viewport.rs
+++ b/tests/unit/style/viewport.rs
@@ -20,12 +20,15 @@ use style_traits::viewport::*;
 
 macro_rules! stylesheet {
     ($css:expr, $origin:ident, $error_reporter:expr) => {
+        stylesheet!($css, $origin, $error_reporter, SharedRwLock::new())
+    };
+    ($css:expr, $origin:ident, $error_reporter:expr, $shared_lock:expr) => {
         Box::new(Stylesheet::from_str(
             $css,
             ServoUrl::parse("http://localhost").unwrap(),
             Origin::$origin,
             Default::default(),
-            SharedRwLock::new(),
+            $shared_lock,
             None,
             &$error_reporter,
             ParserContextExtraData::default()
@@ -41,7 +44,7 @@ fn test_viewport_rule<F>(css: &str,
     PREFS.set("layout.viewport.enabled", PrefValue::Boolean(true));
     let stylesheet = stylesheet!(css, Author, CSSErrorReporterTest);
     let mut rule_count = 0;
-    stylesheet.effective_viewport_rules(&device, |rule| {
+    stylesheet.effective_viewport_rules(&device, &stylesheet.shared_lock.read(), |rule| {
         rule_count += 1;
         callback(&rule.declarations, css);
     });
@@ -253,24 +256,31 @@ fn multiple_stylesheets_cascading() {
     PREFS.set("layout.viewport.enabled", PrefValue::Boolean(true));
     let device = Device::new(MediaType::Screen, TypedSize2D::new(800., 600.));
     let error_reporter = CSSErrorReporterTest;
+    let shared_lock = SharedRwLock::new();
     let stylesheets = vec![
-        stylesheet!("@viewport { min-width: 100px; min-height: 100px; zoom: 1; }", UserAgent, error_reporter),
-        stylesheet!("@viewport { min-width: 200px; min-height: 200px; }", User, error_reporter),
-        stylesheet!("@viewport { min-width: 300px; }", Author, error_reporter)];
-
-    let declarations = Cascade::from_stylesheets(&stylesheets, &device).finish();
+        stylesheet!("@viewport { min-width: 100px; min-height: 100px; zoom: 1; }",
+                    UserAgent, error_reporter, shared_lock.clone()),
+        stylesheet!("@viewport { min-width: 200px; min-height: 200px; }",
+                    User, error_reporter, shared_lock.clone()),
+        stylesheet!("@viewport { min-width: 300px; }",
+                    Author, error_reporter, shared_lock.clone())
+    ];
+
+    let declarations = Cascade::from_stylesheets(&stylesheets, &shared_lock.read(), &device).finish();
     assert_decl_len!(declarations == 3);
     assert_decl_eq!(&declarations[0], UserAgent, Zoom: Zoom::Number(1.));
     assert_decl_eq!(&declarations[1], User, MinHeight: viewport_length!(200., px));
     assert_decl_eq!(&declarations[2], Author, MinWidth: viewport_length!(300., px));
 
     let stylesheets = vec![
-        stylesheet!("@viewport { min-width: 100px !important; }", UserAgent, error_reporter),
+        stylesheet!("@viewport { min-width: 100px !important; }",
+                    UserAgent, error_reporter, shared_lock.clone()),
         stylesheet!("@viewport { min-width: 200px !important; min-height: 200px !important; }",
-        User, error_reporter),
+                    User, error_reporter, shared_lock.clone()),
         stylesheet!("@viewport { min-width: 300px !important; min-height: 300px !important; zoom: 3 !important; }",
-        Author, error_reporter)];
-    let declarations = Cascade::from_stylesheets(&stylesheets, &device).finish();
+                    Author, error_reporter, shared_lock.clone())
+    ];
+    let declarations = Cascade::from_stylesheets(&stylesheets, &shared_lock.read(), &device).finish();
     assert_decl_len!(declarations == 3);
     assert_decl_eq!(&declarations[0], UserAgent, MinWidth: viewport_length!(100., px), !important);
     assert_decl_eq!(&declarations[1], User, MinHeight: viewport_length!(200., px), !important);