Closed Bug 1169268 Opened 6 years ago Closed 6 years ago

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

Categories

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

40 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
e10s m8+ ---
firefox40 --- affected
firefox41 - affected
firefox42 - wontfix
firefox43 + wontfix
firefox44 + fixed

People

(Reporter: arni2033, Assigned: mrbkap)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(3 files)

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)
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
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*)
Assignee: nobody → mrbkap
tracking-e10s: --- → m7+
OS: Windows 7 → Unspecified
Hardware: x86_64 → Unspecified
Duplicate of this bug: 1172034
Probably need to modify this to support copy/paste of files across processes.
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.
Oops, that was the wrong bug number.
Severity: normal → critical
Keywords: reproducible
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::FatalError(char const*, char const*, unsigned long, bool) | mozilla::dom::PContentChild::FatalError(char const* const) | mozilla::dom::PContentChild::Read(mozilla::dom::IPCDataTransferIt…
https://hg.mozilla.org/mozilla-central/rev/0f8df14dfebb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
[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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Target Milestone: Firefox 41 → ---
Adding a tracking flag for FF41. Also setting qe-verify:+ to ensure QA team gets a chance to test the fix.
Flags: qe-verify+
Crash Signature: , bool) | mozilla::dom::PContentChild::FatalError(char const* const) | mozilla::dom::PContentChild::Read(mozilla::dom::IPCDataTransferIt...] → , bool) | mozilla::dom::PContentChild::FatalError(char const* const) | mozilla::dom::PContentChild::Read(mozilla::dom::IPCDataTransferIt...] [@ mozalloc_abort | NS_DebugBreak | mozilla::ipc::FatalError | mozilla::dom::PContentChild::FatalError | mozilla:…
Blocks: 1181467
No longer blocks: 1181467
Untracking for FF41 as e10s is not enabled by default. Tracked for FF42 instead.
Attached patch Patch v1Splinter Review
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.
Attachment #8666976 - Flags: review?(enndeakin)
This applies on top of the other patches.
Attachment #8668149 - Flags: review?(enndeakin)
[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.
Sure. Assuming this affects 44 as well, we can track it there too.
These crash sigs do show up for 44 so I'll assume so.
Attachment #8666976 - Flags: review?(enndeakin) → review+
Attachment #8668148 - Flags: review?(enndeakin) → review+
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?
Attachment #8668149 - Flags: review?(enndeakin) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/36fd2c273344
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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?
Flags: needinfo?(mrbkap)
Wontfix for 43, as e10s isn't enabled on beta yet.
I just checked in a fix for bug 1225829.
Flags: needinfo?(mrbkap)
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.