<AWT Dev> [8] Review request for 8024061: Exception thrown when drag and drop between two components is executed quickly

[anton nashatyrev]

Hello Anthony,

     thanks for having a look at the fix! See comments inlined:

On 15.10.2013 15:02, Anthony Petrov wrote:
> The analysis of the problem looks good. Please clarify one point:
>
> "(2) mouse moved to target in a single X11 event"
>
> Is the "single event" the one from (1) or from (3) ? IOW, are the (1) 
> and (2) a single event, or the (2) and (3) ?
No, each step is a separate X event: (1) ButtonPress, (2) MotionNotify, 
(3) ButtonRelease
>
> Anyway, I have two concerns regarding the fix:
>
> 1. In startDrag() you now call the setNativeContext() and 
> setCurrentJVMLocalSourceTransferable() under the awtLock, whereas 
> previously these calls were preformed w/o the lock. This seems a bit 
> risky as potentially this may cause some deadlocks. We should possibly 
> rearrange the calls so that processMouseMove() is called before the 
> above two calls, and the two calls remain outside the lock. 
> Alternatively, we could acquire the lock again just for the sake of a 
> call to processMouseMove().

Agree, this would be safer.

>
> 2. It looks suspicious that we call processMouseMove() 
> unconditionally. The dragGestureMouseMotion could be stale, or 
> uninitialized yet. 

I'm assuming that startDrag may occur only in response to MotionNotify 
event, which should initialize the field and additionally we are 
synchronizing on awtLock (startDrag() is invoked on the EDT) so it looks 
like there is no need to check for initialization. Anyway if you think 
this is the right thing there is no problem to add an additional check.

> Also, the event might actually be processed twice because 
> doProcessEvent() returns false in that case.

Good point... Do you see any potential problem with that?
The field dragGestureMouseMotion is accessed under awtLock in both 
places, so it would be either initialized on the first doProcessEvent 
call or if the second call occurs it might be rewritten with the same 
value (if dnd is still not started).

Thanks!
Anton.

>
> -- 
> best regards,
> Anthony
>
> On 10/14/2013 06:26 PM, anton nashatyrev wrote:
>> Hi Artem,
>>
>>      with the fix the 'drag entered' event is actually generated on the
>> mouse move event (step 2). Currently this first move event is 'consumed'
>> by the MouseGestureRecognizer and is not treated as the part of a drag
>> operation, while this move could be important (as in our case).
>>      During the process I've created a couple of fixes which tried to
>> address the step 3, but all of them seemed to me rather like hacks than
>> fixes.
>>
>>      BTW, I've run the regression DnD tests (from
>> test/closed/java/awt/dnd + test/java/awt/dnd, totally about 75 tests) -
>> all went fine.
>>
>> Thanks!
>> Anton.
>>
>> On 14.10.2013 17:45, Artem Ananiev wrote:
>>> Hi, Anton,
>>>
>>> thanks for details description of the bug and suggested fix. It helped
>>> a lot.
>>>
>>> My understanding is that the problem is not with step 2, but step 3.
>>> Generating "drag entered" events on mouse press looks incompatible, I
>>> can't predict side effects of such a change. Fixing step 3, so we
>>> don't assume drag entered events already sent, looks more correct.
>>> What do you think?
>>>
>>> Thanks,
>>>
>>> Artem
>>>
>>> On 10/11/2013 7:49 PM, anton nashatyrev wrote:
>>>> Hello,
>>>>      could you please review the following fix:
>>>>
>>>> fix: http://cr.openjdk.java.net/~mcherkas/anton/8024061/webrev.00/
>>>> <http://cr.openjdk.java.net/%7Emcherkas/anton/8024061/webrev.00/>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8024061
>>>>
>>>>      Problem:  when doing quick drag'n'drop in X11 the incorrect
>>>> behavior appears in the different forms. One is the exception when
>>>> trying to get 'local Jvm' Transferable from DropTargetListener.
>>>>
>>>>      Reason: on quick drag the following sequence of events may 
>>>> appear:
>>>> (1) mouse pressed on source  -> (2) mouse moved to target in a single
>>>> X11 event -> (3) mouse released. What's happening in that case: on 
>>>> event
>>>> (2) only a drag gesture is recognized and no drag enter is generated
>>>> yet. On event (3) the XDragSourceContextPeer assumes that entered 
>>>> event
>>>> (if it happened) was already generated and the 'local JVM' 
>>>> transferable
>>>> had been already captured by SunDropTargetContextPeer from the static
>>>> field currentJVMLocalSourceTransferable. Thus on event (3) the
>>>> XDragSourceContextPeer posts the additional mouseMove event (which 
>>>> turns
>>>> into DragEnter later) and initiates the cleanup which then resets the
>>>> currentJVMLocalSourceTransferable to null. Thus on DragEnter the
>>>> currentJVMLocalSourceTransferable is already null and the
>>>> SunDropTargetContextPeer appears in the inconsistent state.
>>>>
>>>>      Solution: the event (2) from the example above should not only
>>>> initiate the DnD operation but also be a part of that operation, i.e.
>>>> this event should also appear as a drag motion. For that I propose to
>>>> keep a track of the XMotionEvents in the XDragSourceContextPeer to 
>>>> catch
>>>> the mouse event which initiated the DnD and when a startDrag() is 
>>>> called
>>>> process this event as the first drag motion event.
>>>>
>>>> Thanks!
>>>> Anton.
>>