From 0ec89cec40e49e5a2e5f3184f30288181951bbe3 Mon Sep 17 00:00:00 2001 From: floriankirmaier Date: Mon, 27 Apr 2026 22:51:09 +0200 Subject: [PATCH 1/2] Ensure, there are no leak for the stage of the application or devtoolsfx. --- .../devtoolsfx/connector/LocalConnector.java | 16 +- .../devtoolsfx/connector/WindowMonitor.java | 11 +- .../main/java/devtoolsfx/util/SceneUtils.java | 2 +- gui/pom.xml | 30 ++++ gui/src/main/java/devtoolsfx/gui/GUI.java | 14 +- .../main/java/devtoolsfx/gui/ToolPane.java | 60 +++++++- .../gui/InspectorMemoryLeakTest.java | 144 ++++++++++++++++++ 7 files changed, 256 insertions(+), 21 deletions(-) create mode 100644 gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java diff --git a/connector/src/main/java/devtoolsfx/connector/LocalConnector.java b/connector/src/main/java/devtoolsfx/connector/LocalConnector.java index 466cf51..10fbe56 100644 --- a/connector/src/main/java/devtoolsfx/connector/LocalConnector.java +++ b/connector/src/main/java/devtoolsfx/connector/LocalConnector.java @@ -11,6 +11,7 @@ import javafx.application.Application; import javafx.beans.property.ReadOnlyBooleanProperty; import javafx.beans.property.ReadOnlyBooleanWrapper; +import javafx.beans.value.ChangeListener; import javafx.collections.ListChangeListener; import javafx.scene.control.PopupControl; import javafx.stage.PopupWindow; @@ -43,6 +44,12 @@ public final class LocalConnector implements Connector { private final Map monitors = new HashMap<>(); private final ListChangeListener windowListChangeListener = this::onWindowListChanged; + private final ChangeListener inspectModeListener = (obs, old, val) -> { + if (!val) { + // prevents ConcurrentModificationException + new ArrayList<>(monitors.values()).forEach(monitor -> monitor.setInspectMode(false)); + } + }; private final ReadOnlyBooleanWrapper started = new ReadOnlyBooleanWrapper(); /** @@ -78,12 +85,7 @@ public LocalConnector(Stage primaryStage, monitors.put(uidOf(primaryStage), createMonitor(primaryStage, application, true)); - this.opts.inspectModeProperty().addListener((obs, old, val) -> { - if (!val) { - // prevents ConcurrentModificationException - new ArrayList<>(monitors.values()).forEach(monitor -> monitor.setInspectMode(false)); - } - }); + this.opts.inspectModeProperty().addListener(inspectModeListener); } @Override @@ -100,7 +102,9 @@ public void stop() { started.set(false); monitors.forEach((hash, monitor) -> monitor.stop()); + monitors.clear(); Window.getWindows().removeListener(windowListChangeListener); + opts.inspectModeProperty().removeListener(inspectModeListener); LOGGER.log(Level.INFO, "LocalConnector stopped"); } diff --git a/connector/src/main/java/devtoolsfx/connector/WindowMonitor.java b/connector/src/main/java/devtoolsfx/connector/WindowMonitor.java index ee3cd95..0cc35fb 100644 --- a/connector/src/main/java/devtoolsfx/connector/WindowMonitor.java +++ b/connector/src/main/java/devtoolsfx/connector/WindowMonitor.java @@ -45,6 +45,7 @@ final class WindowMonitor { private boolean started; private final Map stylesClassSubs = new HashMap<>(); + private final ChangeListener inspectModeListener = (obs, old, val) -> refreshRoot(); /** * Creates a new WindowMonitor instance. Monitors are not reusable; each instance must @@ -69,7 +70,7 @@ public WindowMonitor(Window window, this.attributeListener = new AttributeListener(eventBus, eventSource); - connectorOpts.inspectModeProperty().addListener((obs, old, val) -> refreshRoot()); + connectorOpts.inspectModeProperty().addListener(inspectModeListener); } /////////////////////////////////////////////////////////////////////////// @@ -97,8 +98,8 @@ public void start() { * Stops the monitor. */ public void stop() { - started = false; - + // changeScene/changeRoot are guarded by `started` and skip work when it's false, + // so we tear listeners down first and only flip the flag at the end. window.xProperty().removeListener(windowPropertyReportListener); window.yProperty().removeListener(windowPropertyReportListener); window.widthProperty().removeListener(windowPropertyReportListener); @@ -111,6 +112,10 @@ public void stop() { // cleanup resources clearSelection(); inspectPane.hide(); + + connectorOpts.inspectModeProperty().removeListener(inspectModeListener); + + started = false; } /** diff --git a/connector/src/main/java/devtoolsfx/util/SceneUtils.java b/connector/src/main/java/devtoolsfx/util/SceneUtils.java index b1b7070..9dc187d 100644 --- a/connector/src/main/java/devtoolsfx/util/SceneUtils.java +++ b/connector/src/main/java/devtoolsfx/util/SceneUtils.java @@ -317,7 +317,7 @@ public static void removeListener(@Nullable N node, Function> obs, ChangeListener listener) { if (node != null) { - obs.apply(node).addListener(listener); + obs.apply(node).removeListener(listener); } else { LOGGER.log(Level.INFO, "node is null, this behavior is probably not expected"); } diff --git a/gui/pom.xml b/gui/pom.xml index 339ef4c..fe91c73 100644 --- a/gui/pom.xml +++ b/gui/pom.xml @@ -21,6 +21,36 @@ devtoolsfx-connector ${project.version} + + + org.junit.jupiter + junit-jupiter-api + test + + + org.junit.jupiter + junit-jupiter-engine + test + + + one.jpro + JMemoryBuddy + 0.5.6 + test + + + + + org.apache.maven.plugins + maven-surefire-plugin + 3.2.5 + + false + + + + + diff --git a/gui/src/main/java/devtoolsfx/gui/GUI.java b/gui/src/main/java/devtoolsfx/gui/GUI.java index 58ce446..19ff73a 100644 --- a/gui/src/main/java/devtoolsfx/gui/GUI.java +++ b/gui/src/main/java/devtoolsfx/gui/GUI.java @@ -2,6 +2,7 @@ import devtoolsfx.connector.LocalConnector; import javafx.application.HostServices; +import javafx.event.EventHandler; import javafx.scene.Scene; import javafx.stage.Stage; import javafx.stage.WindowEvent; @@ -51,16 +52,23 @@ public static void openToolStage(Stage primaryStage, Objects.requireNonNull(primaryStage, "primaryStage can not be null"); Objects.requireNonNull(preferences, "hostServices can not be null"); - var toolPane = createToolPane(primaryStage, preferences, applicationName); + var connector = new LocalConnector(primaryStage, applicationName); + var toolPane = new ToolPane(connector, preferences); var scene = new Scene(toolPane, DEFAULT_STAGE_WIDTH, DEFAULT_STAGE_HEIGHT); scene.setUserAgentStylesheet(USER_AGENT_STYLESHEET); var toolStage = new Stage(); toolStage.setScene(scene); toolStage.setTitle("devtoolsfx"); - toolStage.setOnShown(e -> toolPane.getConnector().start()); + toolStage.setOnShown(e -> connector.start()); + + EventHandler closeWithPrimary = event -> toolStage.close(); + primaryStage.addEventHandler(WindowEvent.WINDOW_CLOSE_REQUEST, closeWithPrimary); + toolStage.setOnHidden(e -> { + primaryStage.removeEventHandler(WindowEvent.WINDOW_CLOSE_REQUEST, closeWithPrimary); + connector.stop(); + }); - primaryStage.addEventHandler(WindowEvent.WINDOW_CLOSE_REQUEST, event -> toolStage.close()); toolStage.show(); } diff --git a/gui/src/main/java/devtoolsfx/gui/ToolPane.java b/gui/src/main/java/devtoolsfx/gui/ToolPane.java index 7a302db..6b147e9 100644 --- a/gui/src/main/java/devtoolsfx/gui/ToolPane.java +++ b/gui/src/main/java/devtoolsfx/gui/ToolPane.java @@ -27,6 +27,9 @@ import javafx.scene.layout.StackPane; import javafx.stage.Window; import javafx.util.Duration; +import javafx.util.Subscription; + +import java.util.function.Consumer; import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; @@ -55,7 +58,11 @@ public final class ToolPane extends BorderPane { private final ChangeListener ignoreMouseTransparentListener; private final ChangeListener preventPopupAutoHideListener; + private final ChangeListener darkModeListener; private final Runnable refreshSelectionHandler; + private @Nullable Consumer eventBusSubscriber; + private @Nullable Subscription preferenceSubscriptions; + private @Nullable Timeline eventDispatcher; // tabs private final TabLine tabLine = new TabLine( @@ -89,6 +96,7 @@ public ToolPane(Connector connector, Preferences preferences) { ignoreMouseTransparentListener = (obs, old, val) -> connectorOpts.setIgnoreMouseTransparent(val); preventPopupAutoHideListener = (obs, old, val) -> connectorOpts.setPreventPopupAutoHide(val); + darkModeListener = (obs, old, val) -> toggleDarkMode(val); refreshSelectionHandler = () -> getConnector().refreshSelection(); createLayout(); @@ -98,6 +106,14 @@ public ToolPane(Connector connector, Preferences preferences) { tabLine.selectTab(InspectorTab.TAB_NAME); startListenToEvents(false); + + // Detach everything once the connector stops, so the pane (and the windows + // it transitively references) can be garbage-collected. + connector.startedProperty().subscribe(started -> { + if (!started) { + detachListeners(); + } + }); } /** @@ -135,7 +151,7 @@ public void start() { * See {@link Connector#stop()}}. */ public void stop() { - connector.start(); + connector.stop(); } /** @@ -241,6 +257,31 @@ public String getUserAgentStylesheet() { } } + /** + * Detaches every listener this pane installed on the user's {@link Preferences}, + * the connector's event bus, and the running {@link Timeline}, so the pane and + * the windows it transitively references can be garbage-collected. Triggered + * automatically when the connector stops. + */ + private void detachListeners() { + preferences.ignoreMouseTransparentProperty().removeListener(ignoreMouseTransparentListener); + preferences.preventPopupAutoHideProperty().removeListener(preventPopupAutoHideListener); + preferences.darkModeProperty().removeListener(darkModeListener); + if (preferenceSubscriptions != null) { + preferenceSubscriptions.unsubscribe(); + preferenceSubscriptions = null; + } + if (eventBusSubscriber != null) { + connector.getEventBus().unsubscribe(eventBusSubscriber); + eventBusSubscriber = null; + } + if (eventDispatcher != null) { + eventDispatcher.stop(); + eventDispatcher = null; + } + eventQueue.clear(); + } + /** * Handles the GUI exceptions. *

@@ -288,11 +329,13 @@ private void initListeners() { preferences.preventPopupAutoHideProperty().addListener(preventPopupAutoHideListener); connectorOpts.setPreventPopupAutoHide(preferences.isPreventPopupAutoHide()); - preferences.showLayoutBoundsProperty().subscribe(refreshSelectionHandler); - preferences.showBoundsInParentProperty().subscribe(refreshSelectionHandler); - preferences.showBaselineProperty().subscribe(refreshSelectionHandler); + preferenceSubscriptions = Subscription.combine( + preferences.showLayoutBoundsProperty().subscribe(refreshSelectionHandler), + preferences.showBoundsInParentProperty().subscribe(refreshSelectionHandler), + preferences.showBaselineProperty().subscribe(refreshSelectionHandler) + ); - preferences.darkModeProperty().addListener((obs, old, val) -> toggleDarkMode(val)); + preferences.darkModeProperty().addListener(darkModeListener); tabLine.setOnTabSelect(tab -> { switch (tab) { @@ -326,7 +369,7 @@ private void startListenToEvents(boolean useQueue) { if (useQueue) { // Optionally, we can update the GUI on a separate queue, which adds a small delay. // This is how it was implemented before and was left as an option. - Timeline eventDispatcher = new Timeline(new KeyFrame(Duration.millis(60), event -> { + eventDispatcher = new Timeline(new KeyFrame(Duration.millis(60), event -> { // no need to synchronize while (!eventQueue.isEmpty()) { try { @@ -341,7 +384,7 @@ private void startListenToEvents(boolean useQueue) { eventDispatcher.play(); } - connector.getEventBus().subscribe(ConnectorEvent.class, event -> { + eventBusSubscriber = event -> { if (event instanceof MousePosEvent) { // traffic protection if (System.currentTimeMillis() - lastMousePos < 500) { @@ -355,7 +398,8 @@ private void startListenToEvents(boolean useQueue) { } else { dispatchEvent(event); } - }); + }; + connector.getEventBus().subscribe(ConnectorEvent.class, eventBusSubscriber); } private void dispatchEvent(ConnectorEvent connectorEvent) { diff --git a/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java b/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java new file mode 100644 index 0000000..260591c --- /dev/null +++ b/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java @@ -0,0 +1,144 @@ +package devtoolsfx.gui; + +import one.jpro.jmemorybuddy.JMemoryBuddy; +import javafx.application.Application; +import javafx.application.HostServices; +import javafx.application.Platform; +import javafx.event.Event; +import javafx.scene.Scene; +import javafx.scene.control.Button; +import javafx.scene.layout.StackPane; +import javafx.stage.Stage; +import javafx.stage.Window; +import javafx.stage.WindowEvent; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + +public class InspectorMemoryLeakTest { + + @BeforeAll + static void initFx() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + Platform.startup(latch::countDown); + latch.await(30, TimeUnit.SECONDS); + Platform.setImplicitExit(false); + } + + @Test + void baselineStageWithoutInspectorIsCollectable() { + JMemoryBuddy.memoryTest(checker -> { + Stage stage = onFx(() -> { + Stage s = new Stage(); + s.setScene(new Scene(new StackPane(new Button("Hello")), 200, 200)); + s.show(); + return s; + }); + onFx(stage::close); + + checker.assertCollectable(stage); + }); + } + + @Test + void closingBothStagesReleasesEverything() { + JMemoryBuddy.memoryTest(checker -> { + Stages stages = openInspector(); + onFx(() -> { + stages.tool.close(); + stages.app.close(); + }); + checker.assertCollectable(stages.app); + checker.assertCollectable(stages.tool); + }); + } + + @Test + void closingOnlyTheInspectorStageThenAppReleasesEverything() { + JMemoryBuddy.memoryTest(checker -> { + Stages stages = openInspector(); + onFx(stages.tool::close); + onFx(stages.app::close); + checker.assertCollectable(stages.app); + checker.assertCollectable(stages.tool); + }); + } + + @Test + void closingTheAppViaCloseRequestReleasesEverything() { + JMemoryBuddy.memoryTest(checker -> { + Stages stages = openInspector(); + onFx(() -> { + // Simulate the user clicking the X on the app window: GUI.openToolStage + // hooks WINDOW_CLOSE_REQUEST on the primary stage to also close the tool stage. + Event.fireEvent(stages.app, new WindowEvent(stages.app, WindowEvent.WINDOW_CLOSE_REQUEST)); + stages.app.close(); + }); + checker.assertCollectable(stages.app); + checker.assertCollectable(stages.tool); + }); + } + + private record Stages(Stage app, Stage tool) {} + + private static Stages openInspector() { + Stage appStage = onFx(() -> { + Stage s = new Stage(); + s.setScene(new Scene(new StackPane(new Button("Hello")), 400, 300)); + s.setTitle("App"); + s.show(); + GUI.openToolStage(s, dummyApp().getHostServices()); + return s; + }); + Stage toolStage = onFx(() -> Window.getWindows().stream() + .filter(w -> w instanceof Stage st && "devtoolsfx".equals(st.getTitle())) + .map(Stage.class::cast) + .findFirst() + .orElseThrow()); + return new Stages(appStage, toolStage); + } + + private static Application dummyApp() { + return new Application() { + @Override + public void start(Stage primaryStage) { + } + }; + } + + private static T onFx(Supplier s) { + AtomicReference result = new AtomicReference<>(); + onFx(() -> result.set(s.get())); + return result.get(); + } + + private static void onFx(Runnable r) { + if (Platform.isFxApplicationThread()) { + r.run(); + return; + } + CountDownLatch latch = new CountDownLatch(1); + Throwable[] err = new Throwable[1]; + Platform.runLater(() -> { + try { + r.run(); + } catch (Throwable t) { + err[0] = t; + } finally { + latch.countDown(); + } + }); + try { + latch.await(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + if (err[0] != null) { + throw new RuntimeException(err[0]); + } + } +} From d40350f638842cd5b321ef9aae729691a7f94b86 Mon Sep 17 00:00:00 2001 From: floriankirmaier Date: Mon, 27 Apr 2026 23:44:00 +0200 Subject: [PATCH 2/2] Fixing regression regarding the Connector, and adding another unit-test. --- .../java/devtoolsfx/connector/Connector.java | 7 +++++++ .../devtoolsfx/connector/LocalConnector.java | 7 +++++++ gui/src/main/java/devtoolsfx/gui/ToolPane.java | 4 ++-- .../gui/InspectorMemoryLeakTest.java | 18 ++++++++++++++++++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/connector/src/main/java/devtoolsfx/connector/Connector.java b/connector/src/main/java/devtoolsfx/connector/Connector.java index 7ba81c0..1311d70 100644 --- a/connector/src/main/java/devtoolsfx/connector/Connector.java +++ b/connector/src/main/java/devtoolsfx/connector/Connector.java @@ -38,6 +38,13 @@ public interface Connector { */ ReadOnlyBooleanProperty startedProperty(); + /** + * Becomes {@code true} once {@link #stop()} has been called and stays so for the + * rest of the connector's life. Useful as a one-shot trigger for resource cleanup, + * since {@link #startedProperty()} is also {@code false} before the first start. + */ + ReadOnlyBooleanProperty stoppedProperty(); + /** * Returns the {@link EventBus} to react to the connector events. */ diff --git a/connector/src/main/java/devtoolsfx/connector/LocalConnector.java b/connector/src/main/java/devtoolsfx/connector/LocalConnector.java index 10fbe56..92c859a 100644 --- a/connector/src/main/java/devtoolsfx/connector/LocalConnector.java +++ b/connector/src/main/java/devtoolsfx/connector/LocalConnector.java @@ -51,6 +51,7 @@ public final class LocalConnector implements Connector { } }; private final ReadOnlyBooleanWrapper started = new ReadOnlyBooleanWrapper(); + private final ReadOnlyBooleanWrapper stopped = new ReadOnlyBooleanWrapper(); /** * See {@link LocalConnector#(Stage, ConnectorOptions , String)}. @@ -105,6 +106,7 @@ public void stop() { monitors.clear(); Window.getWindows().removeListener(windowListChangeListener); opts.inspectModeProperty().removeListener(inspectModeListener); + stopped.set(true); LOGGER.log(Level.INFO, "LocalConnector stopped"); } @@ -113,6 +115,11 @@ public ReadOnlyBooleanProperty startedProperty() { return started.getReadOnlyProperty(); } + @Override + public ReadOnlyBooleanProperty stoppedProperty() { + return stopped.getReadOnlyProperty(); + } + @Override public EventBus getEventBus() { return eventBus; diff --git a/gui/src/main/java/devtoolsfx/gui/ToolPane.java b/gui/src/main/java/devtoolsfx/gui/ToolPane.java index 6b147e9..1a0e0a9 100644 --- a/gui/src/main/java/devtoolsfx/gui/ToolPane.java +++ b/gui/src/main/java/devtoolsfx/gui/ToolPane.java @@ -109,8 +109,8 @@ public ToolPane(Connector connector, Preferences preferences) { // Detach everything once the connector stops, so the pane (and the windows // it transitively references) can be garbage-collected. - connector.startedProperty().subscribe(started -> { - if (!started) { + connector.stoppedProperty().subscribe(stopped -> { + if (stopped) { detachListeners(); } }); diff --git a/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java b/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java index 260591c..d69612f 100644 --- a/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java +++ b/gui/src/test/java/devtoolsfx/gui/InspectorMemoryLeakTest.java @@ -68,6 +68,24 @@ void closingOnlyTheInspectorStageThenAppReleasesEverything() { }); } + @Test + void inspectorEventSubscriberIsWiredWhileRunning() throws Exception { + java.lang.reflect.Field subscriberField = ToolPane.class.getDeclaredField("eventBusSubscriber"); + subscriberField.setAccessible(true); + + Stages stages = openInspector(); + ToolPane toolPane = onFx(() -> (ToolPane) stages.tool.getScene().getRoot()); + if (subscriberField.get(toolPane) == null) { + throw new AssertionError( + "ToolPane.eventBusSubscriber is null while the inspector is running -- " + + "the inspector receives no scene-graph events."); + } + onFx(() -> { + stages.tool.close(); + stages.app.close(); + }); + } + @Test void closingTheAppViaCloseRequestReleasesEverything() { JMemoryBuddy.memoryTest(checker -> {