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 A follow-up fix that would address the lock-order problem at its core, rather than special-casing the callback method, would be to give the Java timer an explicit lifecycle: STOPPED-> RUNNING-> STOPPING-> STOPPED. `stop()` should then: 1. acquire the monitor 2. mark the timer `STOPPING` and snapshot the native pointer 3. release the monitor 4. perform the potentially blocking native cancellation 5. reacquire the monitor and report either success or failure. ------------- PR Comment: https://git.openjdk.org/jfx/pull/2290#issuecomment-5527021416
