On Thu, 3 Sep 2026 00:14:24 GMT, Kevin Rushforth <[email protected]> wrote:
>> This PR fixes a bug in `Timer.stop()` on Windows that can happen if stop is >> called while a Timer callback is running. There are three related problems >> with the way the Timer is implemented on Windows: >> >> 1. The native timer kill operation is asynchronous: `timeSetEvent` does not >> include `TIME_KILL_SYNCHRONOUS` so `timeKillEvent` will not wait for a >> callback that might be executing concurrently. >> 2. The native `RunnableTimer::Stop()` method deletes the Timer object while >> it might still be in use running a callback. The deletion triggers a call to >> kill the timer from the Timer base class destructor, after the derived class >> destructor has run, meaning that the `runnable` member will be deleted >> before `timeKillEvent` is called. >> 3. The Java-side `Timer.pause()` method, which is a no-op on Windows, is >> synchronized on the Timer instance as is the `Timer.stop()` method. The >> `pause()` method is called from the Timer callback, which can lead to >> deadlock. >> >> This last problem might affect other platforms as well (e.g., it might >> contribute to a similar deadlock on macOS, tracked by >> [JDK-8238505](https://bugs.openjdk.org/browse/JDK-8238505)), but this fix is >> intentionally limited to Windows platform-specific code. >> >> The fix is as follows: >> >> 1. Pass `TIME_KILL_SYNCHRONOUS` to `timeSetEvent`. >> 2. Move the `timeKillEvent` call from the Timer destructor to an explicit >> `cancel()` method. The destructor calls this method defensively (which >> should be a no-op in the typical case). >> 3. Call `cancel()` from `Stop()`, checking the return code. Delete the timer >> after a successful cancel operation; throw an exception if the timer cannot >> be canceled. This should never occur, but if it does, it is safer to leak >> the Timer object than free an object that might still be in use. >> 4. WinTimer.java - Override `stop()` to call `super.stop()` in a try/catch, >> so an exception doesn't cause `Toolkit.exit()` to hang, and print the >> exception's stack trace if caught. >> 5. WinTimer.java - Override `pause()` and `resume()` to be unsynchronized >> no-op methods. >> >> I have included a test for this. To run just the new test: >> >> >> gradle sdk shims >> gradle --info -PTEST_ONLY=true -PFULL_TEST=true :systemTests:test --tests >> test.com.sun.glass.ui.WinTimerTest >> >> >> Without the fix, it will either crash (without generating an hs_err log) or >> fail with an AssertionError. With the fix, it passes. >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the [Ope... > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > assert that timer is/is not running as expected modules/javafx.graphics/src/main/native-glass/win/Timer.cpp line 52: > 50: RunnableTimer* runnableTimer = > (RunnableTimer*)jlong_to_ptr(timer); > 51: if (runnableTimer->cancel()) { > 52: delete runnableTimer; Deleting `runnableTimer` here frees memory that is still in use. Here is the problematic sequence: 1. `Timer::StaticTimeCallback` dereferences the `Timer*` stored in `dwUser` 2. `RunnableTimer::TimerCallback()` calls the Java `Runnable` 3. That Java runnable calls `timer.stop()` 4. **We are here now**: `RunnableTimer::Stop()` successfully cancels the timer and immediately executes `delete runnableTimer` 4. Control returns to the still-executing `RunnableTimer::TimerCallback()` 5. `TimerCallback()` then calls `CheckAndClearException(GetEnv())`, accessing the deleted C++ object ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2290#discussion_r3925070134
