On Fri, 10 Jul 2026 06:26:04 GMT, Johan Vos <[email protected]> wrote:
>> As it is before this PR: >> >> When `stage.setPlatformWindowClosed()` is called, it clears >> `WindowStage::platformWindow`, >> `stage.stageListener.closed()` ends up calling >> `WindowStage::handleFocusDisabled`, which takes the last windowStage in the >> `activeWindows` list, which is not null, but its platformWindow is, causing >> the NPE. >> >> With my proposed fix: >> `stage.setPlatformWindowClosed()` -> clears `WindowStage::platformWindow`, >> `WindowStage.removeActiveWindow(stage)` -> removes windowStage from the >> `activeWindows` list, >> `stage.stageListener.closed()` -> `WindowStage::handleFocusDisabled` bails >> out, no active windows, so no NPE. >> >> With your suggestion: >> `stage.stageListener.closed()` -> `WindowStage::handleFocusDisabled` which >> takes the last windowStage, which still has a valid `platformWindow`, so no >> NPE, and then `setVisible(false)` removes windowStage from the >> `activeWindows` list, >> `stage.setPlatformWindowClosed()` -> does nothing, the platformWindow was >> already cleared, so this call seems redundant. >> >> In both cases the NPE is fixed, but it seems that this will do it with less >> calls: >> >> case WindowEvent.DESTROY: >> stage.stageListener.closed(); >> >> >> Edit: However, having a closer look, it seems that >> `setPlatformWindowClosed()` is needed in the first place after all: >> `Window.notifyDestroy()` emits `WindowEvent.DESTROY` before it sets `ptr = >> 0`, so >> if `stage.stageListener.closed()` runs first, it can call `window.hide()` -> >> `peer.close()` -> `WindowStage.close()` -> `platformWindow.close()` while >> the Glass window is still not destroyed, possibly causing a second native >> close attempt. Therefore the comment: >> >>> // ... This state is necessary to prevent the platform >>> // window from being closed more than once. >> >> And it is also needed when `stage.stageListener.closed()` bails out if >> `!window.isShowing()`: without setPlatformWindowClosed(), `platformWindow` >> won't be cleared (possible memory leak). >> >> So I'd say we should keep the order of calls in the current proposal. > >> // ... This state is necessary to prevent the platform >> // window from being closed more than once. > > What flow is expected when the window if being closed more than once? I > assume it is always initiated from the native window system? In that case, it > will happen on the FX Thread, and it will not be started until the previous > notification listener code has finished. Everything happens on the FX thread, so the concern isn't about multi-threading. Rather it is that we can get multiple close events, I think there are also multiple things that can trigger it (e.g., when an owner window is closed, all the child windows get closed). In any case, Jose's fix doesn't change anything related to this. We already have defensive calls in place to avoid problems if close is called more than once. The current fix is a good fix, regardless, given what `removeActiveWindow()` does. Also, I think it is the least intrusive fix for the problem. It would have been _way_ more risky to swap the order of the two existing calls (and, as Jose mentioned, it doesn't work to just eliminate one of them). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2202#discussion_r3590667102
