1258857 - Enable test_copypaste.html with e10s

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This test has 12 failures when I run it locally with e10s on OSX. All of the failures are due to the checks of the return value of clipboard.hasDataMatchingFlavors() inside various calls to copySelectionToClipboard() in the test. test_bug166235.html also calls hasDataMatchingFlavors() and works just fine in e10s, so that method can't be totally broken.

Comment 1

[Gabriele Svelto [:gsvelto]]

I'm having a look at this on Linux (where I can reproduce). Deep down at the lowest level it seems that for those tests the list of flavors associated with the clipboard is different depending on when the code is run in e10s and non-e10s mode. More precisely in e10s it's missing the right entry checked in the test. I'm not familiar with this code but I'm prodding around because I assume we're doing something different in e10s mode, possibly not remoting something correctly when setting the type of the element in the clipboard.

Comment 2

[Neil Deakin]

What types and data does it contain?

I think it is more likely that the test just isn't waiting for the clipboard operation to be completed first before verifying the result.

Comment 3

[Gabriele Svelto [:gsvelto]]

I'm looking at the last two failing tests (lines 68 and 69 in the output). In the first one the test fails because gtk_selection_data_targets_include_text() returns false when called on the selection data (it returns true in the non-e10s code). In the second one the list of types returned by gtk_selection_data_get_targets() lacks the text/html type. In non-e10s mode the types returned are:

TIMESTAMP
TARGETS
MULTIPLE
SAVE_TARGETS
text/_moz_htmlcontext
text/_moz_htmlinfo
text/x-moz-url-priv
text/html

In e10s mode the last is not present and the test fails.

Comment 4

[Gabriele Svelto [:gsvelto]]

Digging a little further I've noticed that when nsClipboard::SetData() gets called for the failing tests it's neither setting text/html nor text/unicode when calling gtk_clipboard_set_with_data() so the contents of the clipboard are not being set correctly in the first place. Now to figure out why...

Comment 5

[Gabriele Svelto [:gsvelto]]

I'm slowly getting to the root of the problem: the nsITransferable item we pass to nsClipboard::SetData() has an htmlformatconverter attached to it in non-e10s mode, but doesn't have it anymore in e10s mode. Maybe it's not being passed properly via IPC? I'll look into that as the next step.

Comment 6

[Gabriele Svelto [:gsvelto]]

OK, I think I've nailed the problem. In the child process when we call nsContentUtils::TransferableToIPCTransferable() to turn our transferable object holding the clipboard contents we don't seem to handle the fact that the original Transferable has a converter attached to it that adds the text/html and text/unicode types to its set of flavors. The resulting object after being transferred via IPC contains only the types that associated with specific bits of data since the converter isn't there anymore, namely text/_moz_htmlcontext, text/_moz_htmlinfo and text/x-moz-url-priv. I'm not sure what's the best approach to fix this though.

Comment 7

[Neil Deakin]

The converter should be handled by GetTransferData transparently to the caller though. All the passing tests also use the converter and work ok.

It looks like the only tests that fail are ones where the resulting value is an empty string. Likely, some code isn't handling that case properly, possibly interpreting it as non-existing rather than present but with length 0.

Comment 8

[Neil Deakin]

OK, so the issue here I think is that nsContentUtils::TransferableToIPCTransferable gets the list of flavors which includes "text/html" and "text/unicode" but GetTransferData returns null for these since there isn't any data for them. These types never get added to the ipc data passed to the parent process.

In single-process mode, on Mac, for example, it looks like nsClipboard::PasteboardDictFromTransferable does something similar, sees the null value and, instead of ignoring it, uses a default empty string instead and puts that on the clipboard.

One option might be to just have TransferableToIPCTransferable use an empty string for text/html and text/unicode when GetTransferData returns null.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Neil Deakin from comment #8)
> OK, so the issue here I think is that
> nsContentUtils::TransferableToIPCTransferable gets the list of flavors which
> includes "text/html" and "text/unicode" but GetTransferData returns null for
> these since there isn't any data for them. These types never get added to
> the ipc data passed to the parent process.
> 
> In single-process mode, on Mac, for example, it looks like
> nsClipboard::PasteboardDictFromTransferable does something similar, sees the
> null value and, instead of ignoring it, uses a default empty string instead
> and puts that on the clipboard.
> 
> One option might be to just have TransferableToIPCTransferable use an empty
> string for text/html and text/unicode when GetTransferData returns null.

I think the issue is more subtle here:

TransferableToIPCTransferable is taking the list of flavors from here:

http://hg.mozilla.org/mozilla-central/file/tip/dom/base/nsContentUtils.cpp#l7333

This include three flavors for the data elements present in the original transferable (text/_moz_htmlcontext, text/_moz_htmlinfo and text/x-moz-url-priv) and two that are provided by the nsHTMLFormatConverter do not belong to data entries but are rather "synthesized" in the code below this point:

http://hg.mozilla.org/mozilla-central/file/tip/widget/nsTransferable.cpp#l585

What's the problem with that? That while the nsHTMLFormatConverter claims to be able to output the text/html and text/unicode it's able to output those only if the input is already in text/html format, see here:

http://hg.mozilla.org/mozilla-central/file/tip/widget/nsHTMLFormatConverter.cpp#l132

So what happens is that when converting the Transferable in TransferableToIPCTransferable the code tries to lookup a text/html (or text/unicode) element, doesn't find it then tries to convert the existing data items in text/html (or text/unicode) but fails because the converter doesn't support the conversion, see here:

http://hg.mozilla.org/mozilla-central/file/tip/widget/nsTransferable.cpp#l332

So it looks like this code happened to work "by chance" because the test only queried if the text/html flavor was supported but never actually tried to get data out of the clipboard in that format, because if it did it would have failed in non-e10s mode too.

In the end I think that this code hasn't actually been exercised in a while: the nsHTMLFormatConverter object is used here and nowhere else in the codebase and it doesn't actually seem to do anything useful so I suspect that this might be legacy that was leftover at some point. I want to try to get rid of it and see if something else fails or not to verify my gut feeling.

BTW, the other sub-tests that are failing and are unrelated to this seem to be caused by synthesizing keys (CTRL+V to be precise) so they should be fixable by fixing the test itself.

Comment 10

[Gabriele Svelto [:gsvelto]]

Scratch (part of) what I wrote before, it seems that the converter is being put there precisely to make flavor queries work but not to really convert data, see the comment here:

http://hg.mozilla.org/mozilla-central/file/tip/dom/base/nsCopySupport.cpp#l200

So I guess the fix is to add the supported flavors with empty data in the IPC transferable. I don't believe we can easily transfer the converter over via IPC.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

This is a tentative patch that fix half of the tests (those checking for the MIME type compatibility). The remaining 6 need to be fixed in the test itself.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

Updated patch, this one seems to fix all the tests, here's the try run:

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

Comment 13

[Gabriele Svelto [:gsvelto]]

Wrong try run, the correct one is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf098c7b3cbc

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8736750 [details] [diff] [review]
[PATCH v2] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

Review of attachment 8736750 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ +7355,5 @@
> +        if (!data) {
> +          // Empty element, transfer only the flavor
> +          IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement();
> +          item->flavor() = nsCString(flavorStr);
> +          item->data() = NS_LITERAL_CSTRING("");

Drive-by nit: NS_LITERAL_CSTRING("") should be EmptyCString().

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #14)
> Drive-by nit: NS_LITERAL_CSTRING("") should be EmptyCString().

Thanks for the tip!

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

Try is green, here's the refreshed patch. What it does is for every flavor that has no data associated with it add an item in the IPC transferable object with the said flavor and an empty string for the data. I hope this is the right approach.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd93c3870431

Comment 17

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

Comment on attachment 8737173 [details] [diff] [review]
[PATCH v3] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

Neil, could you review this? I've been a bit overloaded with reviews.
If not, put this back to my review queue.

Comment 18

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8737173 [details] [diff] [review]
[PATCH v3] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

This is causing other tests to fail which I hadn't noticed, clearing the review flag for now.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

Revised patch with the breakage addressed, the description from comment 16 still applies.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57cffdda91f8

Comment 20

[Neil Deakin]

Comment on attachment 8737741 [details] [diff] [review]
[PATCH v4] Add empty items to an IPC transferable object for every flavor of the source object that did not have any data associated to it

>               item->flavor() = nsCString(flavorStr);
>               item->data() = NS_ConvertUTF8toUTF16(flavorStr);
>             }
>+
>+            // Empty element, transfer only the flavor
>+            if (!data) {

Use: else if

Note that since most clipboard types don't use single-byte strings (nsAString is used), nothing will end up getting added, but this is ok if this fixes this issue and gets a test enabled.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b60f2ed39c

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37b60f2ed39c

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Hey, did this miss actually removing the skip-if for this test in the .ini?

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to :Felipe Gomes (needinfo me!) from comment #23)
> Hey, did this miss actually removing the skip-if for this test in the .ini?

Indeed I missed it :-/ I'll open a follow up to address that issue.