1317322 - Pasting image from clipboard fails in some cases

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[alexander]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

OS Windows family, FF 49.

Looks like this bug partially fixed in FF50, but not fully. 

So, in FF49 - pasting images from clipboard doesn't work at all: for example, pasting image on Facebook timeline, Facebook messages, Slack messages (web version), etc. When clipboard contain any image data - Firefox didn't "see" this data. Easy to check - open context menu on edit field in Facebook - item "Paste" was disable.

In FF50 this problem partially fixed. Now it is possible to paste image in Facebook timelime, but not possible to paste on personal message.

How to reproduce:
1. Copy image to clipboard (for example from Paint), 
2. Open Facebook and try to Paste it into timeline post - it is possible to do so.
3. Now open personal messages page (letter icon->See all) and try to paste again into private message to somebody - this is not possible to do, paste action is disabled.
4. Now let's go back to Facebook main page (without reloading, with clicking on logo) and try to paste again into timeline post - Paste is disabled here again.

Only after reloading a page it is possible to paste image again.


Actual results:

Image not pasted normally in facebook private messages (on separate page, not in main page with popup). This could be an error of Facebook or in Firefox.

Also it is not possible to paste image in Slack at all (doesn't work)


Expected results:

Image could be pasted to all corresponding fields (on Facebook, on Slack, etc.)

Comment 1

[Loic]

I can reproduce the issue with Beta on Facebook.

Comment 2

[nemo]

Guess this is a stable FF50 bug now, so bumping the version.
Also, tossed the image paste implementation bug as a depends so what it's related to is clearer.
Should it be a "blocks" ?  No idea which one is appropriate since it is closed anyway ☺
But surely the reference is helpful in case there's an implementation bug and it isn't just the major sites needing Evang.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

CCing Michael for his informed opinion.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Firefox hasn't supported pasting raw image data from the clipboard into web content until 50 - when it was added in bug 906420. This explains why before firefox 50 this feature did not work at all.

With a bit more investigation, it appears as though the problem with Facebook's messenger (and potentially with slack) is caused by our editor controller deciding that the paste command is not enabled when a textarea is focused, while it is available when a contenteditable area is focused. Because of that the paste command is only successfully fired on the contenteditable area. We don't allow it in text areas. This problem could be fixed by doing something similar to what we do with the Copy and Paste commands, which is having them active 100% of the time when the currently focused document is XHTML or HTML, and there is any data which we may support through, for example, DataTransfer, on the clipboard, and only checking for paste support when in "native" XUL documents.

It may also be fixable with some gymnastics by facebook & co.

Ehsan, do you think that this is a reasonable approach?

Comment 5

[(no longer active)]

I'm not sure if I understand the suggestion...

We can't paste images in textareas.  How does enabling the paste command help us do that?  I think perhaps I don't understand what the website is trying to do and how Gecko breaks that yet...

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #5)
> I'm not sure if I understand the suggestion...
> 
> We can't paste images in textareas.  How does enabling the paste command
> help us do that?  I think perhaps I don't understand what the website is
> trying to do and how Gecko breaks that yet...

The document has a `paste` event listener attached to a textarea, when the user pastes an image into the textarea, the website is expecting the paste into the textarea to fail, but for the paste event listener to fire. It does not.

Does that kinda make sense?

Comment 7

[(no longer active)]

(In reply to Michael Layzell [:mystor] [:mrl] from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > I'm not sure if I understand the suggestion...
> > 
> > We can't paste images in textareas.  How does enabling the paste command
> > help us do that?  I think perhaps I don't understand what the website is
> > trying to do and how Gecko breaks that yet...
> 
> The document has a `paste` event listener attached to a textarea, when the
> user pastes an image into the textarea, the website is expecting the paste
> into the textarea to fail, but for the paste event listener to fire. It does
> not.
> 
> Does that kinda make sense?

I see, yes this makes perfect sense.

It seems like in bug 1208217, we fixed this for nsClipboardCommand, but we forgot about the equivalent commands in EditorCommands.cpp (CopyCommand, CutCommand and PasteCommand.)  There are other variants of these commands (such as CutOrDeleteCommand, for example) which I think we can ignore since they're not bound to Ctrl+C/X/V.

Can you please take this, Michael?  Thanks!

Comment 8

[Nika Layzell [:nika] (ni? for response)]

I did some more tests and my understanding was completely incorrect. We are in fact firing the events when you use C-V.

I also tried performing the steps in chrome and it seemed like it didn't work there either, so I don't think that this is our fault.

Comment 9

[alexander]

In Chrome pasting works normally for me (Windows platform), everywhere on FB and in Slack.

Comment 10

[nemo]

Just a bit of an update. :mystor says the Chrome failure he reported above was Linux-only.  Under Windows it worked fine, which agrees with the bug reporter.

That does suggest possibly fail UA sniffing on Facebook's part...
Would be interesting if reporter could try spoofing a Chrome for Windows user agent.  If it works fine then this is an evang bug.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to nemo from comment #10)
> Just a bit of an update. :mystor says the Chrome failure he reported above
> was Linux-only.  Under Windows it worked fine, which agrees with the bug
> reporter.
> 
> That does suggest possibly fail UA sniffing on Facebook's part...
> Would be interesting if reporter could try spoofing a Chrome for Windows
> user agent.  If it works fine then this is an evang bug.

I have tested Firefox with a Chrome UA, and the paste works in that situation. I think this is a facebook problem.

Comment 12

[(no longer active)]

(In reply to Michael Layzell [:mystor] [:mrl] from comment #11)
> (In reply to nemo from comment #10)
> > Just a bit of an update. :mystor says the Chrome failure he reported above
> > was Linux-only.  Under Windows it worked fine, which agrees with the bug
> > reporter.
> > 
> > That does suggest possibly fail UA sniffing on Facebook's part...
> > Would be interesting if reporter could try spoofing a Chrome for Windows
> > user agent.  If it works fine then this is an evang bug.
> 
> I have tested Firefox with a Chrome UA, and the paste works in that
> situation. I think this is a facebook problem.

Can you please contact them to confirm?  Thanks!

Comment 13

[nemo]

Based on the comments going to try to reclassify this. Expanded platform to "all" since it seems *more* broken on Linux apparently (in that Facebook breaks all browsers, not just Firefox)

Comment 14

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

(In reply to :Ehsan Akhgari from comment #12)
> (In reply to Michael Layzell [:mystor] [:mrl] from comment #11)
> > (In reply to nemo from comment #10)
> > > Just a bit of an update. :mystor says the Chrome failure he reported above
> > > was Linux-only.  Under Windows it worked fine, which agrees with the bug
> > > reporter.
> > > 
> > > That does suggest possibly fail UA sniffing on Facebook's part...
> > > Would be interesting if reporter could try spoofing a Chrome for Windows
> > > user agent.  If it works fine then this is an evang bug.
> > 
> > I have tested Firefox with a Chrome UA, and the paste works in that
> > situation. I think this is a facebook problem.
> 
> Can you please contact them to confirm?  Thanks!

Mike - I may have missed this (apologies if I have), but have we perhaps reached out via the DL?

Comment 15

[(no longer active)]

I haven't yet seen this discussed on our internal mailing list...

Comment 16

[Mike Taylor [:miketaylr]]

It looks like we have not, ideally we can give FB more info than "this works when spoofing as Chrome" before reaching out.

Comment 17

[Mike Taylor [:miketaylr]]

Tom, can you take a look at why spoofing as Chrome gets paste to work here? Ideally we can at least point FB to the right file for them to investigate further.

Comment 18

[Thomas Wisniewski]

This is what I'm seeing, in a Windows 10 VM with Firefox 50.0.2 (fresh profile) and in the latest Chrome. Also in Gentoo Linux, with Firefox nightly and the latest Chrome stable build.

In Slack, I can ctrl-V to bring up the "Upload a file?" dialog in both browsers, with the image in my clipboard appearing there. In Chrome I can also right-click the message bar to get a "Paste" option which does the same thing. In Firefox the paste option is greyed-out.

But in Facebook, I can visit a friend's timeline page, click the "write something to X..." box (once the page fully loads), and then ctrl-V or paste the image from the context menu. I can also ctrl-V and menu-paste if I click their "Message" button to send them a PM in the little Messenger pop-up which appears above the timeline message box. I have no clue what the "letter icon" is, though, so I haven't confirmed that option yet.

Changing my UA to Chrome does not seem to change Firefox's behavior on either site, in either OS.

Looking over their markup, Slack is using a plain textarea. Facebook starts with a textarea for the "write something to X..." box, but changes it as the page loads into a contenteditable div. It's also using a contenteditable div for the PM pop-up. (Perhaps that changing from textarea to contentedtiable div explains why sometimes it seems to offer "Paste" in the menu, and other times it doesn't...?)

But unless I'm missing something, there isn't a problem with either site. Firefox is just not offering the "Paste" menu option when for images in textareas (even if they have a paste listener). Yet it does so for contenteditable divs.

In other words, my testing seems to match comment 4's assessment, and that if a paste handler/etc are detected, we could offer the paste option (even if the textarea itself won't end up with anything pasted in it).

Comment 19

[(no longer active)]

Hmm, so that means we should do comment 7, except that Michael had already looked into that...  Michael, can you think of where the disconnect here is?  Thank you!

Comment 20

[Thomas Wisniewski]

If it helps, based on comment 7, I hacked a local build's EditorCommands.cpp PasteCommand::IsCommandEnabled() to always set aIsEnabled=true, and with that the textarea on Slack does what it does in Chrome.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #19)
> Hmm, so that means we should do comment 7, except that Michael had already
> looked into that...  Michael, can you think of where the disconnect here is?
> Thank you!

So the problem which I was running into was whether or not Ctrl-V would work when in a textarea with a paste listener. When I was in firefox I found that facebook wouldn't handle that situation correctly, while when I spoofed chrome it did. I am not sure whether or not this is still the case.

I believe that you are correct about the menu option being not available in firefox within that text area, and that should be a simple fix. I'll try to have a patch up shortly.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Attachment: Activate `paste` in menus when within <textarea> controls

Oops, I apparently forgot to post this patch. :S

Comment 23

[nemo]

So... does this mean there's both a tech evang component (the website behaving differently on ctrl-v based on UA) and a firefox bug (this missing context menu) ?

If so, should this be two bugs?

Comment 24

[Nika Layzell [:nika] (ni? for response)]

(In reply to nemo from comment #23)
> So... does this mean there's both a tech evang component (the website
> behaving differently on ctrl-v based on UA) and a firefox bug (this missing
> context menu) ?
> 
> If so, should this be two bugs?

:twisniewski's comment seems to suggest that the tech evang component is no longer an issue so I figure we don't need to file a separate bug for it and can just use this bug for the context menu changes.

Comment 26

[(no longer active)]

Comment on attachment 8825111 [details] [diff] [review]
Activate `paste` in menus when within <textarea> controls

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

Looks good, but you should also add tests.  r=me with the expectation that the patch will land with tests.

Comment 27

[Nika Layzell [:nika] (ni? for response)]

ni?ing myself to make sure I write the tests.

Comment 28

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add tests for new paste command behavior

I finally got around to writing some simple tests for this change.

MozReview-Commit-ID: FL1tRmyzJvr

Comment 29

[(no longer active)]

Comment on attachment 8827299 [details] [diff] [review]
Part 2: Add tests for new paste command behavior

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

Please fold these patches together when landing, there's no need for them to be separate.  r=me with that.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this (and the followup to disable the test on android) out for frequent clipboard test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=69679287&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/486075111a45

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d6e9229fda
Activate `paste` in menus when within <textarea> controls, r=ehsan

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Attachment: Activate `paste` in menus when within <textarea> controls

Updated patch which should be safe to land without the windows only test failures.

MozReview-Commit-ID: 7YD8qtsC3u6

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f63b9d012c0
Activate `paste` in menus when within <textarea> controls, r=ehsan

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51116aebee59
Fix a windows devtools test failure on inbound, a=bustage

Comment 36

[Phil Ringnalda (:philor)]

Backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=70192957&repo=mozilla-inbound for the Windows mochitest-4 leak we apparently didn't tell you about the last time, https://treeherder.mozilla.org/logviewer.html#?job_id=70192957&repo=mozilla-inbound

Comment 37

[(no longer active)]

Any luck landing this yet?

Comment 38

[(no longer active)]

ping?

Comment 39

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #38)
> ping?

I'm working on fixing the leak in my spare time. The cycle time has been long because I have to push to try every time I want to test a patch.

Comment 40

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #39)
> (In reply to :Ehsan Akhgari from comment #38)
> > ping?
> 
> I'm working on fixing the leak in my spare time. The cycle time has been
> long because I have to push to try every time I want to test a patch.

Thanks!  Just pinging because I keep seeing this issue coming up in various profiles.  :-)

FWIW https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Refcount_tracing_and_balancing is the systematic way of debugging leaks like this, in case you haven't come across it already.  If this only happens on try then you'd need to do look at the leak reports there.  testing/mochitest/runtests.py claims that we save a runtests_leaks.log file with that.  That's also where you want to define the environment variables in the MDN page.  Let me know if I can help!

Comment 41

[Nika Layzell [:nika] (ni? for response)]

Attachment: Change the SetOpenerWindow tabgroup assertion to a DIAGNOSTIC_ASSERT

MozReview-Commit-ID: 13zUBotz7Tr

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8840549 [details] [diff] [review]
Change the SetOpenerWindow tabgroup assertion to a DIAGNOSTIC_ASSERT

oops, wrong bug

Comment 43

[Nika Layzell [:nika] (ni? for response)]

The leak seems to be being caused by this AddRef: http://searchfox.org/mozilla-central/rev/c9ccb8ec1f5acda78e6344ffb87aa3a409031e3d/widget/windows/nsDataObj.cpp#464 which spawns a timer which is not run before the browser shuts down. Basically, as the last reference is destroyed due to OleFlushKeyboard from the OS in nsClipboard::Observe, we queue up a timer to actually destroy it because we have a temp file which we want to destroy and that timer never fires. 

In the test run which I did to discover this, the timer was set at 12:40:28 in my test run in question, and the leakcheck occurs at 12:40:34, which is much less than the 500ms which the timer takes.

I'm looking into fixing this bug by creating an alternate nsIObserver class `TempFileDestroyer` which holds the last reference to the tempfile, and listens for either the xpcom shutdown event or the timer firing to delete the file from the filesystem. That way the object shouldn't be kept alive for longer than is necessary.

Comment 44

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown

MozReview-Commit-ID: 90obWnqRSWk

Comment 45

[Jim Mathies [:jimm]]

Comment on attachment 8842111 [details] [diff] [review]
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown

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

::: widget/windows/nsDataObj.cpp
@@ +441,5 @@
>  	return m_cRef;
>  }
>  
> +namespace {
> +class TempFileDestroyer : public nsIObserver

nit, not a huge fan of 'TempFileDestroyer'. Can we give it a bit more professional name, something like 'RemoveTempFileHelper' or similar.

Comment 46

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown

MozReview-Commit-ID: 90obWnqRSWk

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cba44c517d
Part 1: Activate `paste` in menus when within <textarea> controls, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb8e1322979
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9b08930b65
Part 3: Make RemoveTempFileHelper's constructor explicit, a=bustage

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out (and your other bug) for being a likely cause of bug https://treeherder.mozilla.org/logviewer.html#?job_id=80965408&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d64711bdb426

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2f3894113f
Part 1: Activate `paste` in menus when within <textarea> controls, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a64d16d18e5
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm

Comment 51

[Nika Layzell [:nika] (ni? for response)]

relanded it because it looks like it wasn't my fault (https://bugzilla.mozilla.org/show_bug.cgi?id=1337772)

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac2f3894113f
https://hg.mozilla.org/mozilla-central/rev/5a64d16d18e5