Closed Bug 1287823 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P1)

48 Branch
All
Linux
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox52 --- verified

People

(Reporter: pauly, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fce-active-legacy])

Attachments

(1 file, 1 obsolete file)

[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
Summary: Dragging & dropping a download from the downloads panel on desktop saves a txt file → [Linux] Dragging & dropping a download from the downloads panel on desktop saves a txt file
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.
Flags: needinfo?(enndeakin)
Duplicate of this bug: 1294393
Hi Neil,
Do you have any updates/fixes for this one?
Flags: needinfo?(enndeakin)
Version: Trunk → 48 Branch
Neil, can you try and get this for 50?
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
Flags: needinfo?(enndeakin)
Attached patch When a file exists, use its uri (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8787470 - Flags: review?(karlt)
Whiteboard: [fce-active]
Karl, do you think you'll be able to review this regression soon-ish? If not, could you suggest another reviewer?
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8787470 - Flags: review?(karlt)
> 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.
Priority: -- → P1
(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"
Attached patch dragfileasuriSplinter Review
Attachment #8787470 - Attachment is obsolete: true
Attachment #8802470 - Flags: review?(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?
Flags: needinfo?(enndeakin)
Attachment #8802470 - Flags: review?(karlt) → review+
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
https://hg.mozilla.org/mozilla-central/rev/31ccffb49e72
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi Paul,
Can you help verify if this is fixed as expected?

Hi Neil,
Do you think this is worth uplifting to Beta51?
Flags: needinfo?(paul.silaghi)
Verified fixed FX 52.0a2 (2016-11-29) Ubuntu 16.04
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
> "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.
Flags: needinfo?(enndeakin)
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.