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

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: bull500, Assigned: Neil Deakin)

Tracking

35 Branch
mozilla40
x86_64
Linux
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify ?

Firefox Tracking Flags

(e10sm5+, firefox40 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
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
This image copy bug might be related to or a dupe of drag/drop bug 936092.
Blocks: 879538
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.
Status: UNCONFIRMED → NEW
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
Duplicate of this bug: 1069453
tracking-e10s: ? → m5+
(Assignee)

Updated

3 years ago
Blocks: 1058712

Updated

3 years ago
Flags: firefox-backlog+
Duplicate of this bug: 1098388
Assignee: nobody → ally

Comment 5

3 years ago
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.
Created attachment 8534757 [details] [diff] [review]
expandClipboardOptions wip
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.
Created attachment 8539570 [details] [diff] [review]
hacking around

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
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?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 20

3 years ago
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)

Updated

3 years ago
Assignee: ally → nobody

Updated

3 years ago
Blocks: 1122121
Assignee: nobody → wmccloskey

Updated

3 years ago
Component: File Handling → DOM: Core & HTML
Product: Firefox → Core
Duplicate of this bug: 1134012
(Assignee)

Comment 22

3 years ago
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)

Updated

3 years ago
Assignee: wmccloskey → enndeakin
(Assignee)

Updated

3 years ago
Points: --- → 8
(Assignee)

Comment 24

3 years ago
Created attachment 8568642 [details] [diff] [review]
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.
Attachment #8539570 - Attachment is obsolete: true

Updated

3 years ago
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Flags: qe-verify?
(Assignee)

Comment 25

3 years ago
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)
(Assignee)

Comment 27

3 years ago
Created attachment 8570459 [details] [diff] [review]
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.
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
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 :)
Attachment #8570459 - Flags: review?(bugs) → review+
(Assignee)

Comment 30

3 years ago
(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 :)
(Assignee)

Updated

3 years ago
Blocks: 936092
(Assignee)

Comment 31

3 years ago
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.
(Assignee)

Comment 33

3 years ago
Created attachment 8574666 [details] [diff] [review]
Updated patch
Attachment #8570459 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8574667 [details] [diff] [review]
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.
Attachment #8574667 - Flags: review?(bugs)
(Assignee)

Comment 35

3 years ago
Created attachment 8574669 [details] [diff] [review]
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.
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+

Updated

3 years ago
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
(Assignee)

Updated

3 years ago
No longer blocks: 936092
Depends on: 936092
Duplicate of this bug: 1142813

Updated

3 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar

Updated

3 years ago
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
(Assignee)

Comment 39

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a040986845d
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
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

Updated

3 years ago
Duplicate of this bug: 1153719
(Assignee)

Comment 43

3 years ago
Created attachment 8591818 [details] [diff] [review]
Part 1 - support non-text types for clipboard

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
(Assignee)

Comment 44

3 years ago
Created attachment 8591819 [details] [diff] [review]
Part 2 - fix widget build issues
(Assignee)

Comment 45

3 years ago
Created attachment 8591822 [details] [diff] [review]
Part 3 - fix assertions in drag code that doesn't handle nsCStrings.
(Assignee)

Comment 46

3 years ago
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)
(Assignee)

Comment 47

3 years ago
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.
Attachment #8591819 - Flags: review?(fabrice)
Attachment #8591819 - Flags: review?(fabrice) → review+
Attachment #8591822 - Flags: review?(bugs) → review+

Updated

3 years ago
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)
(Assignee)

Comment 49

3 years ago
I need to update it for bug 1153613.
Flags: needinfo?(enndeakin)

Comment 50

3 years ago
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
Depends on: 1155387
https://hg.mozilla.org/mozilla-central/rev/802512ee57ae
https://hg.mozilla.org/mozilla-central/rev/e812687a81cd
https://hg.mozilla.org/mozilla-central/rev/ace1964512f5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1157193

Updated

3 years ago
Depends on: 1171307

Updated

3 years ago
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.