<AWT Dev> OpenJdk11-28-EA JDialog hanging

[Laurent Bourgès]

Hi Martin,

What an amazing effort !
I am very pleased that my (crazy) test triggered a very complicated case; I
will try asap if it works with your new webrev.

Thanks for your feedback and sharing your test.
>
> I've taken your suggestion of making the filter class final. In regards to
> having a single instance of the filter class, it would have been a good
> idea but now we have some additional requirements you'll see below.
>
> I could reproduce your deadlock. This deadlock scenario is a bit more
> interesting than the one fixed in Webrev00.
>
> At some point, we may reach the following state:
>
> SequenceEvent.list: EV0-AppContext1, EV0-AppContext0, EV1-AppContext0
> EventQueue0: EV0-AppContext0, EV1-AppContext0
>
> EDT0 now takes EV0-AppContext0 out of EventQueue0. When dispatching this
> event, it notices that it's not the first one in SequenceEvent.list and
> proceeds to dispatch a new event from EventQueue0 while waiting. It gets
> EV1-AppContext0 and again, it cannot continue because EV0-AppContext1 is
> still the first one in SequenceEvent.list. EDT0 now waits for either a
> SentEvent (sent by EDT1) or for a new SequencedEvent in EventQueue0.
>
> Let's say that EDT1 dispatches EV0-AppContext1 and sends a SentEvent to
> EDT0. EDT0 wakes up (enabled at Webrev00) and tries to dispatch
> EV1-AppContext0. However, EV1-AppContext0 is not the first one in
> SequenceEvent.list so it goes to sleep again (waiting for a SentEvent or a
> new SequencedEvent to come).
>
> However, EDT0 is the only one that can unlock this by dispatching
> EV0-AppContext0. This is not possible because EV0-AppContext0-dispatch call
> is down the stack. There is a stack frame on top of it: the one for
> EV1-AppContext0-dispatch. It's an inversion of the order.
>
> We don't want EDT0 to take a new SequencedEvent out of EventQueue0 (that
> is: creating a new dispatch frame on the call stack) if all events in
> SequenceEvent.list previous to the currently SequencedEvent event being
> dispatched are from a different AppContext.
>
> My first approach to this was: check SequenceEvent.list before going to
> sleep and see if we need to filter SequencedEvent events or not. However,
> this does not work for 2 reasons:
>
>  1) Events in SequencedEvent.list which are not being dispatched have a
> null AppContext. The only way of knowing if they belong to a specific
> AppContext is by iterating the EventeQueue queue and checking object
> identity. No APIs for this. On the other hand, assigning an AppContext when
> creating SequencedEvent events may break things (i.e.: creating events on a
> TG and posting them to a different one).
>
>  2) We cannot loose SequencedEvent events. Of course.
>
> What I propose to do is capturing out-of-order SequencedEvents events in
> the filter and releasing them once we dispatch the frame. In other words,
> try to dispatch only if the event that arrives is from a previous position
> in SequenceEvent.list (so we won't block).
>
> In addition to these changes, I propose to dispatch any non-SequencedEvent
> event while waiting. There is no need to block other events, and the GUI
> becomes unresponsive under a stress case such as the test otherwise.
>
> Removed an unnecessary "wakeup" on SequencedEvent.dispose function.
>
> Webrev 01:
>
>  * http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01/
>  *
> http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01.zip
>

I had a quick look to the patch.
I noticed that the list pendingEvents is not synchronized and I wonder if
multiple EDT threads may be working on the same SequencedEvent:
- currentSequencedEvent.pendingEvents.add(ev);
- for(AWTEvent e : pendingEvents) { SunToolkit.postEvent(appContext, e); }

I suppose it should never happen, so synchronization is useless !

@Laurent: is it possible to tweak your test a bit to remove internal API
> usage and include an assertion? I don't know if you have tried that before,
> but it would be very nice so we can include your test in the patch. One
> additional comment regarding the test: the 10 ms lapse between events
> injection it is too fast for dispatching to keep up with -at least in my
> environment-; the queue tends to grow in the long run. 100 ms made it
> stable. I don't expect this test to run for too much though.
>

Good idea, I will try rewriting the test.

Regards,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20181012/40c96f7d/attachment.html>