Add a tidy check for problematic match cases in script_thread.rs

4 files changed, 41 insertions, 14 deletions

diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs
index d5d833ef872..703a5f64803 100644
--- a/components/script/script_thread.rs
+++ b/components/script/script_thread.rs
@@ -1175,7 +1175,7 @@ impl ScriptThread {
     fn handle_set_scroll_state(&self,
                                id: PipelineId,
                                scroll_states: &[(UntrustedNodeAddress, Point2D<f32>)]) {
-        let window = match self.documents.borrow().find_window(id) {
+        let window = match { self.documents.borrow().find_window(id) } {
             Some(window) => window,
             None => return warn!("Set scroll state message sent to nonexistent pipeline: {:?}", id),
         };
@@ -1243,7 +1243,7 @@ impl ScriptThread {
     }
 
     fn handle_loads_complete(&self, pipeline: PipelineId) {
-        let doc = match self.documents.borrow().find_document(pipeline) {
+        let doc = match { self.documents.borrow().find_document(pipeline) } {
             Some(doc) => doc,
             None => return warn!("Message sent to closed pipeline {}.", pipeline),
         };
@@ -1408,7 +1408,7 @@ impl ScriptThread {
                                    parent_pipeline_id: PipelineId,
                                    frame_id: Option<FrameId>,
                                    event: MozBrowserEvent) {
-        let doc = match self.documents.borrow().find_document(parent_pipeline_id) {
+        let doc = match { self.documents.borrow().find_document(parent_pipeline_id) } {
             None => return warn!("Mozbrowser event after pipeline {:?} closed.", parent_pipeline_id),
             Some(doc) => doc,
         };
@@ -1496,7 +1496,7 @@ impl ScriptThread {
             Some(r) => r,
             None => return
         };
-        let window = match self.documents.borrow().find_window(pipeline_id) {
+        let window = match { self.documents.borrow().find_window(pipeline_id) } {
             Some(window) => window,
             None => return warn!("Registration failed for {}", scope),
         };
@@ -1554,7 +1554,7 @@ impl ScriptThread {
 
     /// Handles a request for the window title.
     fn handle_get_title_msg(&self, pipeline_id: PipelineId) {
-        let document = match self.documents.borrow().find_document(pipeline_id) {
+        let document = match { self.documents.borrow().find_document(pipeline_id) } {
             Some(document) => document,
             None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
         };
@@ -1611,7 +1611,7 @@ impl ScriptThread {
 
     /// Handles when layout thread finishes all animation in one tick
     fn handle_tick_all_animations(&self, id: PipelineId) {
-        let document = match self.documents.borrow().find_document(id) {
+        let document = match { self.documents.borrow().find_document(id) } {
             Some(document) => document,
             None => return warn!("Message sent to closed pipeline {}.", id),
         };
@@ -1661,7 +1661,7 @@ impl ScriptThread {
     /// Notify a window of a storage event
     fn handle_storage_event(&self, pipeline_id: PipelineId, storage_type: StorageType, url: ServoUrl,
                             key: Option<String>, old_value: Option<String>, new_value: Option<String>) {
-        let window = match self.documents.borrow().find_window(pipeline_id) {
+        let window = match { self.documents.borrow().find_window(pipeline_id) } {
             None => return warn!("Storage event sent to closed pipeline {}.", pipeline_id),
             Some(window) => window,
         };
@@ -1921,7 +1921,7 @@ impl ScriptThread {
             }
 
             MouseMoveEvent(point) => {
-                let document = match self.documents.borrow().find_document(pipeline_id) {
+                let document = match { self.documents.borrow().find_document(pipeline_id) } {
                     Some(document) => document,
                     None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
                 };
@@ -1993,7 +1993,7 @@ impl ScriptThread {
             }
 
             TouchpadPressureEvent(point, pressure, phase) => {
-                let doc = match self.documents.borrow().find_document(pipeline_id) {
+                let doc = match { self.documents.borrow().find_document(pipeline_id) } {
                     Some(doc) => doc,
                     None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
                 };
@@ -2001,7 +2001,7 @@ impl ScriptThread {
             }
 
             KeyEvent(ch, key, state, modifiers) => {
-                let document = match self.documents.borrow().find_document(pipeline_id) {
+                let document = match { self.documents.borrow().find_document(pipeline_id) } {
                     Some(document) => document,
                     None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
                 };
@@ -2015,7 +2015,7 @@ impl ScriptThread {
                           mouse_event_type: MouseEventType,
                           button: MouseButton,
                           point: Point2D<f32>) {
-        let document = match self.documents.borrow().find_document(pipeline_id) {
+        let document = match { self.documents.borrow().find_document(pipeline_id) } {
             Some(document) => document,
             None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
         };
@@ -2028,7 +2028,7 @@ impl ScriptThread {
                           identifier: TouchId,
                           point: Point2D<f32>)
                           -> TouchEventResult {
-        let document = match self.documents.borrow().find_document(pipeline_id) {
+        let document = match { self.documents.borrow().find_document(pipeline_id) } {
             Some(document) => document,
             None => {
                 warn!("Message sent to closed pipeline {}.", pipeline_id);
@@ -2061,7 +2061,7 @@ impl ScriptThread {
     }
 
     fn handle_resize_event(&self, pipeline_id: PipelineId, new_size: WindowSizeData, size_type: WindowSizeType) {
-        let document = match self.documents.borrow().find_document(pipeline_id) {
+        let document = match { self.documents.borrow().find_document(pipeline_id) } {
             Some(document) => document,
             None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
         };
@@ -2144,7 +2144,7 @@ impl ScriptThread {
     }
 
     fn handle_parsing_complete(&self, id: PipelineId) {
-        let document = match self.documents.borrow().find_document(id) {
+        let document = match { self.documents.borrow().find_document(id) } {
             Some(document) => document,
             None => return,
         };
diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index b40bdd67194..7855d4c7a50 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -542,6 +542,9 @@ def check_rust(file_name, lines):
                 lambda match, line: line.startswith('use ')),
             (r"^\s*else {", "else braces should be on the same line", no_filter),
             (r"[^$ ]\([ \t]", "extra space after (", no_filter),
+            # This particular pattern is not reentrant-safe in script_thread.rs
+            (r"match self.documents.borrow", "use a separate variable for the match expression",
+             lambda match, line: file_name.endswith('script_thread.rs')),
         ]
 
         for pattern, message, filter_func in regex_rules:
diff --git a/python/tidy/servo_tidy_tests/script_thread.rs b/python/tidy/servo_tidy_tests/script_thread.rs
new file mode 100644
index 00000000000..5dbeaec0e17
--- /dev/null
+++ b/python/tidy/servo_tidy_tests/script_thread.rs
@@ -0,0 +1,18 @@
+fn main() {
+    // This should trigger an error.
+    match self.documents.borrow_mut() {
+        _ => {}
+    }
+    // This should trigger an error.
+    match self.documents.borrow() {
+        _ => {}
+    }
+    // This should not trigger an error.
+    match { self.documents.borrow().find_window(id) } {
+        => {}
+    }
+    // This should not trigger an error.
+    match self.documents_status.borrow() {
+        => {}
+    }
+}
diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py
index c6fe8bd83fa..265c485a106 100644
--- a/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/python/tidy/servo_tidy_tests/test_tidy.py
@@ -141,6 +141,12 @@ class CheckTidiness(unittest.TestCase):
         self.assertEqual('method declared in webidl is missing a comment with a specification link', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
+    def test_script_thread(self):
+        errors = tidy.collect_errors_for_files(iterFile('script_thread.rs'), [], [tidy.check_rust], print_text=False)
+        self.assertEqual('use a separate variable for the match expression', errors.next()[2])
+        self.assertEqual('use a separate variable for the match expression', errors.next()[2])
+        self.assertNoMoreErrors(errors)
+
     def test_webidl(self):
         errors = tidy.collect_errors_for_files(iterFile('spec.webidl'), [tidy.check_webidl_spec], [], print_text=False)
         self.assertEqual('No specification link found.', errors.next()[2])