1308007 - [e10s] Pasted file objects not correctly transmitted to content process

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

[Tracking Requested - why for this release]:


+++ This bug was initially created as a clone of Bug #1305163 +++

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/ea104eeb14cc54da9a06c3766da63f73117723a0
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 ID:20161005030211

Problem occurs local image on Beta50.0b4, Aurora51.0a2 and Nightly52.0a2.

Steps to reproduce:
1. Open local image with "Windows Photo Viewer"/"Photo"
2. Right clock on the image and "Copy"
3. Open http://www.martinezdelizarrondo.com/ckplugins/simpleuploads.demo4/
4. Click the editor
5. Attempt paste it (Ctrl+v)


Actual results:
  No image is created


Expected results:
  Image should be created

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2409b36cd07bba045bf38dd131166c36ea3da4a&tochange=3d0aef4586bdbd81591811ec627c85161fc11065

Suspect: Bug 906420

Comment 2

[Alfonso Martinez]

I've tested with 51.0a2 (2016-10-07) (32-bit) and the issue is that the event clipboardData object has a "items" property, a DataTransferItemList with one item

That item has .kind == "file", but calling .getAsFile() on it returns null and I didn't expect that so the code crashes.

I've adjusted the code to ignore that bad file contents and now the code doesn't crash and the image is pasted due to the rest of the detections. 

But so it turns out that now it's no longer possible no copy a file from Explorer (like a .zip or .pdf) and paste it in the editor to upload it directly (this still works in 50.0b4)

Disabling e10s makes the paste work correctly again

Comment 3

[Jet Villegas (inactive)]

[Platform Triage] To Michael for a look.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Files are not correctly transmitted over ipc, due to http://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7804-7821. We need some mechanism for being able to transmit these files synchronously over IPC such that the clipboard copy-paste functionality works correctly with files when e10s is enabled.

Comment 7

[Marcia Knous [:marcia]]

Tracking 52+ for this regression - copy and paste seems to be a common operation. I also carried over the 51+ flag from the duplicate bug.

Comment 8

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

Tracked in 50 as it's a new regression.

Comment 9

[Andrew Overholt [:overholt]]

Michael told me he's working on a workaround (which will be in bug 1306645 in the near future) but he'd rather fix *this* bug in a better way. He's concerned about major regression fallout if he does anything other than the workaround so we'll likely have to punt on a proper fix here until at least 52. Maybe Blake will come back with something which is safe to uplift but offline he's expressed concern about that being possible.

Once we have the patch on bug 1306645 I'll mark this fix-optional for 50/51 and maybe 52.

Comment 10

[Andrew Overholt [:overholt]]

Given the patch on bug 1306645 having beta uplift approval, I'm going to mark this fix-optional for 50 and 51 (since the patch which will be uplifted there has branch-specific fixes that paper over this issue).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

One possibility to try would be to somehow always have a blob half-constructed between each child and the parent process to use for this case. The reason that we do the hack pointed to by comment 4 is because trying to create a blob and use it in the return value of a synchronous message crashes when the child tries to use the blob before it has a chance to run the blob constructor (and changing the priority of the blob constructor is a definite non-starter).

Having a half-constructed blob would get around that, but I don't know if there are other pitfalls lurking there.

Comment 12

[Alice0775 White]

No longer reproduce this problem on Beta50Rc1, Aurora51.0a2 and Nightly52.0a1.

Fixed window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6787db4d131dce2eabe0fab90c0e03e531190b5&tochange=3201c03e0c16fcba25c23af79cec8b08ea174ab7

Fixed by:  3201c03e0c16	Michael Layzell — Bug 1306645 - Don't add application/x-moz-file entries from the clipboard to dataTransfer with e10s, r=enndeakin

Comment 13

[Neil Deakin]

The underlying bug hasn't been fixed, just a workaround in bug 1306645. This bug is being used for the real fix.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

This bug IIRC is caused by http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/dom/base/nsContentUtils.cpp#7981-7999. In bug 1353629 there were some changes to how the blob constructor code works. My only understanding of these changes comes from baku's post on dev-platform, but it seemed to me from my quick reading of that email that we may be able to fix this problem with the new IPCBlob refactoring.

Baku, is there any chance that you could take a look?

Comment 15

[Andrea Marchesini [:baku]]

> Baku, is there any chance that you could take a look?

I just tested it and it works. Can somebody else verify it, please?

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Blake, as you wrote this code is there any chance you could remove the check now (as it should be safe to construct blobs over sync IPC now from what I understand). I imagine you'll know better than me what other concerns we could run into from the change being made.

Comment 17

[Andrea Marchesini [:baku]]

(In reply to Michael Layzell [:mystor] from comment #16)
> Blake, as you wrote this code is there any chance you could remove the check
> now (as it should be safe to construct blobs over sync IPC now from what I
> understand). I imagine you'll know better than me what other concerns we
> could run into from the change being made.

Hold on. Creating an Blob using a sync IPC is OK Child to Parent using IPCBlob.
But parent to child it's not yet because PIPCBlobInputStream doesn't have a sync IPC CTOR.
We can change it, but maybe we can find a better solution. Let me take a closer look.

Comment 18

[Andrea Marchesini [:baku]]

We need to make the communication async. The current code doesn't resolve this issue.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrea Marchesini [:baku] from comment #18)
> We need to make the communication async. The current code doesn't resolve
> this issue.

Filed 1375022 to discuss ways to make Msg_GetClipboard async.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 21

[Daniel Bodea [:danibodea]]

While running a test case, I stumble upon this issue that reproduces in Nightly v69.0a1 (latest) on Windows 10.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This causes that editor cannot set InputEvent.dataTransfer of beforeinput and input events as expected.

Comment 23

[Tom S [:evilpie]]

Attachment: Bug 1308007 - Add a pref to allow files in DataTransfer from paste again. r?nika

Comment 24

[Tom Schuster (MoCo)]

Attachment: Bug 1308007 - Enable browser_clipboard_pastefile.js test. r?nika

Depends on D153100

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9cc66c55a13
Add a pref to allow files in DataTransfer from paste again. r=nika
https://hg.mozilla.org/integration/autoland/rev/d5c7e2162b99
Enable browser_clipboard_pastefile.js test. r=nika

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f9cc66c55a13
https://hg.mozilla.org/mozilla-central/rev/d5c7e2162b99