1287823 - [Linux] Dragging & dropping a download from the downloads panel on desktop saves a txt file

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Paul Silaghi, QA [:pauly]]

[Affected versions]:
- 48b9, 50.0a1 (2016-07-19)

[Affected platforms]:
- Ubuntu 16.04

[Steps to reproduce]:
1. Open google.com
2. Search for something and save an image from the results
3. Open the downloads panel and drag the saved image to desktop

[Expected result]:
- Image saved to desktop properly

[Actual result]:
- Text file saved on desktop containing the local path to the image

[Regression range]:
- TBD, doesn't reproduce on 47.0.1

[Additional notes]:
- seems to be reproducible only on Linux

Comment 1

[Paul Silaghi, QA [:pauly]]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1b2c3cc8c06f7e257a8040c04086a99889356b2d&tochange=2747cac2de2fcfd3233930a10e4fa560b3376b60

Neil, one of your patches.

Comment 2

[Neil Deakin]

Yes this is because Linux uses text/uri-list directly which we don't expose any more. Some work will be needed to ensure that this is done safely.

Comment 4

[Gerry Chang [:gchang]]

Hi Neil,
Do you have any updates/fixes for this one?

Comment 5

[David Durst [:ddurst]]

Neil, can you try and get this for 50?

Comment 6

[Neil Deakin]

There are two options here.

1. Allow text/uri-list but filter it remove any urls the source cannot access.
2. Handle file types when dragging on Linux

Comment 7

[Neil Deakin]

Attachment: When a file exists, use its uri

With this patch, when a file is present in the drag data we provide, and no uri is available, we get a uri from the file. This allows us gtk to treat the file as a text/uri-list, but doesn't expose any other parts of text/uri-list.

Eventually, we can finish up the uri checking and validation code.

Comment 8

[Andrew Overholt [:overholt]]

Karl, do you think you'll be able to review this regression soon-ish? If not, could you suggest another reviewer?

Comment 9

[Karl Tomlinson (:karlt)]

I'd be delighted to know someone who understands how everything in this file
works together or is willing to dive in and work it out, but I doubt there is
anyone, except Neil of course.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8787470 [details] [diff] [review]
When a file exists, use its uri

I think this is the right thing to do, thanks, but I have a number of
questions for which answers would help me review this.

Where was text/uri-list generated previously?

(In reply to Neil Deakin from comment #7)
> Created attachment 8787470 [details] [diff] [review]
> When a file exists, use its uri
>
> With this patch, when a file is present in the drag data we provide, and no
> uri is available, we get a uri from the file. This allows us gtk to treat
> the file as a text/uri-list, but doesn't expose any other parts of
> text/uri-list.

I presume all this will go in the commit message.

"file exists" implies a test for existence on the disk, which is not what this is about.  "file is available in drag data" perhaps.

Please say what "its uri" is "use"d for in the first line.

"This allows GTK to treat the text/uri-list as a file" for the first half of
the second sentence of the second paragraph.

To what does "doesn't expose any other parts of text/uri-list" refer?
Is there a reason why there would be a text/uri-list data item in the source
that shouldn't be exposed to GTK, or is this about something else? ...

>                         MOZ_LOG(sDragLm, LogLevel::Debug,
>                                ("adding target %s\n", target->target));
>                         targetArray.AppendElement(target);

because this will export all FlavorsTransferableCanExport().

>+                        }
>                         // Check to see if this is text/unicode.
>                         // If it is, add text/plain
>                         // since we automatically support text/plain
>                         // if we support text/unicode.
>                         if (strcmp(flavorStr, kUnicodeMime) == 0) {

Please make the last line above an else-if, to clarify that only one will
match.

>             }
>+            else {

"} else {" on one line please, when there is no need for a comment between.

>+                // If there is no uri available, but there is a file available,

"There is no uri available.  If there is a file available,"

>+                    nsCOMPtr<nsIFile> file = do_QueryInterface(data);
>+                    if (!file) {
>+                        nsCOMPtr<nsISupportsInterfacePointer> ptr(do_QueryInterface(data));

Wrap to stay within 80 columns, either after '>' or after an "=" before the
initializer.

GetTransferData() returns "Some variant of class in nsISupportsPrimitives.idl"
I don't know why the return type isn't nsISupportsPrimitive, or why it must
be an nsISupportsPrimitive.

Any idea why nsISupportsInterfacePointer is necessary at all if it need not be
used?

Is it really necessary for nsISupports objects to be "wrapped in a nsISupports
primitive so that [they are] accessible from JS"?

http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/widget/nsTransferable.cpp#289

If so, why is nsIFile not always wrapped?

http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/widget/nsITransferable.idl#132

Bug 229327 comment 8 led to modification of the documentation of
setTransferData() to permit nsIFile.

>+                        rv = NS_NewFileURI(getter_AddRefs(fileURI), file);

|rv| is set but not used.  Better not to set it.

Comment 11

[Neil Deakin]

> Where was text/uri-list generated previously?

It is set by the download panel code when a drag starts. text/uri-list is no longer exposed to the system so that a web page can not place a file uri into the drag data.


> To what does "doesn't expose any other parts of text/uri-list" refer?

I think I meant that the rest of the url items are not exposed.


> because this will export all FlavorsTransferableCanExport().

I'm not sure what this comment refers to.


GetTransferData() returns "Some variant of class in nsISupportsPrimitives.idl"
I don't know why the return type isn't nsISupportsPrimitive, or why it must
be an nsISupportsPrimitive.

> Any idea why nsISupportsInterfacePointer is necessary at all if it need not be used?

The wrappers were the way you needed to do it for JS use a long time ago. Nowadays, some code wraps it and some doesn't. Using DataTransfer always wraps it. I filed bug 1310193 to remove the wrapping when it isn't needed. For drag and drop, JS callers shouldn't be using nsTransferable anyway and should use DataTransfer instead.

Comment 12

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22af1f4b343d

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Neil Deakin from comment #11)
> > Where was text/uri-list generated previously?
> 
> It is set by the download panel code when a drag starts. text/uri-list is no
> longer exposed to the system so that a web page can not place a file uri
> into the drag data.

Preventing web pages from specifying arbitrary objects in drags sounds a good
thing, thanks.

Should the download panel no longer add the text/uri-list flavor now?
The download panel is chrome not a content web page and so it could be
permitted to specify arbitrary objects in drags, but I don't know whether
or not text/uri-list is a suitable XP MIME type.
Given Gecko has its own notation for file flavors, translating those to
uri-list for GTK seems good to me thanks.
I guess that makes the uri-list from the download panel unnecessary assuming
that just describes the file.

> > To what does "doesn't expose any other parts of text/uri-list" refer?
> 
> I think I meant that the rest of the url items are not exposed.

I'm not clear what you mean here, but if you are referring to urls from the
content web pages, then I think we can ignore that here, because this code is
not responsible for determining what content is permitted to do.

> GetTransferData() returns "Some variant of class in
> nsISupportsPrimitives.idl"
> I don't know why the return type isn't nsISupportsPrimitive, or why it must
> be an nsISupportsPrimitive.
> 
> > Any idea why nsISupportsInterfacePointer is necessary at all if it need not be used?
> 
> The wrappers were the way you needed to do it for JS use a long time ago.
> Nowadays, some code wraps it and some doesn't. Using DataTransfer always
> wraps it. I filed bug 1310193 to remove the wrapping when it isn't needed.
> For drag and drop, JS callers shouldn't be using nsTransferable anyway and
> should use DataTransfer instead.

Thank you.  Can you add a short comment on the wrapping, please?
Perhaps "Some code still wraps the file.  Bug 1310193"

Comment 14

[Neil Deakin]

Attachment: dragfileasuri

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8802470 [details] [diff] [review]
dragfileasuri

>+                        nsCOMPtr <nsIURI> fileURI;

Can you remove the space between nsCOMPtr and '<' please?

Thanks!

I think these sentences should now be removed from
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Recommended_Drag_Types#file

"If possible, you may also include the file URL of the file using both the text/uri-list and/or text/plain types. These types should be added last so that the more specific application/x-moz-file type has higher priority."

Do you agree?

Comment 16

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf953079890

Comment 17

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ef52a4b71e

Comment 18

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31ccffb49e72e3c02c39aebbb87c72cb495c7c20
Bug 1287823, When a file is available in the drag data we provide, and no uri is available, get the uri of the file and use it as the text/uri-list data, r=karlt

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31ccffb49e72

Comment 20

[Gerry Chang [:gchang]]

Hi Paul,
Can you help verify if this is fixed as expected?

Hi Neil,
Do you think this is worth uplifting to Beta51?

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52.0a2 (2016-11-29) Ubuntu 16.04

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release

Comment 23

[Neil Deakin]

> "If possible, you may also include the file URL of the file using both the
> text/uri-list and/or text/plain types. These types should be added last so
> that the more specific application/x-moz-file type has higher priority."
> 
> Do you agree?

I don't think we should change anything until we can do some more thorough checking on all platforms. Making the types more uniform is a larger project.