<AWT Dev> [9] Review Request: JDK-7185258 [macosx] Deadlock in SunToolKit.realSync()

[Anthony Petrov]

Hi Petr,

The fix version .01 looks good and safe to me.

--
best regards,
Anthony

On 1/17/2014 5:52 PM, Petr Pchelko wrote:
> Hello, AWT Team.
>
> Let's reveal this discussion.
>
> The bug: https://bugs.openjdk.java.net/browse/JDK-7185258
> The fix: http://cr.openjdk.java.net/~pchelko/9/7185258/webrev.01/
>
> This review was left because there was an idea to use a different approach to this problem: just use the timeout, detect a deadlock and return false from syncNativeQueue.
> I've implemented this approach and played with it, but it appears to work not really well. The problem is the timeout: if we set it too little we would have too many false positive deadlocks
> which would result in bad synchronization and tremendously increasing probability of the InfiniteLoop exception. If the timeout is too big tests slow down badly, but what's even worse - if we wait
> for a deadlock too long some events could get posted to EDT (repaints, timer updates etc..) resulting in an InfiniteLoop exception. So, I've decided to return back to my original approach.
>
> The idea of this fix is to interrupt waiting as soon as we detect that we've got into the nested Appkit loop. This happens in DnD and in LWCToolkit.invokeAndWait.
>
> With this fix several tests start to pass. Tested on DnD-related regression tests.
>
>>>> 2. To dispatch some DropTarget events on EDT synchronously we start our own nested loop in doAWTRunLoop using the ToolkitThreadBlockedHandler. In this case we do process the events in a nested runLoop.
>>>> This case might actually be a bug, because we are short-circuiting normal event processing and could "steal" some events from Cocoa. But I would not address this it in this fix.
>>> Probably it is a good time to realize how it should work? Since the fix adds complexity which can not be necessary in the future.
>> I agree. But this would be a long investigation, so let's leave this review until I find out how we should really work.
> This question arises during the review. It's not related, but anyway:
> Our nested loop is indeed quite dangerous, because it could really still events from Cocoa, so it should be used with great cation. But we need to process the events in this nested loop, because DropTarget event handlers are called
> synchronously and opening a modal dialog in such a handler would completely block the application if we do not process events in nested loop. So, everything works fine right now.
>
> With best regards. Petr.
>
> On 24.12.2013, at 12:30, Petr Pchelko <petr.pchelko at oracle.com> wrote:
>
>> Hello, Sergey.
>>
>>>>> As I remember  doAWTRunLoop  process events and selectors in case of drags/DnD otherwise how we got mouse events?  Or I have missed something?
>>>> In case of DnD we have several nested loops in place:
>>>> 1. The native Cocoa DnD nested loop. It's started within the NSView dragImage:... call. It processes only the DnD-related events - some mouse events, some key events and flag changed events. This is for the DragSource.
>>> And we cannot emulate such event in postDummyevent?
>> We could post something like a dummy MouseUp event and get rid of it in the NSApplication sendEvent:..., but I don't like such a solution.
>> May be Cocoa has more nested event loops we do not know about yet which do not accept MouseUp. The same would be valid for any event type.
>>
>>>> 2. To dispatch some DropTarget events on EDT synchronously we start our own nested loop in doAWTRunLoop using the ToolkitThreadBlockedHandler. In this case we do process the events in a nested runLoop.
>>>> This case might actually be a bug, because we are short-circuiting normal event processing and could "steal" some events from Cocoa. But I would not address this it in this fix.
>>> Probably it is a good time to realize how it should work? Since the fix adds complexity which can not be necessary in the future.
>> I agree. But this would be a long investigation, so let's leave this review until I find out how we should really work.
>>
>> With best regards. Petr.
>>
>> On 24.12.2013, at 12:21, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
>>
>>> On 12/24/13 11:57 AM, Petr Pchelko wrote:
>>>> Hello, Sergey.
>>>>
>>>>> As I remember  doAWTRunLoop  process events and selectors in case of drags/DnD otherwise how we got mouse events?  Or I have missed something?
>>>> In case of DnD we have several nested loops in place:
>>>> 1. The native Cocoa DnD nested loop. It's started within the NSView dragImage:... call. It processes only the DnD-related events - some mouse events, some key events and flag changed events. This is for the DragSource.
>>> And we cannot emulate such event in postDummyevent?
>>>> 2. To dispatch some DropTarget events on EDT synchronously we start our own nested loop in doAWTRunLoop using the ToolkitThreadBlockedHandler. In this case we do process the events in a nested runLoop.
>>>> This case might actually be a bug, because we are short-circuiting normal event processing and could "steal" some events from Cocoa. But I would not address this it in this fix.
>>> Probably it is a good time to realize how it should work? Since the fix adds complexity which can not be necessary in the future.
>>>>
>>>> Without the DnD we use a doAWTRunLoop which does not process events. (Except the case with Single-Threaded Interop with FX).
>>>>
>>>> Actually, after some more thinking I believe I should reimplement the fix and follow Anthony's suggestion with a timeout, because my fix is too complex. I'll send a new webrev later today.
>>>>
>>>> With best regards. Petr.
>>>>
>>>> On 24.12.2013, at 1:41, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
>>>>
>>>>> Hi, Petr.
>>>>> On 23.12.2013 16:54, Petr Pchelko wrote:
>>>>>> The problem:
>>>>>> nativeSyncQueue synchronizes the native event queue. However, there are several situations when a dummy event posted to the native queue is not dispatched. These are our own nested loop in doAWTRunLoop and Cocoa's internal nested loop in NSView:dragImage.
>>>>> As I remember  doAWTRunLoop  process events and selectors in case of drags/DnD otherwise how we got mouse events?  Or I have missed something?
>>>>>
>>>>>> Solution:
>>>>>>
>>>>>> 1. The interruptNativeSyncQueue was introduced. In case we are waiting for the native event queue this method interrupts waiting, otherwise it's a no-op. This is needed for the following reason: suppose the nativeSyncQueue is called on EDT. While the queue is flushed some event was processed which caused us to call doAWTRunLoop. As EDT is blocked we would have got a deadlock. Interrupting the wait lets EDT flush it's events and lets AppKit exit a nested loop. When the nativeSyncQueue is interrupted we do not immediately exit realSync, but flush EDT and try to sync native queue again. Most likely in this case we would not be in a nested loop and will successfully sync a native queue.
>>>>>>
>>>>>> 2. A lightweight version of nativeSyncQueue was introduced. This one does not flush the event queue, it only flushes a selector queue. This is needed to not deadlock when realSync was called during DnD. Suppose nativeSyncQueue is called while the app is in the native nested dragging loop. Until dragging operation finishes only dragging-related events will be processed. So we have no opportunity to flush the queue as our dummy event will be blocked by a dragging nested loop.
>>>>>>
>>>>>> I've tested it by running almost all awt and swing tests. No new failures, some tests start to pass after the fix.
>>>>>>
>>>>>> With best regards. Petr.
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>>>
>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>
>