Auto merge of #22474 - csmoe:unregister, r=gterzian

bors-servo · web-flow · commit 59a9da370319 · 2018-12-17T19:54:55.000-05:00
Unregister components while exiting r=@gterzian  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22468 (GitHub issue number if applicable)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22474)
diff --git a/components/background_hang_monitor/background_hang_monitor.rs b/components/background_hang_monitor/background_hang_monitor.rs
@@ -72,6 +72,8 @@ impl BackgroundHangMonitorClone for HangMonitorRegister {
 pub enum MonitoredComponentMsg {
     /// Register component for monitoring,
     Register(Box<Sampler>, Duration, Duration),
+    /// Unregister component for monitoring.
+    Unregister,
     /// Notify start of new activity for a given component,
     NotifyActivity(HangAnnotation),
     /// Notify start of waiting for a new task to come-in for processing.
@@ -119,6 +121,10 @@ impl BackgroundHangMonitor for BackgroundHangMonitorChan {
         let msg = MonitoredComponentMsg::NotifyWait;
         self.send(msg);
     }
+    fn unregister(&self) {
+        let msg = MonitoredComponentMsg::Unregister;
+        self.send(msg);
+    }
 }
 
 struct MonitoredComponent {
@@ -200,6 +206,12 @@ impl BackgroundHangMonitorWorker {
                     "This component was already registered for monitoring."
                 );
             },
+            (component_id, MonitoredComponentMsg::Unregister) => {
+                let _ = self
+                    .monitored_components
+                    .remove_entry(&component_id)
+                    .expect("Received Unregister for an unknown component");
+            },
             (component_id, MonitoredComponentMsg::NotifyActivity(annotation)) => {
                 let component = self
                     .monitored_components
diff --git a/components/background_hang_monitor/tests/hang_monitor_tests.rs b/components/background_hang_monitor/tests/hang_monitor_tests.rs
@@ -105,3 +105,30 @@ fn test_hang_monitoring() {
     // Still no new alerts because the hang monitor has shut-down already.
     assert!(background_hang_monitor_receiver.try_recv().is_err());
 }
+
+#[test]
+fn test_hang_monitoring_unregister() {
+    let (background_hang_monitor_ipc_sender, background_hang_monitor_receiver) =
+        ipc::channel().expect("ipc channel failure");
+
+    let background_hang_monitor_register =
+        HangMonitorRegister::init(background_hang_monitor_ipc_sender.clone());
+    let background_hang_monitor = background_hang_monitor_register.register_component(
+        MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
+        Duration::from_millis(10),
+        Duration::from_millis(1000),
+    );
+
+    // Start an activity.
+    let hang_annotation = HangAnnotation::Script(ScriptHangAnnotation::AttachLayout);
+    background_hang_monitor.notify_activity(hang_annotation);
+
+    // Unregister the component.
+    background_hang_monitor.unregister();
+
+    // Sleep until the "transient" timeout has been reached.
+    thread::sleep(Duration::from_millis(10));
+
+    // No new alert yet
+    assert!(background_hang_monitor_receiver.try_recv().is_err());
+}
diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs
@@ -926,6 +926,7 @@ impl LayoutThread {
         self.root_flow.borrow_mut().take();
         // Drop the rayon threadpool if present.
         let _ = self.parallel_traversal.take();
+        self.background_hang_monitor.unregister();
     }
 
     fn handle_add_stylesheet(&self, stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard) {
diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs
@@ -469,4 +469,6 @@ pub trait BackgroundHangMonitor {
     fn notify_activity(&self, annotation: HangAnnotation);
     /// Notify the start of waiting for a new event to come in.
     fn notify_wait(&self);
+    /// Unregister the component from monitor.
+    fn unregister(&self);
 }
diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs
@@ -2379,6 +2379,8 @@ impl ScriptThread {
             self.handle_exit_pipeline_msg(pipeline_id, DiscardBrowsingContext::Yes);
         }
 
+        self.background_hang_monitor.unregister();
+
         debug!("Exited script thread.");
     }
 

Original file line number	Diff line number	Diff line change
`@@ -926,6 +926,7 @@ impl LayoutThread {`
`926`	`926`	`self.root_flow.borrow_mut().take();`
`927`	`927`	`// Drop the rayon threadpool if present.`
`928`	`928`	`let _ = self.parallel_traversal.take();`
	`929`	`+ self.background_hang_monitor.unregister();`
`929`	`930`	`}`
`930`	`931`
`931`	`932`	`fn handle_add_stylesheet(&self, stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard) {`
Original file line number	Diff line number	Diff line change
`@@ -469,4 +469,6 @@ pub trait BackgroundHangMonitor {`
`469`	`469`	`fn notify_activity(&self, annotation: HangAnnotation);`
`470`	`470`	`/// Notify the start of waiting for a new event to come in.`
`471`	`471`	`fn notify_wait(&self);`
	`472`	`+ /// Unregister the component from monitor.`
	`473`	`+ fn unregister(&self);`
`472`	`474`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2379,6 +2379,8 @@ impl ScriptThread {`
`2379`	`2379`	`self.handle_exit_pipeline_msg(pipeline_id, DiscardBrowsingContext::Yes);`
`2380`	`2380`	`}`
`2381`	`2381`
	`2382`	`+ self.background_hang_monitor.unregister();`
	`2383`	`+`
`2382`	`2384`	`debug!("Exited script thread.");`
`2383`	`2385`	`}`
`2384`	`2386`