On Thu, 3 Sep 2026 13:31:43 GMT, Michael Strauß <[email protected]> wrote:
>> 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
>
> This could be solved by keeping a reference count on `RunnableTimer`:
> ```c++
> static void CALLBACK StaticTimeCallback(..., DWORD_PTR dwUser, ...)
> {
> auto* timer = reinterpret_cast<RunnableTimer*>(dwUser);
>
> timer->retain(); // callback owns one reference
> timer->TimerCallback();
> timer->release(); // may destroy only after TimerCallback returned
> }
>
>
> The timer would start out with an initial owned reference. When `cancel()`
> succeeds, `Stop()` then releases that owner reference instead of calling
> `delete`. That produces the correct behavior:
>
> * With no callback running, cancellation releases the final reference and
> destroys immediately.
> * With an external callback running, synchronous cancellation waits;
> destruction follows normally.
> * With self-cancellation, the owner reference is dropped, but the callback
> reference keeps the object alive until the callback has returned.
> * With overlapping callbacks, an atomic reference count keeps the context
> alive until the last callback returns.
This scenario never happens given how QuantumToolkit uses the Timer.
Specifically:
> 3. That Java runnable calls timer.stop()
If we were to do this, then yes, we would have a problem. However, the only two
places that call `timer.stop()` in production code are in QuantumToolkit and
are not in a timer callback.
I'd rather handle this in a follow-up fix, likely by explicitly disallowing it.
There is no reason in the current design to need to support calling either
`Timer.start()` or `Timer.stop()` from a timer callback runnable. Similarly,
the advice to call invokeAndWait() should be removed -- that's asking for a
deadlock.
There are other items to address in a follow-up fix as well. I'll reply to your
latest comment some time this afternoon (Pacific time) with my current list.
There may be others.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2290#discussion_r3926005214
