1183915 - [e10s] Can't drag and drop some (mostly https?) images into some applications with e10s on

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This is an odd problem that I'm not sure how to diagnose.

1. Load an application that allows dragging images from Firefox to the application.

At this stage, the only one I know of is Anki (http://ankisrs.net/). If you use it and create a new card, then you can drag images directly from Firefox into the text area and it displays as an image.

2. Load https://www.w3.org/Icons/w3c_home in Firefox with e10s turned ON
3. Drag the image onto the application in (1)

ER: Image is displayed (or whatever the application is supposed to do with it)
AR: Nothing happens

*However*, if you do the same thing with other resources it does work.

Resources that DON'T work:
https://www.w3.org/Icons/w3c_home
http://www.w3.org/Icons/w3c_home
https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQFUlCXdGtPXNnBdSj4zxMHOuKBrRl9slCtvZn6qFYZnMCJ2BsfhzsOZIQ7

Resources that DO work:
http://www.smh.com.au/content/dam/images/3/z/a/4/i/image.related.articleLeadwide.300x0.3za4h.11xli1.png/1436914211883.jpg
http://ankisrs.net/img/anki-logo.png

At first I thought https was the determining factor but I notice that (http://www.w3.org/Icons/w3c_home) doesn't seem to work.

Note that all resources work with e10s turned off.

Comment 1

[Neil Deakin]

We probably need to support images in TabParent::RecvInvokeDragSession and ContentChild::RecvInvokeDragSession. We can use similar code as ContentParent::RecvSetClipboard.

Comment 3

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

This looks like a possible regression related to e10s; nominating for e10s triage. 
Brian, are you still able to reproduce the bug?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> This looks like a possible regression related to e10s; nominating for e10s
> triage. 
> Brian, are you still able to reproduce the bug?

Yes. I can reproduce this on current Nightly with e10s enabled, but not with e10s disabled.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I'll look into this tomorrow.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

With smaug's help on IRC, I have a plan to fix this. Hopefully I'll have a patch tomorrow.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch 1 - Formatting and style nits

The code to do images in the clipboard case was very badly indented and wrapped. I fixed up the indentation and wrapping (as well as adding what appeared to be a missing & in one case).

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch 2 - Refactor imgIContainer creation code

Now that both the clipboard and D&D code will have to create imgIContainers from an IPCDataTransferItem, I moved this code into a helper in nsContentUtils so it could be used from both ContentParent and TabParent.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch 3 - Make the D&D code image-aware

With this patch, the simpler version of IPCDataTransferItem (called DataTransferItem) merged with the more complex version in order to hold more image data. While fixing this, I fixed an apparent bug in TabParent::RecvInvokeDragSession where it checks for IPCDataTransferItem::PBlobChild instead of PBlobParent. The code stick an imgIContainer directly in the variant in order to follow the example of nsContentAreaDragDropDataProvider.

With this patch, I'm able to drag an image and drop it in iTunes, which is the STR from bug 1248140. I'll need someone to test on Windows to verify it there.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: rolled-up patch

Olli, this is a rolled-up patch in case you prefer to review everything at once.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8721004 [details] [diff] [review]
Patch 1 - Formatting and style nits

># HG changeset patch
># User Blake Kaplan <[e-mail]>
>
>Bug 1183915 - Fix formatting and style nits.
>
>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 25c79fa..c779ba6 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp
>@@ -2809,85 +2809,87 @@ bool
> ContentParent::RecvSetClipboard(const IPCDataTransfer& aDataTransfer,
>                                 const bool& aIsPrivateData,
>                                 const int32_t& aWhichClipboard)
> {
>   nsresult rv;
>   nsCOMPtr<nsIClipboard> clipboard(do_GetService(kCClipboardCID, &rv));
>   NS_ENSURE_SUCCESS(rv, true);
> 
>-  nsCOMPtr<nsITransferable> trans = do_CreateInstance("@mozilla.org/widget/transferable;1", &rv);
>+  nsCOMPtr<nsITransferable> trans =
>+    do_CreateInstance("@mozilla.org/widget/transferable;1", &rv);
>   NS_ENSURE_SUCCESS(rv, true);
>   trans->Init(nullptr);
> 
>   const nsTArray<IPCDataTransferItem>& items = aDataTransfer.items();
>-  for (uint32_t j = 0; j < items.Length(); ++j) {
>-    const IPCDataTransferItem& item = items[j];
>-
>+  for (const auto& item : items) {
So this makes the code actually harder to read (because of IPCDataTransferItem& -> auto& change) and in general unsafer since range-for just breaks immediately if one modifies the array in any way.
But ok, in this case I can live with auto. This is close to the iterator use case where auto isn't totally horrible.
Not that I understand this change at all ;)


Hmm, odd style in the existing code. I wonder what has happened there.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8721012 [details] [diff] [review]
Patch 3 - Make the D&D code image-aware

> TabParent::AddInitialDnDDataTo(DataTransfer* aDataTransfer)
> {
>   for (uint32_t i = 0; i < mInitialDataTransferItems.Length(); ++i) {
>-    nsTArray<DataTransferItem>& itemArray = mInitialDataTransferItems[i];
>-    for (uint32_t j = 0; j < itemArray.Length(); ++j) {
>-      DataTransferItem& item = itemArray[j];
>+    nsTArray<IPCDataTransferItem>& itemArray = mInitialDataTransferItems[i];
>+    for (auto& item : itemArray) {
>       RefPtr<nsVariantCC> variant = new nsVariantCC();
>       // Special case kFilePromiseMime so that we get the right
>       // nsIFlavorDataProvider for it.
>-      if (item.mFlavor.EqualsLiteral(kFilePromiseMime)) {
>+      if (item.flavor().EqualsLiteral(kFilePromiseMime)) {
>         RefPtr<nsISupports> flavorDataProvider =
>           new nsContentAreaDragDropDataProvider();
>         variant->SetAsISupports(flavorDataProvider);
>-      } else if (item.mType == DataTransferItem::DataType::eString) {
>-        variant->SetAsAString(item.mStringData);
>-      } else if (item.mType == DataTransferItem::DataType::eBlob) {
>-        variant->SetAsISupports(item.mBlobData);
>+      } else if (item.data().type() == IPCDataTransferData::TnsString) {
>+        variant->SetAsAString(item.data().get_nsString());
>+      } else if (item.data().type() == IPCDataTransferData::TPBlobParent) {
>+        auto* parent = static_cast<BlobParent*>(item.data().get_PBlobParent());
>+        RefPtr<BlobImpl> impl = parent->GetBlobImpl();
>+        variant->SetAsISupports(impl);
>+      } else if (item.data().type() == IPCDataTransferData::TnsCString &&
>+                 nsContentUtils::IsFlavorImage(item.flavor())) {
>+        // An image! Get the imgIContainer for it and set it int eh variant.
the

>-  nsTArray<nsTArray<DataTransferItem>> mInitialDataTransferItems;
>+  nsTArray<nsTArray<IPCDataTransferItem>> mInitialDataTransferItems;
Took a bit time to figure out this is actually safe. But I think it is. PBlob implementation isn't refcounted on either side or anything like that

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87c6d3d8d91c65cfd764408e0c7958fe8ee84a9
Bug 1183915 - Put images dragged from content processes in the drag data in the parent. r=smaug

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d87c6d3d8d91

Comment 16

[Chris Peterson [:cpeterson]]

Blake, should we uplift this D&D fix to Beta 46 for our e10s experiment?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8721013 [details] [diff] [review]
rolled-up patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Users won't be able to drop images into certain applications.
[Describe test coverage new/current, TreeHerder]: No good treeherder coverage. This has been baking on trunk for two weeks with no reports of problems.
[Risks and why]: This is medium risk in that there are no automated tests and the code is very platform-specific. That being said, I think it's worth it to uplift this patch as it can be very annoying to not be able to drag and drop images.

Comment 18

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

Jim, given the medium risk associated with this patch, I'd like to get your opinion on whether we should take this in Aurora47 or not. Please let me know. Thanks!

Comment 19

[Chris Peterson [:cpeterson]]

We are unlikely to ship e10s to GA with FF 46. We plan to run an e10s experiment on Beta 46, so we'd like e10s experimenters to have fixes for known e10s issues. But if this is a medium risk patch, we can probably do without this drag & drop fix if it won't improve the e10s metrics we'll be monitoring (like stability or performance).

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8721013 [details] [diff] [review]
rolled-up patch

As intrusive as this is, lets limit this to 47.

Comment 21

[Chris Peterson [:cpeterson]]

status-firefox46=wontfix as per Jim's comment 20.