> 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 [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai).
Kevin Rushforth has updated the pull request incrementally with one additional commit since the last revision: rm extra newlines ------------- Changes: - all: https://git.openjdk.org/jfx/pull/2290/files - new: https://git.openjdk.org/jfx/pull/2290/files/39ff7fae..97fae627 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=2290&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=2290&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/2290.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/2290/head:pull/2290 PR: https://git.openjdk.org/jfx/pull/2290
