1169268 - [e10s] tab crashes when user paste files if [contenteditable] is around

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[arni2033]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150528004000

Steps to reproduce:

1. Open page data:text/html,<div contenteditable style="display:none">
2. Paste file from disk (simply press Ctrl+V)


Actual results:

Tab has crashed (tested on clean profile)

Comment 1

[Jim Mathies [:jimm]]

https://crash-stats.mozilla.com/report/index/1ce6399e-c360-4844-a845-401962150528

Comment 2

[Jim Mathies [:jimm]]

1 	xul.dll 	NS_DebugBreak 	xpcom/base/nsDebugImpl.cpp
2 	xul.dll 	mozilla::ipc::FatalError(char const*, char const*, unsigned long, bool) 	ipc/glue/ProtocolUtils.cpp
3 	xul.dll 	mozilla::dom::PContentChild::FatalError(char const* const) 	obj-firefox/ipc/ipdl/PContentChild.cpp
4 	xul.dll 	mozilla::dom::PContentChild::Read(mozilla::dom::IPCDataTransferItem*, IPC::Message const*, void**) 	obj-firefox/ipc/ipdl/PContentChild.cpp
5 	xul.dll 	mozilla::dom::PContentChild::Read(nsTArray<mozilla::dom::IPCDataTransferItem>*, IPC::Message const*, void**) 	obj-firefox/ipc/ipdl/PContentChild.cpp
6 	xul.dll 	mozilla::dom::PContentChild::Read(mozilla::dom::IPCDataTransfer*, IPC::Message const*, void**) 	obj-firefox/ipc/ipdl/PContentChild.cpp
7 	xul.dll 	mozilla::dom::PContentChild::SendGetClipboard(nsTArray<nsCString> const&, int const&, mozilla::dom::IPCDataTransfer*)

Comment 4

[Alice0775 White]

Pushlog
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ffd1b16f058b&tochange=4ddadc870ef6

Comment 5

[Neil Deakin]

Probably need to modify this to support copy/paste of files across processes.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

We try to support it, but it fails because we call PContent::GetClipboard (a sync message) from the child, which then tries to create a blob to send to the parent and return that blob. However, in receiving the returned blob in the child, we haven't yet processed its constructor and crash. There's no especially clean way to fix this (we could special-case constructors for objects created in sync messages, but I'm kind of scared of doing that). I'm going to cheat a lot and read the file in the parent and (if it's an image) send the raw image data down to the child. We only care about image files for copy/paste, so I think that's the fastest way to victory.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8df14dfebb

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Oops, that was the wrong bug number.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f8df14dfebb

Comment 10

[Alice0775 White]

[Tracking Requested - why for this release]:

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/0f8df14dfebb

This is wrong bug number.
typo change note, see Bug 1176447 Comment 6

Comment 11

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

Adding a tracking flag for FF41. Also setting qe-verify:+ to ensure QA team gets a chance to test the fix.

Comment 12

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

Untracking for FF41 as e10s is not enabled by default. Tracked for FF42 instead.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I stared at this for a long time and there's no obviously good solution for it. This crashes because we're in a sync message (GetClipboard) which tries to create a Blob to to pass to the child but we don't process the constructor message before trying to use the blob in the child, which crashes.

Additionally, there's currently no nsIFile implementation that's not backed by blobs. I thought about slurping the contents of the file and passing them down as a string, possibly only if it contains an image, (I'll do that in a separate patch) but that'll break non-paste consumers of nsIClipboard::GetClipboard (if those exist). We should definitely fix ths crash now, though.

The blob code is correct for the drag and drop case.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Factor out file-slurping code from editor

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Allow pasting images into HTML

This applies on top of the other patches.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 17

[Sylvestre Ledru [:Sylvestre]]

[Tracking Requested - why for this release]:
e10s is disabled in 42 too. Liz will see if she wants that to be fixed in 43 or not.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Sure. Assuming this affects 44 as well, we can track it there too.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

These crash sigs do show up for 44 so I'll assume so.

Comment 20

[Neil Deakin]

Comment on attachment 8668149 [details] [diff] [review]
Allow pasting images into HTML

>           // Otherwise, handle this as a file.
>           nsCOMPtr<BlobImpl> blobImpl;
>           nsCOMPtr<nsIFile> file = do_QueryInterface(data);
>           if (file) {
>+            // If we can send this over as a blob, do so. Otherwise, we're
>+            // responding to a sync message and the child can't process the blob
>+            // constructor before processing our response, which would crash. In
>+            // that case, hope that the caller is nsClipboardProxy::GetData,
>+            // called from editor and send over images as raw data.

The caller might not be the editor after bug 906420. Is that a problem?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Neil Deakin from comment #20)
> The caller might not be the editor after bug 906420. Is that a problem?

It shouldn't be a problem. I was thinking more in terms of compatibility because editor only cares if the file is an image (and if so it reads it synchronously and pastes the image into the document). Other callers might actually want to use the file.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36fd2c273344

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36fd2c273344

Comment 24

[Ada [:adalucinet]]

Reproduced with Nightly (from 2015-10-03): bp-71b65e8c-c01f-4646-9410-687f62151118
With latest 44.0a2 and 45.0a1 (from 2015-11-17) and the provided STR, I constantly hit crash bug 1225829.
Any ideas?

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 43, as e10s isn't enabled on beta yet.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

I just checked in a fix for bug 1225829.

Comment 27

[Ada [:adalucinet]]

No tab/browser crashes encountered with latest 46.0a1 (from 2015-12-27), across platforms [1].

[1] Windows 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.10.5