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

RESOLVED FIXED in Firefox 40



5 years ago
2 years ago


(Reporter: bullionareboy, Assigned: enndeakin)


35 Branch
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify ?

Firefox Tracking Flags

(e10sm5+, firefox40 fixed)



(3 attachments, 7 obsolete attachments)

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:

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
This image copy bug might be related to or a dupe of drag/drop bug 936092.
Blocks: core-e10s
tracking-e10s: --- → ?
Component: Untriaged → File Handling
Depends on: 936092
Oh, no, this is separate. When I added clipboard support, I only made it work for text.
Ever confirmed: true
Summary: [e10s] copy image not copying image to clipboard → [e10s] Support non-text content types for content process clipboard
No longer depends on: 936092
Blocks: 1058712
Flags: firefox-backlog+
Duplicate of this bug: 1098388
Assignee: nobody → ally
Good to see this having ownership! Thanks!
I have to switch to Chrome every time I need to copy/paste in Google Docs.
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.
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.
gets msg in nsDocShell::DoCommand *
>	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.
mNoHidePanels & mPopups are only set to nonnull values in nsXULPopupManager::ShowPopupCallback()
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.
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.
Posted patch hacking around (obsolete) — Splinter Review
Largely in case Jim wants to play with it or view the inline notes.
Attachment #8534757 - Attachment is obsolete: true
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.
(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.
progress: The code progresses to nsDocShell::DoCommand (means the check is now passing). However controller->DoCommand() fails, so onto the next problem.
Duplicate of this bug: 1119343
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 


What is the best way to reconcile dependencies on the nsXULPopupManager with multiprocess?
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
Assignee: ally → nobody
Blocks: 1122121
Assignee: nobody → wmccloskey
Component: File Handling → DOM: Core & HTML
Product: Firefox → Core
Duplicate of this bug: 1134012
Bill, are you working on this? I have a rough patch that handles copying images to the clipboard.
I was waiting for the drag and drop code to land. Please feel free to take this if you want.
Assignee: wmccloskey → enndeakin
Points: --- → 8
Posted patch Work in progress (obsolete) — Splinter Review
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.
Attachment #8539570 - Attachment is obsolete: true
Iteration: --- → 39.1 - 9 Mar
Flags: qe-verify?
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.)
(one can also test by copying from non-e10s FF window)
Posted patch Support non-text copy/paste (obsolete) — Splinter Review
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.
Attachment #8568642 - Attachment is obsolete: true
Attachment #8570459 - Flags: review?(bugs)
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 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

>+// 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
(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)
Same also elsewhere.

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

Looks surprisingly simple to me :)
Attachment #8570459 - Flags: review?(bugs) → review+
(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 :)
Blocks: 936092
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.
Flags: needinfo?(amarchesini)
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.
Posted patch Updated patch (obsolete) — Splinter Review
Attachment #8570459 - Attachment is obsolete: true
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.
Attachment #8574667 - Flags: review?(bugs)
This patch, (also included in the main one), combines the two places where the image surface data is taken into a utility function.
Attachment #8574669 - Flags: review?(bugs)
Comment on attachment 8574667 [details] [diff] [review]
Extra patch - fix html copy on Windows

So many different hacks and inconsistencies in our Transferable handling :/
Attachment #8574667 - Flags: review?(bugs) → review+
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?
Attachment #8574669 - Flags: review?(bugs) → review+
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
No longer blocks: 936092
Depends on: 936092
Duplicate of this bug: 1142813
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Duplicate of this bug: 1153719
This is the original patch as checked in.
Attachment #8574666 - Attachment is obsolete: true
Attachment #8574667 - Attachment is obsolete: true
Attachment #8574669 - Attachment is obsolete: true
Comment on attachment 8591822 [details] [diff] [review]
Part 3 - fix assertions in drag code that doesn't handle nsCStrings.

This fixes the assertion.
Attachment #8591822 - Flags: review?(bugs)
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:


Picking the same reviewer as the original code here.
Attachment #8591819 - Flags: review?(fabrice)
Attachment #8591819 - Flags: review?(fabrice) → review+
Attachment #8591822 - Flags: review?(bugs) → review+
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Looks like we have reviews here, are we still waiting on something to land this?
Flags: needinfo?(enndeakin)
I need to update it for bug 1153613.
Flags: needinfo?(enndeakin)
Depends on: 1155387
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1157193
Depends on: 1171307
Depends on: 1169268
Flags: needinfo?(amarchesini)
Depends on: 1349340
You need to log in before you can comment on or make changes to this bug.