Closed Bug 1160279 Opened 9 years ago Closed 9 years ago

[e10s] Dragging a file into an e10s tab doesn't load that file

Categories

(Core :: DOM: Events, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m8+ ---
firefox40 --- fixed

People

(Reporter: mstange, Assigned: mossop)

References

Details

Attachments

(1 file)

Steps to reproduce:
 0. Have an e10s tab open, for example this bug.
 1. Open a Finder window of a directory that contains html files.
 2. Drag such an html file over into the tab content area.

Expected results:
The html page should be loaded in the tab.

Actual results:
Nothing happens.
WFM on Linux, and I was testing also on Windows when writing the initial dnd support for e10s.
I don't have access to any OSX devices.
In other words, I'm hoping someone could help here ;)
This annoys the heck out of me so I'll try to take a quick look.
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #3)
> This annoys the heck out of me so I'll try to take a quick look.

Hi Dave,
If you don't mind, I would like to share my finding here.
It seems the problem is at [1]. In content process, I think the type of file is not nsIFile, it's FileImpl.
We just need to find a way to get the correct url from FileImpl.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/contentAreaDropListener.js#46
Attached patch patchSplinter Review
So yeah the problem is that when creating the drag session we convert everything to things that can be sent over IPC, for nsIFiles this means FileImpls. We then never convert back so contentAreaDropListener doesn't know what to do. I have no idea why this apparently works in Windows and Linux, it shouldn't from what I see.

There are two options.

One is to convert contentAreaDropListener (and any other code that handles dropping of files) to use dataTransfer.getFiles (which doesn't look like it is exposed to JS) and find some way to get a url from a File (I can't see one right now). This is probably the right long-term solution given sandboxing.

The alternative is just to convert FileImpls back to nsIFiles when setting up the content side drag session. This patch does that and seems like a good quick fix.

What do you think smaug?
Flags: needinfo?(dtownsend)
Attachment #8602377 - Flags: review?(bugs)
(In reply to Dave Townsend [:mossop] from comment #5)
> I have no idea why this apparently works in Windows and
> Linux, it shouldn't from what I see.

We have totally crazy setup for dnd. Platform specific code all over, and the best example is that
Windows backend relies on certain OSX specific "flavor" to be in the transferable, but doesn't actually use it for anything.

At least on Linux dnd code tends to rely on urls, not files, IIRC.

Will look at the patch after lunch or so.


DataTransfer::GetFiles() is very much exposed to scripts:
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/DataTransfer.webidl#28
Comment on attachment 8602377 [details] [diff] [review]
patch

Could you perhaps just make contentAreaDropListener to use normal
blobs, or maybe event dataTransfer.files?

I guess we should convert dom::FileImpl to dom::File in 
DataTransfer::MozGetDataAt, and then contentAreaDropListener could just
use normal DOM File, and not nsIFile.
DataTransfer::GetFiles does similar conversion.

DOM File has mozFullPath for chrome JS usage, 
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl

"application/x-moz-file" is rarely used, and looks like in case of dnd only contentAreaDropListener in e10s/childprocess.


(I think for sandboxing purposing using raw nsIFile in child process may become a bit difficult.)
Attachment #8602377 - Flags: review?(bugs) → review-
Comment on attachment 8602377 [details] [diff] [review]
patch

Sounds like we don't have any sane way to convert path to URI without nsIFile, so
let's take this for now.
Attachment #8602377 - Flags: review- → review+
Filed bug 1162775 for the follow-up work.

https://hg.mozilla.org/integration/fx-team/rev/b62e2d74176f
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/b62e2d74176f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1163375
Backed out for causing bug 1163375. Will do the real fix in bug 1162775.
Depends on: 1162775
https://hg.mozilla.org/integration/fx-team/rev/8d78c47be72c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Fixed by bug 1162775
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 1168346
No longer depends on: 1168346
QA Whiteboard: [good first verify][verify in Nightly only]
You need to log in before you can comment on or make changes to this bug.