On Fri, 15 Dec 2023 20:57:01 GMT, Martin Fox <[email protected]> wrote:
>> While processing a key down event the Glass GTK code sends out PRESSED and
>> TYPED KeyEvents back to back. If the stage is closed during the PRESSED
>> event the code will end up referencing freed memory while sending out the
>> TYPED event. This can lead to intermittent crashes.
>>
>> In GlassApplication.cpp the EventCounterHelper object ensures the
>> WindowContext isn't deleted while processing an event. Currently the helper
>> object is being created *after* IME handling instead of before. If the IME
>> is enabled it's possible for the WindowContext to be deleted in the middle
>> of executing a number of keyboard-related events.
>>
>> The fix is simple; instantiate the EventCounterHelper object earlier. There
>> isn't always a WindowContext so I tweaked the EventCounterHelper to do
>> nothing if the context is null.
>>
>> To make the crash more reproducible I altered the WindowContext such that
>> when it's deleted the freed memory is filled with 0xCC. This made the crash
>> more reproducible and allowed me to test the fix. I did the same with
>> GlassView since that's the only other Glass GTK class that's instantiated
>> with `new` and discarded with `delete`.
>
> Martin Fox has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Debugging code turned off by default. Empty line removed.
LGTM now. I did ask one question and will reapprove if you make the change.
modules/javafx.graphics/src/main/native-glass/gtk/DeletedMemDebug.h line 46:
> 44: static void operator delete[](void* ptr, std::size_t sz)
> 45: {
> 46: ::memset(ptr, 0xcc, sz);
Should this use `FILL` instead of hard-coded `0xcc`?
-------------
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1307#pullrequestreview-1786885134
PR Review Comment: https://git.openjdk.org/jfx/pull/1307#discussion_r1430137769
