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.
