IJPL-247543 dispose stranded child and its subtree in Disposer when parent is already disposed

When a service constructor calls Disposer.register(this, someInner) — the common
pattern for services that build a MergingUpdateQueue, Alarm, or similar helper in
their init — the service ends up in the Disposer tree as a child of the tree root
before ServiceInstanceInitializer runs Disposer.register(serviceParentDisposable,
instance). If serviceParentDisposable is disposed concurrently in that window, the
register call throws IncorrectOperationException and leaves the service and its
constructor-registered subtree stranded as a root-orphan, keeping the Project (and
whatever else the constructor captured) alive.

The previous attempt (280a0fdb reverted b27dd3223) required callers to switch from
Disposer.register to Disposer.tryRegister + explicit Disposer.dispose. This take
moves the cleanup into ObjectTree itself: register delegates to tryRegister, and
tryRegister's "parent already disposed" branch calls collectStrandedChild to
detach the child (plus any subtree the constructor attached) and stage them for
disposal. beforeTreeDispose + dispose then run OUTSIDE the tree lock via a new
disposeStrandedList helper, mirroring runWithTrace's collect-under-lock,
dispose-after pattern so dispose() bodies can safely acquire other locks or
perform I/O (e.g. CratesLocalIndexServiceImpl closing its PersistentHashMap,
RunningEmulatorCatalog shutting down emulators, DisposeAndRefreshService
terminating its thread pool).

CheckedDisposable entries in the stranded set have their isDisposed flag set
inside the tree lock so isDisposed() reflects the truth immediately, before the
deferred dispose() runs. runWithTrace is refactored to delegate to the same
disposeStrandedList helper, removing a duplicated 12-line beforeTreeDispose +
dispose block.

Also in this commit:
- assertNoReferenceKeptInTree overloads now walk from myRootNode so root-level
  orphans (Disposables that sit as direct children of the tree root without any
  tracked descendants) are covered by the assertion.
- collectStrandedChild and the second (post-registration) failure branch now
  preserve a debug-mode Throwable trace, so Disposer.getDisposalTrace(strandedChild)
  points to the register() call site instead of returning null.
- Disposer.register Javadoc documents that on failure the child's subtree is
  detached and disposed before the exception propagates — callers should not
  follow up with Disposer.dispose(child).

Regression tests in ServiceContainerDisposalRaceTest exercise three scenarios:
after-disposal getService, concurrent disposal via Disposable.Parent blocker,
and a service whose constructor pre-registers a child in the Disposer tree.

Signed-off-by: Dmitry Batkovich <dmitry.batkovich@jetbrains.com>

GitOrigin-RevId: 0b99ab6c808d2ef64fb7b2f93de7507638558dc8

platform/service-container/testSrc/com/intellij/serviceContainer/ServiceContainerDisposalRaceTest.kt

@@ -17,15 +17,20 @@ import java.util.function.Predicate
  * a `Disposable` service must not stay reachable through `Disposer.getTree()` if the container's
  * `serviceParentDisposable` is disposed concurrently with service registration.
  *
- * The fix relies on:
- *  - `ComponentManagerImpl.serviceParentDisposable` being a `CheckedDisposable`
- *    (so `ObjectTree.executeAll` marks it disposed inside the tree lock); and
- *  - `ServiceInstanceInitializer.createInstance` using `Disposer.tryRegister` and
- *    disposing the just-created instance when registration fails.
+ * The fix lives in `ObjectTree.tryRegister` (which `ObjectTree.register` now delegates to):
+ *  - `ObjectTree.executeAll` populates `myDisposedObjects[serviceParentDisposable]` inside the
+ *    tree lock, so the `isDisposed(parent)` check at the top of `tryRegister` sees the truth
+ *    without any visibility race — even though `serviceParentDisposable` is a plain `Disposable`,
+ *    not a `CheckedDisposable`.
+ *  - On that branch, `collectStrandedChild(child)` detaches the child (and any subtree the
+ *    child's constructor registered under itself via `Disposer.register(this, ...)`) from the
+ *    tree, marks it disposed, and stages it so `dispose()` is invoked after the tree lock is
+ *    released (mirroring `runWithTrace`'s "collect under lock, dispose after" pattern).
  */
 class ServiceContainerDisposalRaceTest {
   private val pluginDescriptor = DefaultPluginDescriptor("service-container-disposal-race-test")
   private val noLeakedRaceService: Predicate<Any> = Predicate { it is RaceTestService }
+  private val noLeakedServiceOrChild: Predicate<Any> = Predicate { it is ServiceWithPreRegisteredChild || it is ServiceChild }
 
   @Test
   fun `getService after serviceParentDisposable is disposed throws and does not leak`() {
@@ -90,8 +95,73 @@ class ServiceContainerDisposalRaceTest {
       assertThat(executor.awaitTermination(10, TimeUnit.SECONDS)).isTrue
     }
   }
+
+  @Test
+  fun `service whose constructor registers a child in Disposer tree must not leak when parent disposed concurrently`() {
+    val executor = ConcurrencyUtil.newSingleThreadExecutor("ServiceContainerDisposalRaceTest")
+    try {
+      repeat(20) { iteration ->
+        val componentManager = TestComponentManager()
+        componentManager.registerService(
+          ServiceWithPreRegisteredChild::class.java,
+          ServiceWithPreRegisteredChild::class.java,
+          pluginDescriptor,
+          false,
+        )
+
+        val inBeforeTreeDispose = CountDownLatch(1)
+        val proceedDispose = CountDownLatch(1)
+
+        val blocker = object : Disposable.Parent {
+          override fun beforeTreeDispose() {
+            inBeforeTreeDispose.countDown()
+            assertThat(proceedDispose.await(10, TimeUnit.SECONDS))
+              .`as`("Blocker should be unblocked within timeout (iteration=$iteration)")
+              .isTrue
+          }
+
+          override fun dispose() {}
+        }
+        Disposer.register(componentManager.serviceParentDisposable, blocker)
+
+        val disposeFuture = executor.submit {
+          Disposer.dispose(componentManager.serviceParentDisposable)
+        }
+
+        assertThat(inBeforeTreeDispose.await(10, TimeUnit.SECONDS))
+          .`as`("Disposing thread should reach beforeTreeDispose within timeout (iteration=$iteration)")
+          .isTrue
+
+        assertThrows<Throwable>("getService should fail because serviceParentDisposable is already marked disposed (iteration=$iteration)") {
+          componentManager.getService(ServiceWithPreRegisteredChild::class.java)
+        }
+
+        proceedDispose.countDown()
+        disposeFuture.get(10, TimeUnit.SECONDS)
+
+        Disposer.getTree().assertNoReferenceKeptInTree(noLeakedServiceOrChild)
+      }
+    }
+    finally {
+      executor.shutdown()
+      assertThat(executor.awaitTermination(10, TimeUnit.SECONDS)).isTrue
+    }
+  }
 }
 
 private class RaceTestService : Disposable {
   override fun dispose() {}
 }
+
+private class ServiceChild : Disposable {
+  override fun dispose() {}
+}
+
+private class ServiceWithPreRegisteredChild : Disposable {
+  init {
+    @Suppress("LeakingThis")
+    Disposer.register(this, ServiceChild())
+  }
+
+  override fun dispose() {}
+}

platform/util/src/com/intellij/openapi/util/Disposer.java

@@ -160,6 +160,11 @@ public final class Disposer {
    * Registers {@code child} so it is disposed right before its {@code parent}. See {@link Disposer class JavaDoc} for more details.
    * This method overrides parent disposable for the {@code child}, i.e., if {@code child} is already registered with {@code oldParent},
    * then it's unregistered from {@code oldParent} before registering with {@code parent}.
+   * <p>
+   * If {@code parent} is already disposed, {@link IncorrectOperationException} is thrown. Before throwing, any subtree that
+   * {@code child}'s constructor may have attached to the disposer tree (e.g. via {@code Disposer.register(this, someInner)}) is
+   * detached and its {@code dispose()} is invoked, so callers do not need to (and should not) call {@code Disposer.dispose(child)}
+   * afterwards.
    *
    * @throws IncorrectOperationException If {@code child} has been registered with {@code parent} before;
    *                                     if {@code parent} is being disposed or already disposed, see {@link #isDisposed(Disposable)}.

platform/util/src/com/intellij/openapi/util/ObjectTree.java

@@ -36,25 +36,11 @@ public final class ObjectTree {
     if (parent == child) {
       throw new IllegalArgumentException("Cannot register to itself: "+parent);
     }
-    synchronized (getTreeLock()) {
-      if (isDisposed(parent)) {
-        throw new IncorrectOperationException("Sorry but parent: " + parent + " (" + parent.getClass()+") has already been disposed " +
-                                              "(see the cause for stacktrace) so the child: "+child+ " (" + child.getClass()+") will never be disposed",
-                                              getDisposalTrace(parent));
-      }
-
-      myDisposedObjects.remove(child);
-      if (child instanceof Disposer.CheckedDisposableImpl) {
-        // if we dispose a child and then register it back, it means it's not disposed anymore
-        ((Disposer.CheckedDisposableImpl)child).isDisposed = false;
-      }
-
-      ObjectNode parentNode = getParentNode(parent).findOrCreateChildNode(parent);
-      ObjectNode childNode = getParentNode(child).moveChildNodeToOtherParent(child, parentNode);
-      myObject2ParentNode.put(child, parentNode);
-
-      assert childNode.getObject() == child;
-      checkWasNotAddedAlreadyAsChild(parentNode, childNode);
+    if (!tryRegister(parent, child)) {
+      throw new IncorrectOperationException(
+        "Sorry but parent: " + parent + " (" + parent.getClass() + ") has already been disposed " +
+        "(see the cause for stacktrace) so the child: " + child + " (" + child.getClass() + ") will never be disposed",
+        getDisposalTrace(parent));
     }
   }
 
@@ -63,39 +49,141 @@ public final class ObjectTree {
   }
 
   boolean tryRegister(@NotNull Disposable parent, @NotNull Disposable child) {
+    List<Disposable> toDispose = null;
+    boolean success = false;
     synchronized (getTreeLock()) {
       if (isDisposed(parent)) {
-        return false;
+        // must be called inside the tree lock
+        toDispose = collectStrandedChild(child);
       }
-      if (parent == child) {
+      else if (parent == child) {
         throw new IllegalArgumentException("Cannot register to itself: " + parent);
       }
-
-      myDisposedObjects.remove(child);
-      if (child instanceof Disposer.CheckedDisposableImpl) {
-        // if we dispose a child and then register it back, it means it's not disposed anymore
-        ((Disposer.CheckedDisposableImpl)child).isDisposed = false;
-      }
-
-      ObjectNode parentNode = getParentNode(parent).findOrCreateChildNode(parent);
-      ObjectNode childNode = getParentNode(child).moveChildNodeToOtherParent(child, parentNode);
-      myObject2ParentNode.put(child, parentNode);
-
-      assert childNode.getObject() == child;
-      checkWasNotAddedAlreadyAsChild(parentNode, childNode);
-
-      // parent could be disposed outside our lock while we are messing with the pointers here - undo registering in this case
-      if (isDisposed(parent)) {
-        List<Disposable> disposables = new ArrayList<>();
-        childNode.removeChildNodesRecursively(disposables, this, null, null);
-        for (Disposable disposable : disposables) {
-          myObject2ParentNode.remove(disposable);
+      else {
+        myDisposedObjects.remove(child);
+        if (child instanceof Disposer.CheckedDisposableImpl) {
+          // if we dispose a child and then register it back, it means it's not disposed anymore
+          ((Disposer.CheckedDisposableImpl)child).isDisposed = false;
         }
-        parentNode.removeChildNode(childNode);
-        return false;
-      }
 
-      return true;
+        // main parent -> child wire block
+        ObjectNode grandparentNode = getParentNode(parent);
+        boolean parentNodeWasNew = grandparentNode.findChildNode(parent) == null;
+        ObjectNode parentNode = grandparentNode.findOrCreateChildNode(parent);
+        ObjectNode oldChildParent = getParentNode(child);
+        boolean childNodeWasNew = oldChildParent.findChildNode(child) == null;
+        ObjectNode childNode = oldChildParent.moveChildNodeToOtherParent(child, parentNode);
+        myObject2ParentNode.put(child, parentNode);
+
+        assert childNode.getObject() == child;
+        checkWasNotAddedAlreadyAsChild(parentNode, childNode);
+
+        // parent could be disposed outside our lock while we are messing with the pointers here - undo registering in this case
+        if (isDisposed(parent)) {
+          Throwable trace = Disposer.isDebugMode() ? ThrowableInterner.intern(new Throwable()) : null;
+          toDispose = new ArrayList<>();
+          childNode.removeChildNodesRecursively(toDispose, this, trace, null);
+          for (Disposable disposable : toDispose) {
+            myObject2ParentNode.remove(disposable);
+          }
+          if (childNodeWasNew) {
+            parentNode.removeChildNode(childNode);
+            myObject2ParentNode.remove(child);
+            if (rememberDisposedTrace(child, trace) == null) {
+              toDispose.add(child);
+            }
+          }
+          else {
+            parentNode.moveChildNodeToOtherParent(child, oldChildParent);
+            if (oldChildParent == myRootNode) {
+              // Root-level disposables aren't tracked in myObject2ParentNode.
+              myObject2ParentNode.remove(child);
+            }
+            else {
+              myObject2ParentNode.put(child, oldChildParent);
+            }
+          }
+
+          if (parentNodeWasNew) {
+            grandparentNode.removeChildNode(parentNode);
+          }
+
+          // mark as disposed inside the tree lock
+          for (Disposable disposable : toDispose) {
+            if (disposable instanceof Disposer.CheckedDisposableImpl) {
+              ((Disposer.CheckedDisposableImpl)disposable).isDisposed = true;
+            }
+          }
+        }
+        else {
+          success = true;
+        }
+      }
+    }
+    // must be called outside tree lock
+    if (toDispose != null && !toDispose.isEmpty()) {
+      disposeStrandedList(toDispose);
+    }
+    return success;
+  }
+
+
+  // Must be called with tree lock held.
+  private @NotNull List<Disposable> collectStrandedChild(@NotNull Disposable child) {
+    ObjectNode currentParent = getParentNode(child);
+    ObjectNode childNode = currentParent.findChildNode(child);
+    if (childNode == null) {
+      // child never made it into the tree - nothing to clean up.
+      return Collections.emptyList();
+    }
+    Throwable trace = Disposer.isDebugMode() ? ThrowableInterner.intern(new Throwable()) : null;
+    List<Disposable> collected = new ArrayList<>();
+    childNode.removeChildNodesRecursively(collected, this, trace, null);
+    currentParent.removeChildNode(childNode);
+    for (Disposable descendant : collected) {
+      myObject2ParentNode.remove(descendant);
+    }
+    myObject2ParentNode.remove(child);
+    if (rememberDisposedTrace(child, trace) == null) {
+      collected.add(child);
+    }
+    for (Disposable disposable : collected) {
+      if (disposable instanceof Disposer.CheckedDisposableImpl) {
+        ((Disposer.CheckedDisposableImpl)disposable).isDisposed = true;
+      }
+    }
+    return collected;
+  }
+
+   // Must be called outside tree lock.
+  private static void disposeStrandedList(@NotNull List<Disposable> toDispose) {
+    List<Throwable> exceptions = null;
+    // beforeTreeDispose in pre-order - some clients are hardcoded to see parents-then-children order
+    for (int i = toDispose.size() - 1; i >= 0; i--) {
+      Disposable disposable = toDispose.get(i);
+      if (disposable instanceof Disposable.Parent) {
+        try {
+          ((Disposable.Parent)disposable).beforeTreeDispose();
+        }
+        catch (Throwable t) {
+          if (exceptions == null) exceptions = new SmartList<>();
+          exceptions.add(t);
+        }
+      }
+    }
+    // dispose in post-order (bottom-up)
+    for (Disposable disposable : toDispose) {
+      try {
+        //noinspection SSBasedInspection
+        disposable.dispose();
+      }
+      catch (Throwable e) {
+        if (exceptions == null) exceptions = new SmartList<>();
+        exceptions.add(e);
+      }
+    }
+    if (exceptions != null) {
+      handleExceptions(exceptions);
     }
   }
 
@@ -136,37 +224,14 @@ public final class ObjectTree {
     synchronized (getTreeLock()) {
       disposables = removeFromTreeAction.apply(this, trace);
     }
-    // second, call "beforeTreeDispose" in pre-order (some clients are hardcoded to see parents-then-children order in "beforeTreeDispose")
-    List<Throwable> exceptions = null;
-    for (int i = disposables.size() - 1; i >= 0; i--) {
-      Disposable disposable = disposables.get(i);
-      if (disposable instanceof Disposable.Parent) {
-        try {
-          ((Disposable.Parent)disposable).beforeTreeDispose();
-        }
-        catch (Throwable t) {
-          if (exceptions == null) exceptions = new SmartList<>();
-          exceptions.add(t);
-        }
-      }
+    try {
+      // second/third: beforeTreeDispose in pre-order, dispose in post-order (bottom-up).
+      disposeStrandedList(disposables);
     }
-
-    // third, dispose in post-order (bottom-up)
-    for (Disposable disposable: disposables) {
-      try {
-        //noinspection SSBasedInspection
-        disposable.dispose();
+    finally {
+      if (needTrace) {
+        ourTopmostDisposeTrace.remove();
       }
-      catch (Throwable e) {
-        if (exceptions == null) exceptions = new SmartList<>();
-        exceptions.add(e);
-      }
-    }
-    if (needTrace) {
-      ourTopmostDisposeTrace.remove();
-    }
-    if (exceptions != null) {
-      handleExceptions(exceptions);
     }
   }
 
@@ -251,18 +316,16 @@ public final class ObjectTree {
   @TestOnly
   public void assertNoReferenceKeptInTree(@NotNull Disposable disposable) {
     synchronized (getTreeLock()) {
-      for (ObjectNode node : myObject2ParentNode.values()) {
-        node.assertNoReferencesKept(disposable);
-      }
+      // Walk from myRootNode so root-level orphans (a Disposable that lives as a direct child
+      // of the tree root without any tracked descendants in myObject2ParentNode) are also covered.
+      myRootNode.assertNoReferencesKept(disposable);
     }
   }
 
   @TestOnly
   public void assertNoReferenceKeptInTree(@NotNull Class<Disposable> disposableClass) {
     synchronized (getTreeLock()) {
-      for (ObjectNode node : myObject2ParentNode.values()) {
-        node.assertNoReferencesKept(disposableClass);
-      }
+      myRootNode.assertNoReferencesKept(disposableClass);
     }
   }
 
@@ -275,9 +338,7 @@ public final class ObjectTree {
   @TestOnly
   public void assertNoReferenceKeptInTree(@NotNull Predicate<Object> predicate) {
     synchronized (getTreeLock()) {
-      for (ObjectNode node : myObject2ParentNode.values()) {
-        node.assertNoReferencesKept(predicate);
-      }
+      myRootNode.assertNoReferencesKept(predicate);
     }
   }