440911 - Add CF_HDROP support back into Windows drag and drop data object (dragging images to some applications, e.g. PhotoShop, fails)

Status: VERIFIED FIXED

(Core :: Widget: Win32, enhancement, P2)

Description

[Ben Carlson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

With Firefox 2.0, I used to be able to drag images directly from Firefox (both off of a regular .html page or from .jpg URI image location) into the album art section in MP3Tag, a program for editing ID3 information on MP3s. I now have to drag from Firefox into a Windows Explorer window, and _then_ into MP3Tag.

Reproducible: Always

Steps to Reproduce:
1. Open MP3Tag
2. Visit page with image or direct image URL location
3. Drag (unsuccessfully) from Firefox into album art section (either in main pane or 'Extended Tags...' window)


Expected Results:  
The image should be allowed into its designated area in MP3Tag, as it was in Firefox 2.0

Comment 1

[Jim Mathies [:jimm]]

Ben, does the mp3tag program save the file out or something usually? Can you give a bit more detail on what your trying to do? Not much changed in drag and drop for xp, although we did add drag image support.

Comment 2

[Ben Carlson]

Jim, the program stores the image into the ID3 info for each MP3. (The program is free, so you can reproduce this if you'd like, but..) Let's say I want to use this album art: http://ecx.images-amazon.com/images/I/51MfHB97LtL._SS500_.jpg

In FF2, I could drag it from viewing it in Firefox into the pane in MP3Tag, it would allow me to insert the image that way, in either of the two spots where you see album art in this screenshot: http://farm3.static.flickr.com/2012/2104607798_ffe1d16096.jpg

You then hit OK, and the album is embedded in the ID3 tag. But with FF3, it doesn't allow you to do this. Try dragging the image in the first link into the status bar on the bottom of Firefox, you see that O with a / through it, you get the same thing in MP3Tag. Does that help?

Comment 3

[Jim Mathies [:jimm]]

One datapoint - in Vista, I'm unable to drag anything into these windows. (from the desktop, ff3, or ie7). I'll take a look in an xp image.

Comment 4

[Jim Mathies [:jimm]]

Actually, cancel that. Something was acting funny on my end, after I restarted the app it now appears to be working in Vista. I can also confirm in XP.

Comment 5

[Jim Mathies [:jimm]]

Looks like this came out of a patch a while back that removed CF_HDROP support from our data object. This app is only accepting that and without it, refuses to work with us.

Comment 6

[Jim Mathies [:jimm]]

Looks like what we used to do was copy the file out of the cache, put it in a temp location and then hand the path to the requesting drop handler when we receive a CF_HDROP request. I'm not entirely sure we want to add all that code back in. It's quite a bit of bloat. The folks who develop the tagging app could address this by updating their drop code. However, this does work with other browsers so maybe it's a parity thing we want to support.

Comment 8

[Jim Mathies [:jimm]]

Looks like we'll need a fix...

Comment 9

[Jim Mathies [:jimm]]

Attachment: cf_hdrop support patch v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: cf_hdrop support patch v.1

turned off debug output plus some code and commenting cleanup.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

This missed 1.9.0.1, but definitely wanted for a branch patch.

Comment 12

[Ere Maijala (slow)]

Comment on attachment 326715 [details] [diff] [review]
cf_hdrop support patch v.1 

The async implementation in bug 203307 fixed bug 245861 too. As far as I can see, that problem would resurface with this patch. We need to track the files and make sure they're cleaned up upon exit or such.

Comment 13

[Jim Mathies [:jimm]]

> The async implementation in bug 203307 fixed bug 245861 too. As far as I can
> see, that problem would resurface with this patch. We need to track the files
> and make sure they're cleaned up upon exit or such.

There is code in there that tracks, and I just tested to be sure, it's cleaning up the temp file after the drop operation completes. 

Comment 14

[Jim Mathies [:jimm]]

- delete all files within the temp-dir (the system temp, not the firefox cache)
- open a site that contains at least one picture (a non-link picture)
- open a new tab and switch back to site whith the picture
- drag&drop this picture to the new tab 

Actually, I should have tested - this specific scenario does leave an image behind. :/ 

Comment 15

[Yuri Kovalchuk]

Most likely that mp3tag just does not support IStream drag and drop method. This is why functionality described in comment #0 stopped working. CF_HDROP is for local files, I know that small files are downloaded by the time user drags something, but for bigger files (or slow connection) it would fail. That was the original intention of the patch for bug 203307 - to allow user to drag big files or specify what to download.

Comment 16

[Yuri Kovalchuk]

I've contacted the developer of mp3tag, he said he'll be using CF_DIB instead of CF_HDROP in the next version, so there it makes no sence to restore it here :)

Comment 17

[Jim Mathies [:jimm]]

>I've contacted the developer of mp3tag, he said he'll be using CF_DIB instead
>of CF_HDROP in the next version, so there it makes no sence to restore it here
>:)

Unfortunately at least one other major app suffers from this as well, so I'm still inclined to address it at some point.

Comment 19

[dan]

gavin, you said 

"Indeed, in a build with the patch from bug 440911, the drag to PhotoShop
succeeds."

how can i add the patch from this page to firefox 3.0 so it works for me also. can you host the version yourself?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)
> how can i add the patch from this page to firefox 3.0 so it works for me also.
> can you host the version yourself?

I suppose I could, if you really need it urgently, but I really would suggest waiting until it's at least landed on the trunk (after which you can download a nightly). The current patch isn't ready yet because of comment 14.

Comment 21

[Jim Mathies [:jimm]]

dan, you can try it, but this patch has a timing flaw so you might get sporadic results in your drag drops. 

Comment 22

[dan]

i don't know what that even means, but sporadic results is definitely better than none at all and i'm definitely willing to test it out.

since i have no experience in decompiling, recompiling, patching, etc., can you host a compiled version of ff 3 with the patch in it so i can test it out? can you email me it or host it?

Comment 23

[Jim Mathies [:jimm]]

Attachment: cf_hdrop support patch v.2

Comment 24

[Jim Mathies [:jimm]]

I'm not happy with the second patch but it does address the problem. The issue is that nsContentAreaDragDrop fires off a request for the content using an nsIWebBrowserPersist and then returns the path to the destination. The drag-drop nsDataObj then returns this to the target saying basically that the file is available. Once the receiver acknoledges receipt, the data object attempts to delete the file and is destroyed. 9 times out of 10 the file is not fully copied over yet, so the delete fails.

Communication between the data object and the content area is handled through a transferable. I haven't found a way to get a hold of the content area persist over in the data object, which seems to be the best solution if it was possible. In the second patch I wait for the delete operation to succeed and then let the nsDataObj destroy. However, if something were to go wrong and the file never lost it's lock, this would create a race condition that could freeze up the browser.

I'd welcome any feedback / suggestions.

Comment 25

[Samuel Sidler (old account; do not CC)]

Any update on this?

Comment 26

[Jim Mathies [:jimm]]

>Any update on this?

No, I'll be working on this again next week. cf_hdrop support is addressed but I still need to find a better way of dealing with left over files.

Comment 27

[Jim Mathies [:jimm]]

Attachment: cf_hdrop support patch v.3

This addresses a few problems. Our old cf_hdrop support had a problem in that it relied on delay rendered content (in the form of a temp file) when images were dragged out of the content area. However cf_hdrop was not designed for use in this case - the specs for the format dictate the content must exist before the drag operation completes. Dragged content was being saved through a web persist handler which rarely completed until after the drag operation finished. This in turn caused problems for some apps which didn't react well to the file not being written out completely. Also, since the data object is responsible for removing temp files at the end of the drag, temp files were sometimes being left around due to write locks, creating a privacy issue.

cf_hdrop was removed completely in Fx3 as it was assumed most apps had upgraded to newer clipboard formats. But it turns out a number apps still rely on it in a few cases. Dragging a file into the main window of Photoshop is probably the best example.

This patch adds support back in for cf_hdrop for the few apps out there that still require it. Image content is retrieved by the data object as native image data, then using routines in the clipboard image utilities the image is converted to and written out as a bitmap in the temp dir.  After the drag session closes the data object then deletes the temp file. The resulting functionality is identical to the process we go through when Fx3 copy - pastes image data from the content area.

Comment 28

[Daniel Veditz [:dveditz]]

No time for trunk testing before landing it for 1.9.0.4. Request branch approval when you get a tested, reviewed patch.

Comment 29

[Jim Mathies [:jimm]]

+	fileHdr.bfType		    = ((WORD) ('M' << 8) | 'B');
+	fileHdr.bfSize		    = GlobalSize (bits) + sizeof(fileHdr);
+	fileHdr.bfReserved1 	= 0;
+	fileHdr.bfReserved2 	= 0;
+	fileHdr.bfOffBits		  = (DWORD) (sizeof(fileHdr) + bmpHdr->biSize);

Note I'll clean this tab mess up before it lands.

Comment 30

[Ere Maijala (slow)]

Comment on attachment 344099 [details] [diff] [review]
cf_hdrop support patch v.3

Not particularly elegant, but it works :)

Comment 31

[Jim Mathies [:jimm]]

(In reply to comment #30)
> (From update of attachment 344099 [details] [diff] [review])
> Not particularly elegant, but it works :)

Yeah I wasn't particularly happy with what I had to do either. But I didn't find any other solutions.

Comment 32

[Jim Mathies [:jimm]]

See comment 27 for details on why this should get into 3.1.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 344099 [details] [diff] [review]
cf_hdrop support patch v.3

a=beltzner

While you're waiting for the tree to go green, make sure that this doesn't bust any tests before landing :)

Comment 34

[Myk Melez [:myk] [@mykmelez]]

Patch pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f3b3edd87633

Comment 35

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081117 Minefield/3.1b2pre. I verified by dragging some images from Firefox to Photoshop CS4.

Comment 36

[Anthony Meehan]

I've just tried this using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081117 Minefield/3.1b2pre and the CF_HDROP is now present, except the image is not in the original format. If I drag a JPG from a web page, the file in the CF_HDROP is a .bmp file. Is this by design?

Comment 38

[Nochum Sossonko [:Natch]]

removing fixed1.9.1 b/c these bugs are verified1.9.1, sorry for the spam.

Comment 40

[Tor Lillqvist]

Is the CF_HDROP mechanism the same as the WM_DROPFILES mechnism, or is there supposed to be some magic automatic compatibility trnslation going on in Windows to make CF_HDROP drag sources work as WM_DROPFILES sources, too?

Is there some known problem in the WM_DROPFILES support? At least dragging images from Firefox to a GTK+ client doesn't work (the GTK+ app gets the WM_DROPFILES message, but the file name returned in DragQueryFile() does not exist). (Dragging files from Explorer works fine.)

Comment 41

[Jim Mathies [:jimm]]

(In reply to comment #40)
> Is the CF_HDROP mechanism the same as the WM_DROPFILES mechnism, or is there
> supposed to be some magic automatic compatibility trnslation going on in
> Windows to make CF_HDROP drag sources work as WM_DROPFILES sources, too?

Sounds like it. Windows may be querying our ole implementation for a file name and we aren't responding with something appropriate. Any suggestions on a simple GTK+ client we could use to test with?

Comment 42

[Tor Lillqvist]

Well, it's not simple, but GIMP is readily avaibale as an all-inclusive installer, and it accepts WM_DROPFILES style drops. From gimp-win.sf.net. Run it with the environment variable GDK_DEBUG set to "dnd" and the command-line argument --console-messages to get some hopefully easy to interpret debugging printout about DND in a console window. (Or to stdout, if you redirect it, then the --console-messages isn't necessary.)

When dragging an image from Firefox to GIMP, the DND debugging output looks like this:

gdk_window_register_dnd: 00130A60
gdk_window_register_dnd: 001109C6
gdk_window_register_dnd: 001409CA
WM_DROPFILES: 00130A60
gdk_drag_context_init 07AEAD00
... C:\Users\tml\AppData\Local\Temp\wngsw7xh.bmp: file:///C:/Users/tml/AppData/Local/Temp/wngsw7xh.bmp
gdk_selection_owner_get: DROPFILES_DND = 00000000
gdk_selection_convert: 00140A2E DROPFILES_DND text/uri-list
gdk_drop_reply
gdk_selection_property_get: 00140A2E text/uri-list format:8 length:55
gdk_drop_finish
gdk_drop_finish
gdk_drag_context_finalize 07AEAD00
gdk_property_delete: 00140A2E GDK_SELECTION
_gdk_selection_property_delete: 00140A2E (no-op)

and that file C:\Users\tml\AppData\Local\Temp\wngsw7xh.bmp does not exist.

When dragging a file from Explorer, the output looks like this:

gdk_window_register_dnd: 00050BD0
gdk_window_register_dnd: 00040C4E
gdk_window_register_dnd: 00040BD6
WM_DROPFILES: 00050BD0
gdk_drag_context_init 07A14D00
... C:\tmp\d.bmp: file:///C:/tmp/d.bmp
gdk_selection_owner_get: DROPFILES_DND = 00000000
gdk_selection_convert: 00100A12 DROPFILES_DND text/uri-list
gdk_drop_reply
gdk_selection_property_get: 00100A12 text/uri-list format:8 length:23
gdk_drop_finish
gdk_drop_finish
gdk_drag_context_finalize 07A14D00
gdk_property_delete: 00100A12 GDK_SELECTION
_gdk_selection_property_delete: 00100A12 (no-op)

I.e. to the WM_DROPFILES handler in GTK+ everything works the same way, but the file name received from DragQueryFile() is an existing one, the one dragged.

Comment 43

[Jim Mathies [:jimm]]

Ok this makes sense, due to privacy concerns we only leave that file around until the drag operation completes, then we delete it. This gives apps like Photoshop a chance to open the file and display it. But it's not meant to stick around. 

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.cpp#626

We could maybe create a pref option for leaving these files around? But I'm not sure we would want it on by default.

(We should spin this out into a separate bug for discussion.)

Comment 44

[Jim Mathies [:jimm]]

Actually, see bug 245861. ;)

Comment 45

[Jim Mathies [:jimm]]

Tor, do files dragged from IE stick around? Just curious.

Comment 46

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It's actually not just a privacy issue, but also a disk usage issue.. most of the time a user has no idea how to get to the tempdir.  But what we could do is delete files after some time elapses, or at app shutdown?  Doesn't windows have a flag that says "delete this file at next restart", too?

Comment 47

[Jim Mathies [:jimm]]

(In reply to comment #46)
> It's actually not just a privacy issue, but also a disk usage issue.. most of
> the time a user has no idea how to get to the tempdir.  But what we could do is
> delete files after some time elapses, or at app shutdown?  Doesn't windows have
> a flag that says "delete this file at next restart", too?

There's a risk there too - applications like photoshop receive a file path we mark for deletion. If the user then edits the file and saves without moving it, we delete it.

It would be better to hand the data directly through CF_DIB to GTK, this requires they update their handling. We added support back in for CF_HDROP specifically because some apps have not yet upgraded their handling. In time the CF_HDROP code should become obsolete.

Comment 48

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #47)
> (In reply to comment #46)
> > It's actually not just a privacy issue, but also a disk usage issue.. most of
> > the time a user has no idea how to get to the tempdir.  But what we could do is
> > delete files after some time elapses, or at app shutdown?  Doesn't windows have
> > a flag that says "delete this file at next restart", too?
> 
> There's a risk there too - applications like photoshop receive a file path we
> mark for deletion. If the user then edits the file and saves without moving it,
> we delete it.

That's fine, I think -- much better than the current situation.  No different than editing a temporary file from IE or something.  CF_DIB doesn't preserve any metadata on the image, and doesn't work for non-images.. so I don't think it's a solution.

Comment 49

[Tor Lillqvist]

A problem on the GTK side is that at this moment WM_DROPFILES is the only kind of inter-process DND that it supports. Sad but true. Nobody has ever been sufficiently motivated to implement the OLE-based (or is that COM-based, I always mix up these acronyms) generic DND stuff. (Partly, at least for me, because it involves doing programming with those interfaces and vtables which is always somewhat tedious in plain C.)

But yeah, I really should try to gather inspiration some weekend and just do it.

The fact that the "file" being dropped is just a temporary one, and thus starting to edit it in an app like GIMP, perhaps saving it back even, would be silly indeed. I should ask what the users complaining in the GIMP and GTK+ bugzilla think should happen if it worked to drag-and-drop and image from Firefox onto GIMP. Should GIMP pretend it is just a normal local file? The ideal would be to open a drag-and-dropped image file so that it is not associated with a file name in the app, like a cut-and-pasted images. Hmm.

One workaround hack would be to make GTK recognize that the file being dropped is a temporary file (hmmm... check if it is in the user's "Temp"... horrible hack indeed), and in that case let GTK copy it to another file whose name then is given to the upper levels...

About IE in comment #45, unfortunately I can't test right now, as for some reason this IE8 doesn't even let me drag images, for some reason. Did MSFT remove this feature, dunno.

Comment 50

[Weeble]

In Windows, when you create a file with CreateFile you could set FILE_FLAG_DELETE_ON_CLOSE and a sharing mode that only allows FILE_SHARE_READ. Then so long as you keep the file open, the other application can read it but cannot write it. When you close the file (or even if your process terminates unexpectedly) Windows will delete the file. Perhaps that would be more useful behaviour?

http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #50)
> In Windows, when you create a file with CreateFile you could set
> FILE_FLAG_DELETE_ON_CLOSE and a sharing mode that only allows FILE_SHARE_READ.
> Then so long as you keep the file open, the other application can read it but
> cannot write it. When you close the file (or even if your process terminates
> unexpectedly) Windows will delete the file. Perhaps that would be more useful
> behaviour?
> 
> http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx

That's certainly a better solution than what we currently have, although I wonder if it would fix the GTK issue? They don't appear to be opening the file upon receiving WM_DROPFILES, or windows is sending the message after the operation completes. A little testing might be in order.

Comment 52

[Jim Mathies [:jimm]]

Looks like bug 494989 will address the temp file issues. Bug 338478 also adds drop file support.