1071562 - [e10s] Support non-text content types for content process clipboard

Status: RESOLVED FIXED

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

Description

[bull500]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140922030204

Steps to reproduce:

1. Open either of these links:
http://hotgeekz.com/wp-content/uploads/2014/01/BFHelmet.jpg
http://static.comicvine.com/uploads/original/11/116475/4035926-boba+fett.png

2. Right click -> Copy image.
3. Try paste in Gimp - Image not copied to clipboard

Note: e10s only, works perfect in non-e10s window


Actual results:

Image not getting copied to clipboard


Expected results:

image should have copied and user must be able to paste elsewhere

Comment 1

[Chris Peterson [:cpeterson]]

This image copy bug might be related to or a dupe of drag/drop bug 936092.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, no, this is separate. When I added clipboard support, I only made it work for text.

Comment 5

[Armen [:armenzg]]

Good to see this having ownership! Thanks!
I have to switch to Chrome every time I need to copy/paste in Google Docs.

Comment 6

[Allison Naaktgeboren :ally]

No pressure. :p We're polishing off the m4 milestone and moving onto m5 bugs, which this is part of. We didn't forget it, I promise.

Comment 7

[Allison Naaktgeboren :ally]

Attachment: expandClipboardOptions wip

Comment 8

[Allison Naaktgeboren :ally]

code in wip when run produces expected output. However, when the message manager sends cmd_copyImage, the child never gets the message, unlike cmd_copy which arrives in the child just fine.

Comment 9

[Allison Naaktgeboren :ally]

gets msg in nsDocShell::DoCommand *

Comment 10

[Allison Naaktgeboren :ally]

>	xul.dll!nsXULPopupManager::GetLastTriggerNode(nsIDocument * aDocument, bool aIsTooltip) Line 1547	C++
 	xul.dll!nsXULPopupManager::GetLastTriggerPopupNode(nsIDocument * aDocument) Line 510	C++
 	xul.dll!nsDocumentViewer::GetPopupNode(nsIDOMNode * * aNode) Line 3410	C++
 	xul.dll!nsDocumentViewer::GetPopupImageNode(nsIImageLoadingContent * * aNode) Line 3469	C++
 	xul.dll!nsDocumentViewer::GetInImage(bool * aInImage) Line 3524	C++
 	xul.dll!nsClipboardImageCommands::IsClipboardCommandEnabled(const char * aCommandName, nsIContentViewerEdit * aEdit, bool * outCmdEnabled) Line 680	C++
 	xul.dll!nsSelectionCommand::IsCommandEnabled(const char * aCommandName, nsISupports * aCommandContext, bool * outCmdEnabled) Line 582	C++
 	xul.dll!nsControllerCommandTable::IsCommandEnabled(const char * aCommandName, nsISupports * aCommandRefCon, bool * aResult) Line 102	C++
 	xul.dll!nsBaseCommandController::IsCommandEnabled(const char * aCommand, bool * aResult) Line 105	C++
 	xul.dll!nsDocShell::IsCommandEnabled(const char * inCommand, bool * outEnabled) Line 12899	C++

For those playing along at home, this is as deep as the stack gets for cmd_copyImage.
GetLastTriggerNode has 
nsMenuChainItem* item = aIsTooltip ? mNoHidePanels : mPopups;
item --> null.
parameter aIsTooltip is false. mNoHidePanels & mPopups are both NULL.

Comment 11

[Allison Naaktgeboren :ally]

mNoHidePanels & mPopups are only set to nonnull values in nsXULPopupManager::ShowPopupCallback()

Comment 12

[Allison Naaktgeboren :ally]

For those playing along at home:

: the parent process's nsXULPopupManager has state matching single process nsXULPopupManager. However, the child receives none of that state, specifically around mPopups

:the only thing used in nsXULPopupManager  is getLastTriggerPopupNode, which is consumed by XULDocument.cpp & nsDocumentViewer only.

Sending just the event doesn't cause the nsXULPopupManager to update state in the child

To make this work at the nsXULPopupManager level, we'd need to keep the two copies in synch. However, code is triggered on every click, and we cannot afford a synchronous msg with every mouse click.

New plan is to route around the nsXULPopupManager entirely in nsDocumentViewer and try to find an acceptable node in nsDocumentViewer.

Comment 13

[Allison Naaktgeboren :ally]

Spent some time hacking around on this and trying different things, however none of them to seem to pan out around the popupnode.

Spent some more time talking to dbaron & khuey about documentviewer. What we're actually looking for is the node that the popup was rooted in, not the popup node itself(the node we right clicked on), which in single process the popup system keeps track of. Since the child doesn't get any of that we're going to have to find another place to store & access that information in the child.

Comment 14

[Allison Naaktgeboren :ally]

Attachment: hacking around

Largely in case Jim wants to play with it or view the inline notes.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just a note that bug 936092 has some code for dealing with transferables that would probably be useful here. It sounds like that's not the main problem though.

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Bill McCloskey (:billm) from comment #15)
> Just a note that bug 936092 has some code for dealing with transferables
> that would probably be useful here. It sounds like that's not the main
> problem though.

I had a solid look. It'll be helpful once we can get that far. First I have to convince firefox that we can execute the command, preferably while supplying the root node properly.

Comment 17

[Allison Naaktgeboren :ally]

progress: The code progresses to nsDocShell::DoCommand (means the check is now passing). However controller->DoCommand() fails, so onto the next problem.

Comment 19

[Allison Naaktgeboren :ally]

Enn,
This bug hinges on the ability of the child process to find the parent root node of the popup(to figure out if it can be copied, to actually find the thing to copy, etc).

In single process firefox that information is managed by the Popup node manager and updated every mouse move. In multiprocess, the popups are in the parent process, so the xul popup node manager in the child is always empty and therefore the child thinks there is never anything valid to copy. If I lie to the child, it is still unable to copy because of the missing information.

In particular, I have been looking at cmd_CopyImage, whose commandEnabled's stack eventually ends up in 

nsDocumentViewer::GetInImage(bool* aInImage)

which fails on 

nsresult rv = GetPopupImageNode(getter_AddRefs(node)); 

which boils down to a call to 

nsXULPopupManager::>GetLastTriggerPopupNode()


What is the best way to reconcile dependencies on the nsXULPopupManager with multiprocess?

Comment 20

[Neil Deakin]

Bug 1058712 already has a patch for copy image, so the best way is to just apply that patch. :)

This bug is supposed to be for implementing the copying of images across processes and onto the clipboard. I think smaug may have done something that can be built on top of in bug 936092 but I haven't looked at the patches there much yet.

Comment 22

[Neil Deakin]

Bill, are you working on this? I have a rough patch that handles copying images to the clipboard.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I was waiting for the drag and drop code to land. Please feel free to take this if you want.

Comment 24

[Neil Deakin]

Attachment: Work in progress

This patch allow copy and paste of text and html, any other text type and the copying of images. It doesn't yet support image pasting.

Comment 25

[Neil Deakin]

Several ways to test this once complete:

1. Open data:text/html,<div contenteditable="true">Test <b>Bold</b> Test</div>
  Then, select some of bold text, copy it, then paste it in the same field. The bold text should be maintained.
2. When bug 1058712 is done, copy image from Firefox and then into the same data url from 1
3. Copy image from Chrome and then into the same area.

(3 does not yet work with the attached patch.)

Comment 26

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

(one can also test by copying from non-e10s FF window)

Comment 27

[Neil Deakin]

Attachment: Support non-text copy/paste

This builds on top of the drag and drop patch in bug 936092.

Two things I'm not sure about:

1. I added nsCString to IPCDataTransferData to handle images, but I'm not sure what the best approach is here. Is a blob better here and how would I go about creating it?
2. nsContentUtils::TransferableToIPCTransferable also handles files but I haven't tested this yet. There's limited support for cut/paste of files in the widget code.

Comment 28

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

1. Blob might be better, since I guess image data can be large. (dnd might want to use similar setup too in some cases)
File::CreateMemoryFile perhaps?
(Yes, it is odd that File implements Blob, but http://mxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl#18)


2. sounds like something to test. 
nsContentUtils::TransferableToIPCTransferable should just deal with Files, if such are being passed around.

looking at the patch...

Comment 29

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

Comment on attachment 8570459 [details] [diff] [review]
Support non-text copy/paste

>@@ -0,0 +1,126 @@
>+// This test is used to check copy and paste in editable areas to enusre that non-text
ensure

>+// types (html and images) and copied to and pasted from the clipboard properly.
s/and/are/ ?
>+          // Images to be pasted on the clipboard are nsIInputStreams
>+          nsCOMPtr<nsIInputStream> stream(do_QueryInterface(data));
>+          if (stream) {
>+            IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement();
>+            item->flavor() = nsCString(flavorStr);
>+
>+            nsCString imageData;
>+            NS_ConsumeStream(stream, UINT32_MAX, imageData);
>+            item->data() = imageData;
>+            continue;
Ah this way. I think we may want to have a new type of Blob implementation which can take inputstream as a param.
Want to ask baku about it? But perhaps we could do that in a followup.


>+            mozilla::RefPtr<mozilla::gfx::DataSourceSurface> dataSurface =
>+              surface->GetDataSurface();
>+            mozilla::gfx::DataSourceSurface::MappedSurface map;
>+            dataSurface->Map(mozilla::gfx::DataSourceSurface::MapType::READ, &map);
>+            mozilla::gfx::IntSize size = dataSurface->GetSize();
>+            mozilla::CheckedInt32 requiredBytes =
>+              mozilla::CheckedInt32(map.mStride) * mozilla::CheckedInt32(size.height);
>+            size_t bufLen = requiredBytes.isValid() ? requiredBytes.value() : 0;
>+            mozilla::gfx::SurfaceFormat format = dataSurface->GetFormat();
>+            bufLen = bufLen - map.mStride + (size.width * BytesPerPixel(format));
All this could use a comment the gfx APIs make it really hard to use the data.
We might even want to add a helper method to nsContentUtils or something to get the
image data.



>+ContentParent::RecvSetClipboard(const IPCDataTransfer& dataTransfer,
>+                                const bool& isPrivateData,
>+                                const int32_t& whichClipboard)
I wouldn't mind if you used mozilla coding style here. So, aFoo
(I know ContentParent uses all sorts of styles)

>-    // If our data flavor has already been added, this will fail. But we don't care
>-    trans->AddDataFlavor(kUnicodeMime);
>+    auto& items = dataTransfer.items();
I really would prefer the actual type name here. I have no idea about the type of
items.
(IMHO auto is great for writing code, but not at all great when one needs to read the code.)

>+      if (item.data().type() == IPCDataTransferData::TnsString) {
>+        nsCOMPtr<nsISupportsString> dataWrapper =
>+            do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv);
2 spaces is enough for indentation



>+        const gfxIntSize size(imageDetails.width(), imageDetails.height());
>+        if (!size.width || !size.height)
>+          return true;
always {} with 'if'



>+ContentParent::RecvClipboardHasType(nsTArray<nsCString>&& types,
>+                                    const int32_t& whichClipboard,
>+                                    bool* hasType)
aFoo
Same also elsewhere.

> union IPCDataTransferData
> {
>   nsString;
>+  nsCString;
Perhaps add a comment that nsCString is for image data.



Looks surprisingly simple to me :)

Comment 30

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #29)
> Ah this way. I think we may want to have a new type of Blob implementation
> which can take inputstream as a param.
> Want to ask baku about it? But perhaps we could do that in a followup.
> 

Ah, there's a patch for this in bug 891247 (patch version 3). I'll look into adapting it.


> >-    // If our data flavor has already been added, this will fail. But we don't care
> >-    trans->AddDataFlavor(kUnicodeMime);
> >+    auto& items = dataTransfer.items();
> I really would prefer the actual type name here. I have no idea about the
> type of
> items.

I just copied this from your code :)

Comment 31

[Neil Deakin]

baku, see comment 29. Essentially, we want to transfer the image data stored in an nsIInputStream and send it to the child process. What is the best way to do this?

Going the other way, we currently send images from child to parent using a nsCString.

Comment 32

[Andrea Marchesini [:baku]]

We are going to have something for Fetch. This is not landed yet. I'll leave this NI so that when that new feature lands, I remember to write a new comment here. Definitely a follow up.

Comment 33

[Neil Deakin]

Attachment: Updated patch

Comment 34

[Neil Deakin]

Attachment: Extra patch - fix html copy on Windows

Windows is using single-byte strings for html so it needs to be supported.

This patch is included in the other one. I just pulled these changes out to make them easier to review.

Comment 35

[Neil Deakin]

Attachment: Extra patch - merge both surface data handling cases

This patch, (also included in the main one), combines the two places where the image surface data is taken into a utility function.

Comment 36

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

Comment on attachment 8574667 [details] [diff] [review]
Extra patch - fix html copy on Windows

So many different hacks and inconsistencies in our Transferable handling :/

Comment 37

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

Comment on attachment 8574669 [details] [diff] [review]
Extra patch - merge both surface data handling cases

Should the method return const uint8_t* to hint about the ownership?
And please document whether caller should free the return value.

Why we need nsCString dragImage(dragImagePart); in nsDragServiceProxy.cpp?

Comment 39

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a040986845d

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/40bc31482401

https://treeherder.mozilla.org/logviewer.html#?job_id=8711312&repo=mozilla-inbound

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Looks like it angered mochitest-e10s as well.
https://treeherder.mozilla.org/logviewer.html#?job_id=8716053&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=8717599&repo=mozilla-inbound

Comment 43

[Neil Deakin]

Attachment: Part 1 - support non-text types for clipboard

This is the original patch as checked in.

Comment 44

[Neil Deakin]

Attachment: Part 2 - fix widget build issues

Comment 45

[Neil Deakin]

Attachment: Part 3 - fix assertions in drag code that doesn't handle nsCStrings.

Comment 46

[Neil Deakin]

Comment on attachment 8591822 [details] [diff] [review]
Part 3 - fix assertions in drag code that doesn't handle nsCStrings.

This fixes the assertion.

Comment 47

[Neil Deakin]

Comment on attachment 8591819 [details] [diff] [review]
Part 2 - fix widget build issues

Try shows no build or test errors but I've no idea how to test this manually. Hopefully someone can test this out:

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

Picking the same reviewer as the original code here.

Comment 48

[Dave Townsend [:mossop]]

Looks like we have reviews here, are we still waiting on something to land this?

Comment 49

[Neil Deakin]

I need to update it for bug 1153613.

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/802512ee57ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/e812687a81cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace1964512f5

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/802512ee57ae
https://hg.mozilla.org/mozilla-central/rev/e812687a81cd
https://hg.mozilla.org/mozilla-central/rev/ace1964512f5