[component_manager] Ensure Discovered dispatched before Purged

This change fixes a race condition in component destruction where the
following occurs:

1. Component is created and added to parent as child.
2. Component is destroyed.
3. Discover action is run for component.

In this sequence of operations, the Discover action in #3 will no-op
(and error) because the component is in a Destroyed state. This means a
Discovered event is never dispatched for the component, even though
Destroyed is. This is hypothesized to trigger the crash in the bug.

Usually, #2 and #3 happen in the other order, which gives the expected
behavior: Discovered event followed by Purged.

Fix by forcing the Purge action to register a Discover action.

Bug: 78289

Change-Id: Ibd15f8f9fb1b6e7f44b07b9ac04cdfb1ad427e60
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/540483
Commit-Queue: Gary Bressler <[email protected]>
Reviewed-by: Fady Samuel <[email protected]>

src/sys/component_manager/src/model/actions/purge.rs

@@ -50,6 +50,10 @@ async fn do_purge(component: &Arc<ComponentInstance>) -> Result<(), ModelError>
         );
     }
 
+    // Require the component to be discovered before purging it so a Purged event is always
+    // preceded by a Discovered.
+    ActionSet::register(component.clone(), DiscoverAction::new()).await?;
+
     let nfs = {
         match *component.lock_state().await {
             InstanceState::Resolved(ref s) => {
@@ -97,7 +101,6 @@ async fn do_purge(component: &Arc<ComponentInstance>) -> Result<(), ModelError>
     let nfs = {
         let actions = component.lock_actions().await;
         vec![
-            wait(actions.wait(DiscoverAction::new())),
             wait(actions.wait(ResolveAction::new())),
             wait(actions.wait(StartAction::new(BindReason::Unsupported))),
         ]
@@ -134,8 +137,9 @@ pub mod tests {
         },
         cm_rust::EventMode,
         cm_rust_testing::ComponentDeclBuilder,
-        fuchsia_async as fasync, fuchsia_zircon as zx,
+        fidl_fuchsia_sys2 as fsys, fuchsia_async as fasync, fuchsia_zircon as zx,
         futures::{join, FutureExt},
+        matches::assert_matches,
         moniker::{AbsoluteMoniker, ChildMoniker, PartialChildMoniker},
         std::sync::atomic::Ordering,
         std::sync::Weak,
@@ -158,15 +162,15 @@ pub mod tests {
             .expect("could not bind to a");
         assert!(is_executing(&component_a).await);
 
-        // Register shutdown first because DeleteChild requires the component to be shut down.
+        // Register shutdown first because PurgeChild requires the component to be shut down.
         ActionSet::register(component_a.clone(), ShutdownAction::new())
             .await
             .expect("shutdown failed");
         // Register delete child action, and wait for it. Component should be purged.
         ActionSet::register(component_root.clone(), PurgeChildAction::new("a:0".into()))
             .await
             .expect("purge failed");
-        // DeleteChild should not mark the instance non-live. That's done by Destroyed which we
+        // PurgeChild should not mark the instance non-live. That's done by Destroy which we
         // don't call here.
         assert!(!is_destroyed(&component_root, &"a:0".into()).await);
         assert!(is_child_deleted(&component_root, &component_a).await);
@@ -298,13 +302,21 @@ pub mod tests {
         }
     }
 
-    async fn setup_purge_blocks_test(event_type: EventType) -> (ActionsTest, EventStream) {
+    async fn setup_purge_blocks_test(event_types: Vec<EventType>) -> (ActionsTest, EventStream) {
         let components = vec![
             ("root", ComponentDeclBuilder::new().add_lazy_child("a").build()),
             ("a", component_decl_with_test_runner()),
         ];
         let test = ActionsTest::new("root", components, None).await;
-        let events = vec![event_type.into()];
+        let event_stream = setup_purge_blocks_test_event_stream(&test, event_types).await;
+        (test, event_stream)
+    }
+
+    async fn setup_purge_blocks_test_event_stream(
+        test: &ActionsTest,
+        event_types: Vec<EventType>,
+    ) -> EventStream {
+        let events: Vec<_> = event_types.into_iter().map(|e| e.into()).collect();
         let mut event_source = test
             .builtin_environment
             .lock()
@@ -327,7 +339,7 @@ pub mod tests {
         }
         let model = test.model.clone();
         fasync::Task::spawn(async move { model.start().await }).detach();
-        (test, event_stream)
+        event_stream
     }
 
     async fn run_purge_blocks_test<A>(
@@ -390,7 +402,7 @@ pub mod tests {
 
     #[fuchsia::test]
     async fn purge_blocks_on_discover() {
-        let (test, mut event_stream) = setup_purge_blocks_test(EventType::Discovered).await;
+        let (test, mut event_stream) = setup_purge_blocks_test(vec![EventType::Discovered]).await;
         run_purge_blocks_test(
             &test,
             &mut event_stream,
@@ -405,9 +417,76 @@ pub mod tests {
         .await;
     }
 
+    #[fuchsia::test]
+    async fn purge_registers_discover() {
+        let components = vec![("root", ComponentDeclBuilder::new().build())];
+        let test = ActionsTest::new("root", components, None).await;
+        let component_root = test.look_up(vec![].into()).await;
+        // This setup circumvents the registration of the Discover action on component_a.
+        {
+            let mut resolved_state = component_root.lock_resolved_state().await.unwrap();
+            let child = cm_rust::ChildDecl {
+                name: format!("a"),
+                url: format!("test:///a"),
+                startup: fsys::StartupMode::Lazy,
+                environment: None,
+            };
+            resolved_state
+                .add_child_for_test(
+                    &component_root,
+                    &child,
+                    None,
+                    false, /* !register_discover */
+                )
+                .await
+                .unwrap()
+                .await
+                .unwrap();
+        }
+        let mut event_stream = setup_purge_blocks_test_event_stream(
+            &test,
+            vec![EventType::Discovered, EventType::Purged],
+        )
+        .await;
+
+        // Shut down component so we can purge it.
+        let component_root = test.look_up(vec![].into()).await;
+        let component_a = match *component_root.lock_state().await {
+            InstanceState::Resolved(ref s) => {
+                s.get_live_child(&PartialChildMoniker::from("a")).expect("child a not found")
+            }
+            _ => panic!("not resolved"),
+        };
+        ActionSet::register(component_a.clone(), ShutdownAction::new())
+            .await
+            .expect("shutdown failed");
+
+        // Confirm component is still in New state.
+        {
+            let state = &*component_a.lock_state().await;
+            assert_matches!(state, InstanceState::New);
+        };
+
+        // Register PurgeChild.
+        let nf = {
+            let mut actions = component_root.lock_actions().await;
+            actions.register_no_wait(&component_root, PurgeChildAction::new("a:0".into()))
+        };
+
+        // Wait for Discover action, which should be registered by Purge, followed by
+        // Purged.
+        let event =
+            event_stream.wait_until(EventType::Discovered, vec!["a:0"].into()).await.unwrap();
+        event.resume();
+        let event = event_stream.wait_until(EventType::Purged, vec!["a:0"].into()).await.unwrap();
+        event.resume();
+        nf.await.unwrap();
+        assert!(is_child_deleted(&component_root, &component_a).await);
+    }
+
     #[fuchsia::test]
     async fn purge_blocks_on_resolve() {
-        let (test, mut event_stream) = setup_purge_blocks_test(EventType::Resolved).await;
+        let (test, mut event_stream) = setup_purge_blocks_test(vec![EventType::Resolved]).await;
         let event = event_stream.wait_until(EventType::Resolved, vec![].into()).await.unwrap();
         event.resume();
         // Cause `a` to resolve.
@@ -433,7 +512,7 @@ pub mod tests {
 
     #[fuchsia::test]
     async fn purge_blocks_on_start() {
-        let (test, mut event_stream) = setup_purge_blocks_test(EventType::Started).await;
+        let (test, mut event_stream) = setup_purge_blocks_test(vec![EventType::Started]).await;
         let event = event_stream.wait_until(EventType::Started, vec![].into()).await.unwrap();
         event.resume();
         // Cause `a` to start.
@@ -481,7 +560,7 @@ pub mod tests {
             _ => panic!("not resolved"),
         };
 
-        // Register delete action on "a", and wait for it.
+        // Register purge action on "a", and wait for it.
         ActionSet::register(component_a.clone(), ShutdownAction::new())
             .await
             .expect("shutdown failed");

src/sys/component_manager/src/model/component.rs

@@ -8,8 +8,8 @@ use {
         channel,
         model::{
             actions::{
-                Action, ActionSet, DestroyChildAction, DiscoverAction, PurgeChildAction,
-                ResolveAction, StopAction,
+                ActionSet, DestroyChildAction, DiscoverAction, PurgeChildAction, ResolveAction,
+                StopAction,
             },
             binding,
             component_controller::ComponentController,
@@ -1025,7 +1025,30 @@ impl ResolvedInstanceState {
         component: &Arc<ComponentInstance>,
         child: &ChildDecl,
         collection: Option<&CollectionDecl>,
-    ) -> Option<impl Future<Output = <DiscoverAction as Action>::Output>> {
+    ) -> Option<BoxFuture<'static, Result<(), ModelError>>> {
+        self.add_child_internal(component, child, collection, true).await
+    }
+
+    #[cfg(test)]
+    #[must_use]
+    pub async fn add_child_for_test(
+        &mut self,
+        component: &Arc<ComponentInstance>,
+        child: &ChildDecl,
+        collection: Option<&CollectionDecl>,
+        register_discover: bool,
+    ) -> Option<BoxFuture<'static, Result<(), ModelError>>> {
+        self.add_child_internal(component, child, collection, register_discover).await
+    }
+
+    #[must_use]
+    async fn add_child_internal(
+        &mut self,
+        component: &Arc<ComponentInstance>,
+        child: &ChildDecl,
+        collection: Option<&CollectionDecl>,
+        register_discover: bool,
+    ) -> Option<BoxFuture<'static, Result<(), ModelError>>> {
         let instance_id = match collection {
             Some(_) => {
                 let id = self.next_dynamic_instance_id;
@@ -1051,9 +1074,11 @@ impl ResolvedInstanceState {
             self.live_children.insert(partial_moniker, (instance_id, child.clone()));
             // We can dispatch a Discovered event for the component now that it's installed in the
             // tree, which means any Discovered hooks will capture it.
-            let nf = {
+            let nf = if register_discover {
                 let mut actions = child.lock_actions().await;
-                actions.register_no_wait(&child, DiscoverAction::new())
+                actions.register_no_wait(&child, DiscoverAction::new()).boxed()
+            } else {
+                async { Ok(()) }.boxed()
             };
             Some(nf)
         } else {