From bf743e57d6ca738747f0552b7d643e6e3bb16a08 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 16 Dec 2016 08:53:27 -0800 Subject: [PATCH] servo: Merge #14589 - Reduce calls into arbitrary code with the ScriptThread::documents borrow held (from servo:harden-script); r=jdm+Ms2ger Source-Repo: https://github.com/servo/servo Source-Revision: 0c56499bdc5c92bac44f8c33e4781899c34d64e1 --- servo/components/script/script_thread.rs | 122 ++++++++++-------- servo/python/tidy/servo_tidy/tidy.py | 3 + .../tidy/servo_tidy_tests/script_thread.rs | 18 +++ .../python/tidy/servo_tidy_tests/test_tidy.py | 6 + 4 files changed, 97 insertions(+), 52 deletions(-) create mode 100644 servo/python/tidy/servo_tidy_tests/script_thread.rs diff --git a/servo/components/script/script_thread.rs b/servo/components/script/script_thread.rs index b719a8faed65..c2d8deec6221 100644 --- a/servo/components/script/script_thread.rs +++ b/servo/components/script/script_thread.rs @@ -1142,7 +1142,8 @@ impl ScriptThread { } fn handle_resize(&self, id: PipelineId, size: WindowSizeData, size_type: WindowSizeType) { - if let Some(ref window) = self.documents.borrow().find_window(id) { + let window = self.documents.borrow().find_window(id); + if let Some(ref window) = window { window.set_resize_event(size, size_type); return; } @@ -1155,7 +1156,8 @@ impl ScriptThread { } fn handle_viewport(&self, id: PipelineId, rect: Rect) { - if let Some(document) = self.documents.borrow().find_document(id) { + let document = self.documents.borrow().find_document(id); + if let Some(document) = document { if document.window().set_page_clip_rect_with_new_viewport(rect) { self.rebuild_and_force_reflow(&document, ReflowReason::Viewport); } @@ -1172,7 +1174,7 @@ impl ScriptThread { fn handle_set_scroll_state(&self, id: PipelineId, scroll_states: &[(UntrustedNodeAddress, Point2D)]) { - 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), }; @@ -1240,7 +1242,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), }; @@ -1295,7 +1297,8 @@ impl ScriptThread { /// To slow/speed up timers and manage any other script thread resource based on visibility. /// Returns true if successful. fn alter_resource_utilization(&self, id: PipelineId, visible: bool) -> bool { - if let Some(window) = self.documents.borrow().find_window(id) { + let window = self.documents.borrow().find_window(id); + if let Some(window) = window { if visible { window.upcast::().speed_up_timers(); } else { @@ -1308,7 +1311,8 @@ impl ScriptThread { /// Updates iframe element after a change in visibility fn handle_visibility_change_complete_msg(&self, parent_pipeline_id: PipelineId, id: FrameId, visible: bool) { - if let Some(iframe) = self.documents.borrow().find_iframe(parent_pipeline_id, id) { + let iframe = self.documents.borrow().find_iframe(parent_pipeline_id, id); + if let Some(iframe) = iframe { iframe.change_visibility_status(visible); } } @@ -1336,7 +1340,8 @@ impl ScriptThread { /// Handles freeze message fn handle_freeze_msg(&self, id: PipelineId) { - if let Some(window) = self.documents.borrow().find_window(id) { + let window = self.documents.borrow().find_window(id); + if let Some(window) = window { window.upcast::().suspend(); return; } @@ -1350,7 +1355,8 @@ impl ScriptThread { /// Handles thaw message fn handle_thaw_msg(&self, id: PipelineId) { - if let Some(document) = self.documents.borrow().find_document(id) { + let document = self.documents.borrow().find_document(id); + if let Some(document) = document { if let Some(context) = document.browsing_context() { let needed_reflow = context.set_reflow_status(false); if needed_reflow { @@ -1401,14 +1407,16 @@ impl ScriptThread { parent_pipeline_id: PipelineId, frame_id: Option, event: MozBrowserEvent) { - match self.documents.borrow().find_document(parent_pipeline_id) { - None => warn!("Mozbrowser event after pipeline {:?} closed.", parent_pipeline_id), - Some(doc) => match frame_id { - None => doc.window().dispatch_mozbrowser_event(event), - Some(frame_id) => match doc.find_iframe(frame_id) { - None => warn!("Mozbrowser event after iframe {:?}/{:?} closed.", parent_pipeline_id, frame_id), - Some(frame_element) => frame_element.dispatch_mozbrowser_event(event), - }, + 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, + }; + + match frame_id { + None => doc.window().dispatch_mozbrowser_event(event), + Some(frame_id) => match doc.find_iframe(frame_id) { + None => warn!("Mozbrowser event after iframe {:?}/{:?} closed.", parent_pipeline_id, frame_id), + Some(frame_element) => frame_element.dispatch_mozbrowser_event(event), }, } } @@ -1417,7 +1425,8 @@ impl ScriptThread { parent_pipeline_id: PipelineId, frame_id: FrameId, new_pipeline_id: PipelineId) { - if let Some(frame_element) = self.documents.borrow().find_iframe(parent_pipeline_id, frame_id) { + let frame_element = self.documents.borrow().find_iframe(parent_pipeline_id, frame_id); + if let Some(frame_element) = frame_element { frame_element.update_pipeline_id(new_pipeline_id); } } @@ -1486,13 +1495,14 @@ impl ScriptThread { Some(r) => r, None => return }; - if let Some(window) = self.documents.borrow().find_window(pipeline_id) { - let script_url = maybe_registration.get_installed().get_script_url(); - let scope_things = ServiceWorkerRegistration::create_scope_things(window.upcast(), script_url); - let _ = self.constellation_chan.send(ConstellationMsg::RegisterServiceWorker(scope_things, scope.clone())); - } else { - warn!("Registration failed for {}", scope); - } + let window = match { self.documents.borrow().find_window(pipeline_id) } { + Some(window) => window, + None => return warn!("Registration failed for {}", scope), + }; + + let script_url = maybe_registration.get_installed().get_script_url(); + let scope_things = ServiceWorkerRegistration::create_scope_things(window.upcast(), script_url); + let _ = self.constellation_chan.send(ConstellationMsg::RegisterServiceWorker(scope_things, scope.clone())); } pub fn dispatch_job_queue(&self, job_handler: Box) { @@ -1543,7 +1553,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), }; @@ -1600,7 +1610,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), }; @@ -1641,7 +1651,8 @@ impl ScriptThread { /// Handles a Web font being loaded. Does nothing if the page no longer exists. fn handle_web_font_loaded(&self, pipeline_id: PipelineId) { - if let Some(document) = self.documents.borrow().find_document(pipeline_id) { + let document = self.documents.borrow().find_document(pipeline_id); + if let Some(document) = document { self.rebuild_and_force_reflow(&document, ReflowReason::WebFontLoaded); } } @@ -1649,12 +1660,14 @@ impl ScriptThread { /// Notify a window of a storage event fn handle_storage_event(&self, pipeline_id: PipelineId, storage_type: StorageType, url: ServoUrl, key: Option, old_value: Option, new_value: Option) { - let storage = 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) => match storage_type { - StorageType::Local => window.LocalStorage(), - StorageType::Session => window.SessionStorage(), - }, + Some(window) => window, + }; + + let storage = match storage_type { + StorageType::Local => window.LocalStorage(), + StorageType::Session => window.SessionStorage(), }; storage.queue_storage_event(url, key, old_value, new_value); @@ -1907,7 +1920,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), }; @@ -1979,17 +1992,19 @@ impl ScriptThread { } TouchpadPressureEvent(point, pressure, phase) => { - match self.documents.borrow().find_document(pipeline_id) { - Some(doc) => doc.handle_touchpad_pressure_event(self.js_runtime.rt(), point, pressure, phase), - None => warn!("Message sent to closed pipeline {}.", 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), + }; + doc.handle_touchpad_pressure_event(self.js_runtime.rt(), point, pressure, phase); } KeyEvent(ch, key, state, modifiers) => { - match self.documents.borrow().find_document(pipeline_id) { - Some(document) => document.dispatch_key_event(ch, key, state, modifiers, &self.constellation_chan), - None => warn!("Message sent to closed pipeline {}.", 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), + }; + document.dispatch_key_event(ch, key, state, modifiers, &self.constellation_chan); } } } @@ -1999,10 +2014,11 @@ impl ScriptThread { mouse_event_type: MouseEventType, button: MouseButton, point: Point2D) { - match self.documents.borrow().find_document(pipeline_id) { - Some(document) => document.handle_mouse_event(self.js_runtime.rt(), button, point, mouse_event_type), - None => warn!("Message sent to closed pipeline {}.", 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), + }; + document.handle_mouse_event(self.js_runtime.rt(), button, point, mouse_event_type); } fn handle_touch_event(&self, @@ -2011,13 +2027,14 @@ impl ScriptThread { identifier: TouchId, point: Point2D) -> TouchEventResult { - match self.documents.borrow().find_document(pipeline_id) { - Some(document) => document.handle_touch_event(self.js_runtime.rt(), event_type, identifier, point), + let document = match { self.documents.borrow().find_document(pipeline_id) } { + Some(document) => document, None => { warn!("Message sent to closed pipeline {}.", pipeline_id); - TouchEventResult::Processed(true) + return TouchEventResult::Processed(true); }, - } + }; + document.handle_touch_event(self.js_runtime.rt(), event_type, identifier, point) } /// https://html.spec.whatwg.org/multipage/#navigating-across-documents @@ -2043,7 +2060,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), }; @@ -2126,7 +2143,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, }; @@ -2174,7 +2191,8 @@ impl ScriptThread { } fn handle_reload(&self, pipeline_id: PipelineId) { - if let Some(window) = self.documents.borrow().find_window(pipeline_id) { + let window = self.documents.borrow().find_window(pipeline_id); + if let Some(window) = window { window.Location().Reload(); } } diff --git a/servo/python/tidy/servo_tidy/tidy.py b/servo/python/tidy/servo_tidy/tidy.py index b40bdd67194b..7855d4c7a50b 100644 --- a/servo/python/tidy/servo_tidy/tidy.py +++ b/servo/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/servo/python/tidy/servo_tidy_tests/script_thread.rs b/servo/python/tidy/servo_tidy_tests/script_thread.rs new file mode 100644 index 000000000000..5dbeaec0e17a --- /dev/null +++ b/servo/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/servo/python/tidy/servo_tidy_tests/test_tidy.py b/servo/python/tidy/servo_tidy_tests/test_tidy.py index c6fe8bd83faf..265c485a1061 100644 --- a/servo/python/tidy/servo_tidy_tests/test_tidy.py +++ b/servo/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])