On Thu, 3 Sep 2026 15:23:56 GMT, Kevin Rushforth <[email protected]> wrote:
>> 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.
I just now got back to this. I added a general comment with the list of things
I want to address in a follow-up issue. I plan to file it in the next day or so.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2290#discussion_r3962915975
