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

[Anthony Petrov]

Hi Anton,

On 10/16/2013 03:26 PM, anton nashatyrev wrote:
> 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

Please clarify what exactly do you mean by "mouse moved to target in a 
single X11 event"? Do I understand correctly that the real problem we're 
trying to fix here is the absence of a subsequent MotionNotify before 
the final ButtonRelease gets processed?


>> 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.

I think this assumption needs to be documented then. Please check all 
possible code paths leading to startDrag(). If it's only called in 
response to the MotionNotify event, then please add a comment about that 
to the code (right before the startDrag() definition seems to be the 
best place). In that case, no additional checks are needed.


>> 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).

I don't know if this is a problem or not, but this is a change in the 
(long-standing) behavior and some legacy apps may not be ready to handle 
that. Perhaps we could suppress handling of the mouse event somehow if 
we know startDrag() will process it anyway?

--
best regards,
Anthony

>
> 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.
>>>
>