Community
Participate
Working Groups
- attach a Media Device to your Windows machine (e.g. an Android Smartphone), - open the Windows Explorer and select that device - launch the attached snippet - start a drag from "Start drag from here" onto the Windows Explorer and release the mouse -> the event sequence in the output will look like: dragStart dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragEnd dragSetData dragEnd async dragSetData dragSetData which is very suspicious, because dragSetData is still invoked after dragEnd (which IMHO is designed to do cleanup stuff on the drag source). Note, that dragging to a normal Windows file system, the sequence is correct: dragStart dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragEnd dragSetData dragEnd async This is not a theoretical problem, but causes our application to get exceptions like this because of cleanup code in dragEnd: org.eclipse.swt.SWTException: Data does not have correct format for type at org.eclipse.swt.dnd.DND.error(DND.java:291) at org.eclipse.swt.dnd.DND.error(DND.java:241) at org.eclipse.swt.dnd.FileTransfer.javaToNative(FileTransfer.java:73) at org.eclipse.swt.dnd.DragSource.GetData(DragSource.java:479) at org.eclipse.swt.dnd.DragSource.access$500(DragSource.java:106) at org.eclipse.swt.dnd.DragSource$2.method3(DragSource.java:266) at org.eclipse.swt.internal.ole.win32.COMObject.callback3(COMObject.java:95) at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3595)
Created attachment 279427 [details] Snippet to reproduce
Unfortunately, this seems to have been introduced by commit c1dbf469ed "Bug 547634 - [win32] Add CFSTR_SHELLIDLIST transfer type to FileTransfer to support drop on taskbar".
Can confirm your observations. Will see if I can figure out if it is my fault or some strange Windows behavior. Before bug 547634 a Media Device did not accept FileTransfer at all. Now the Explorer signals acceptance but does not work.
From what I found so far I think we have two (related) problems. (and maybe some more hidden problems in dnd) 1. dragSetData called after dragEnd. When SWT detect a drag operation it first fires the dragStart event and then invokes the winapi DoDragDrop function which manages the whole operation. When DoDragDrop returns the operation is finished (either dropped, canceled or failed) and SWT invokes dragEnd. The problematic dragSetData is kind of outside SWT's control. Source and target communicated through interfaces. Most interesting IDataObject. The source create this data object and the target get a reference from DragEnter (and from the final Drop). A call to the data objects GetData method become a dragSetData event in SWT. Usually GetData is not used after DoDragDrop finished. Windows Portable Devices (apparently the more official term for Media Device) seem to be an exception for this rule. In fact since the target got the data object it could invoke GetData and therefore trigger dragSetData events all day long until DropSource is disposed even if the drop operation was canceled long ago. So in this context I would say bug 547634 is not the fault but enables the problematic behavior. I see two possible solutions so far: a) Create an unique data object for each drag operation and dispose it once the operation is finished. b) Add a flag and discard all calls to GetData outside the DoDragDrop loop. For a) the problematic part is the tight coupling between the IDataObject and the DragSource. A first test with createCOMInterfaces on drag start and disposeCOMInterfaces on drag end broke drag'n'drop from SWT to SWT. (btw it did not break from SWT to Windows Explorer) Solution b) is simpler but still problematic for malicious drop targets. If drop target A was entered and then calls GetData forever canceling the drag operation would stop the dragSetData events. But they would reappear for a new drag operation on target B since current DragSource has only one single IDataObject.
2. Dropping files on Windows Portable Device. Since bug 547634 dragging a file onto WPD indicates a drop is accepted but fails always. On Windows 10 only audio feedback (and the fact the file is not transfered) indicate the failure. On Windows 7 an error dialog is shown additional. I will see if I can fix it and enable dropping onto WPD. If not there might be no good solution to keep drop on taskbar and at the same time disable drop on WPD. I think there is no way to tell a target we support ShellIDListTransfer but not if you are a WPD.
@Thomas are you sure for the event sequence for dropping on normal file system? The dragSetData between dragEnd and dragEndAsync is also suspicious.
(In reply to Paul Pazderski from comment #6) > @Thomas are you sure for the event sequence for dropping on normal file > system? The dragSetData between dragEnd and dragEndAsync is also suspicious. No, this was a copy-paste error. It should be dragStart dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragSetData dragEnd dragEnd async
For the second problem "drop fails on Media Device" I opened bug 549661.
After more research I would say this topic is worse as expected. First of all my previous findings are still valid i.e. to my best knowledge I made no mistake in my comments so far (and hopefully none in this). The center point of Windows DnD is the DoDragDrop function which track mouse and keyboard and manages most communication between drop source and target. Any documentation, any example, any secondary literature I found said a dnd operation is finished when DoDragDrop returns. Only the Portable Device part of Windows Explorer is ignoring this which is in my opinion a bug. Drop on Portable Device fails due to the incomplete PIDL I described in bug 549661 but even worse it requires the later dragSetData events. I.e. drop on Portable Device is not finished when DoDragDrop returns and blocking all dragSetData events after dragEnd kills the dropping on Portable Device operation. How can we detect when DnD is finished? The bad tldr: you can not in any case. If you count the return of DoDragDrop as finish you miss such faulty targets as Portable Device. Another signal for the drop end can be the IDataObject. A short reminder on IDataObject. This COM object is created by the DragSource to store the data to drag and to render those data in the various formats the source supports. The target get a reference to this IDataObject and can request the drag data in its preferred format. Like any COM object it does reference counting to manage its lifetime. That is what can be used to recognize DnD finished. This definition works for regular file drop on Desktop as well as for dropping on Portable Device. But! The IDataObject is created by the source and passed to the target. The target could be any buggy or malicious application. If it does not probably release its references the drop would never finish. (btw. SWT DropTarget is such a faulty implementation...) Why do other applications have no such problems? Various reasons. At first I do not know much applications which can drag files at all. Next several applications only support the more simple CF_HDROP where dropping on Portable Device does not work and dodge this problematic behavior. The main difference is that SWT does DnD slightly different than usually implemented in Windows. As mentioned before the usual procedure is create IDataObject, add the data, start DoDragDrop. SWT instead creates one hollow data object for all drags and instead of holding the data generates a dragSetData event for each data request. I.e. most implementations do not care much for data cleanup (what Thomas want to do in dragEnd) because they can cleanup when the IDataObject object is released and as long as it is not released it can still answer GetData (basically the same as SWT's dragSetData) requests. To support my claims I found one open source application which implement its own OLE based file dragging and support ShellIDList. The file explorer doublecmd [1,2]. It even has similar dragStart and dragEnd events. After adding some logging on start, end and GetData I could produce the following output when dropping a file from doublecmd onto Desktop: 31.07.2019 20:59:09 DragBegin 31.07.2019 20:59:09 GetData 31.07.2019 20:59:09 GetData ... 31.07.2019 20:59:09 GetData 31.07.2019 20:59:09 GetData 31.07.2019 20:59:09 DragEnd and this output when dropping onto a Portable Device: 31.07.2019 20:59:17 DragBegin 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 DragEnd 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData 31.07.2019 20:59:17 GetData What can we do? My plan so far is to keep the addition from bug 547634, fix this bug and fix bug 549661 as well. To archive this I would: 1. fix COM object reference handling in DragSource (almost ok) and DropTarget (has a little bug) 2. if after DoDragDrop the IDataObject is not already released than delay the dragEnd event (while processing other messages) and include a timeout for the case that IDataObject is never released and force dispose it in this case [1] https://doublecmd.sourceforge.io/ [2] https://sourceforge.net/p/doublecmd/code/HEAD/tree/branches/0.9/src/platform/uOleDragDrop.pas#l1524
New Gerrit change created: https://git.eclipse.org/r/147515
(In reply to Eclipse Genie from comment #10) > New Gerrit change created: https://git.eclipse.org/r/147515 This should fix the reported problem. In addition it should prevent another "vulnerability". If a malicious target calls Release on the IDataObject multiple times the DragSource will dispose and null it's singleton IDataObject and therefore all following Drag operations fail with a NPE.
Gerrit change https://git.eclipse.org/r/147515 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=fb29bcbf1cf8004cbc97a1d4db4621c00a0fe7cf
