491239 - text/x-moz-url should not be required for transferring data to a local file

Status: REOPENED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Phil Lacy]

The x-moz-file-promise-xx flavours govern data transferred to disk
The x-moz-url is used for transfer of url's (in so far as nsDataObj.cpp referenced later here)
re:http://mxr.mozilla.org/comm-central/source/mozilla/widget/public/nsITransferable.idl#67

The nsDataObj.cpp in all respects (and intents), assigns url transfer and data transfer as indicated above.
re: http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#945
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#914

But it appears like nsClipboard.cpp lumps all of the above flavours in x-moz-url
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsClipboard.cpp#202
Also in keeping with the older nsDataObj.cpp code from TB2. It expects the x-moz-file-promise should always be a move. This is because it use to be a temp file that should be moved and not copied.  But in present code it is only using a stream to send data to a file created by the O/S shell.

Here's another spot where the nsClipboard code and nsDataObj code differ in what format assigned the kfilepromise.
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsClipboard.cpp#112
I'm not sure yet what effect this has but doesn't appear correct.

The upshot of all this, besides not having the nsClipboard code up to date with the nsDataObj, the front end has to set the x-moz-url for any of the x-moz-file-promise flavours.

It may be nit-picky but maybe in the future or other uses for dnd. It will always be a question why it is needed.

Another questionable implementation, kFileMime is treated the same as kFilePromiseMime
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#917
the first should be a file (maybe temp file in most cases) the second should use the stream.  They are both set up to transfer with the stream.

Comment 1

[Phil Lacy]

Attachment: corrects 'x-file-promise...' type flavours

kFilePromiseMime in nsDataObj.cpp has data as stream but does not set up the data with the correct FORMATETC needed for streams.  Therefore front end code has to rely on also illogically setting x-moz-url to get the backend to set up correctly.

this patch corrects that.

Comment 2

[Phil Lacy]

Comment on attachment 379781 [details] [diff] [review]
corrects 'x-file-promise...' type flavours

>-  else if (strcmp(aMimeStr, kFileMime) == 0 || 
>-           strcmp(aMimeStr, kFilePromiseMime) == 0)
>-    format = CF_HDROP;
>+  // tied to GetDataFromDataObject() only following is local file
>+  else if (strcmp(aMimeStr, kFileMime) == 0 )
>+    format = CF_HDROP;  // format for local file

This change does nothing. Supposedly, since CF_HDROP is used by some apps the original code was put in but there is no way for the app to access it since it only gets data for images
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#1405
this is accessed by the target here:
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#515

This needs to be fixed in another bug

Comment 3

[Ere Maijala (slow)]

Comment on attachment 379781 [details] [diff] [review]
corrects 'x-file-promise...' type flavours

Sorry, for some reason I have real hard time following this. Anyway, as far as I can see this would directly contradict the definition of kFilePromiseMime in nsITransferable.idl ("a *dataless* flavor"), no?

Comment 4

[Phil Lacy]

Actually, I was trying to correct our current implementation to bring it in line with the nsITransferable.idl.  Without this change the nsITransferable would need to be commented "x-moz-file-promise requires the x-moz-url flavor".  This didn't seem logical and was the reason for this patch. The kFilePromiseMime can still be dataless. Data /is/ required to be set for x-moz-file-promise-url. Here's a summary of how it currently works.

With x-moz-url (data inconsequential) windows format becomes CFSTR_FILEDESCRIPTOR , windows calls getdata asking for this format, gets GetFileDescriptor() which checks if x-moz-file-promise is present (this is why the patch sets CFSTR_FILEDESCRIPTOR for x-moz-file-promise and again flavor data is inconsequential). Then (in another disjointed logic), GetFileDescriptor() asks for x-moz-file-promise-url to get the data for the file. Here we do need data for this flavor.

Then windows asks for the data with format CFSTR_FILECONTENTS. Again x-moz-url sets this format but getdata() conditionally sets up the stream only if we have x-moz-file-promise.  Like above this is why the patch has CFSTR_FILECONTENTS set up with the x-file-promise flavor. No flavor data is required because the stream gets the data from x-file-promise-url.

Comment 5

[Phil Lacy]

for example of the strange logic of current code.

In TB3 attachment code the drag transfer flavor and data is set like this:

"text/x-moz-url", tmpurlWithExtraInfo + "\n" + attachmentDisplayName
"text/x-moz-url-data", tmpurl
"text/x-moz-url-desc", attachmentDisplayName
"application/x-moz-file-promise-url", tmpurl
"application/x-moz-file-promise", new nsFlavorDataProvider(), 0, Components.interfaces.nsISupports
taken from http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#1917

yet if you replace this with following flavor/data it still works:
"text/x-moz-url", "none"
"text/x-moz-url-data", "none"
"text/x-moz-url-desc", "none"
"application/x-moz-file-promise-url", tmpurl
"application/x-moz-file-promise",  0

Comment 6

[Phil Lacy]

in this patch kfilepromisemime and kfilemime were originally given CF_HDROP formatetc for the benefit of foreign data sources and this app acting as drop target.

But this routine is also used to set up internal app acting as data source and yet we don't currently return data on calls to data source's GetData for these flavors.

Therefore I'm taking it out for now.

Comment 7

[Phil Lacy]

Well, I can't get away from this because the code is just not right.

First the reason for reopening, hopefully this will be reviewed and verified by those familiar with this part of code as used in FF so we can patch it. I only can vouch for thunderbird use.
I had assumed, I think mistakenly, that FF would be drop target to external app and would somehow equate kfilepromisemime with CF_HDROP from the external source. But it doesn't work that way at all.
This getformat call is done by internal drag source, and, as I detailed in previous comments, when we tell windows apps (with this call) we have the native CF_HDROP for our internal kfilepromisemime flavor is a bust because we do not support the dataobj's GetData() for CF_HDROP.

This bug is the Thunderbird side of the hangs of bug 514148. The patch for that bug is the FF side of the hang. If we land this we won't care when users patch FF with the other fix.

I'd sure like to campaign for resolving this.

If needed someone can advise what FF d&d scenarios I can test this patch.

Comment 8

[Phil Lacy]

My reason for flag?block 1.9.1, if we land this in 1.9.1.x I understand TB3 final will include it and the hangs of bug 514148 won't depend on users updating FF.

I don't think we should ship TB3 with its own code capable of hanging external apps.

Comment 9

[Daniel Veditz [:dveditz]]

This isn't going to make 1.9.1.4 (we have builds already). When is Thunderbird shipping off that branch?

Comment 10

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

I don't see how this is blocking 1.9.1. There are no comments here from any of the Thunderbird drivers saying this is wanted or needed. The patch is nowhere near ready (it's been minused and no work has been done since it was minused). Not blocking. If Thunderbird drivers decide they need this, they can re-nominate, though I'd expect some work to be done in this bug before that happens.

Comment 11

[David :Bienvenu]

The fix for bug 514148 landed in 1.9.1.4, so upgraded versions of Firefox will be OK - I think that's sufficient as far as Thunderbird is concerned (though we'd really like to have the fix in 1.9.2 as well)

Comment 12

[Phil Lacy]

Attachment: patched to trunk

request re-review. This is top of a series of patches enabling various drag and drop flavors.
This patch corrects code that assigns a windows clip_format (CF_HDROP) to mozilla flavors but the GetData for the format is not implemented and has caused FF to freeze, and may cause problems to other apps that are expecting data.

Comment 13

[Phil Lacy]

Comment on attachment 412356 [details] [diff] [review]
patched to trunk

Jim, can you advise a reviewer on a string of patches, starting with this one.
They enable drag and drop to external files, drag and drop of files, and corrects some bugs in nsDataObj

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> (From update of attachment 412356 [details] [diff] [review])
> Jim, can you advise a reviewer on a string of patches, starting with this one.
> They enable drag and drop to external files, drag and drop of files, and
> corrects some bugs in nsDataObj

Sorry for the delay, I'll get to this as soon as we finish up with the 1.9.2 freeze.

Comment 15

[Jim Mathies [:jimm]]

Why did you drop the review? Are you working up a new patch?

Comment 16

[Phil Lacy]

No, I thought I should revisit it.
If you get a chance to look it over I could use your opinion, otherwise it's not breaking anything currently.

Comment 17

[Phil Lacy]

Comment on attachment 412356 [details] [diff] [review]
patched to trunk

my other comments helped me keep track of this bug, this comment pretty much is what it boils down to. I'll go back in for your review as this is still valid bug even if it isn't breaking anything currently.

The first hunk removes registering support of CF_HDROP with windows because we don't support it in GetData.
Bug 484667 adds support back in for CF_HDROP and creates a temp file like use to occur.

The second hunk registers the windows formats necessary for stream transfer of data which is what we support for kFilePromiseMime. Currently those are registered under x-moz-url flavor.

The diff cuts off the lead-in line:
"else if ( strcmp(flavorStr, kFilePromiseMime) == 0 ) {"

Comment 18

[Phil Lacy]

Comment on attachment 412356 [details] [diff] [review]
patched to trunk

r+ of bug 484667 make the first part of this patch pointless since it enables kFilePromiseMime to handle CF_HDROP

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.