1305163 - FF50b1: Double Images Created when using CTRL-V at Imgur.com

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P2)

Description

[MarkRH]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160920155715

Steps to reproduce:

Go to imgur.com (while logged in) and press CTRL-V to upload an image via the clipboard. 


Actual results:

When pressing CTRL-V, two new images were created out of what was in the clipboard: http://i.imgur.com/6gZGQUQ.png (the two on the top-right). What's strange is that one is a .PNG http://i.imgur.com/7YkBpLw.png and one is a .JPG http://i.imgur.com/ulUdiXj.jpg I then have to delete the extra one.


Expected results:

Only one new image should have be been created I would think, as happens in previous versions of Firefox. They only make the single first .PNG image.  So, something is different with FF50 and the clipboard and what it does with CTRL-V, at least on Imgur.

Comment 1

[Loic]

I'm not able to reproduce it with FF50 or 52. Only one instance of the image is uploaded.

Are you able to reproduce the issue with a clean profile?
https://support.mozilla.org/fr/kb/utiliser-gestionnaire-profils-creer-supprimer-profils

Comment 2

[MarkRH]

Does the same thing with a clean profile. When I press CTRL-V is actually says "2 images uploading".  Tried both the New Post page and the Images page. http://i.imgur.com/Un6dsq5.png

Comment 3

[MarkRH]

I did another test where I set all the plugins to never activate and disabled the Roboform Toolbar (only extension listed). When I press CTRL-V it uploads 2 images: Snapshot of this showing 2 images uploading: http://i.imgur.com/X7ya3Li.png

Comment 4

[Loic]

I dunno which step I miss to be not able to reproduce the bug.

Did you enable e10s in FF50?

Comment 5

[MarkRH]

No, it was disabled in both normal and clean profiles: Multiprocess Windows 	0/1 (Disabled)

I then made a new "betaclean" profile so it didn't share with my normal Firefox. This time Multiprocess was enabled but it still pasted 2 images with CTRL-V.

Comment 6

[MarkRH]

Oh before, was running the same "clean" profile in both FF49 and 50 and it pasted 1 image with 49 and 2 images with 50.

Comment 7

[Onek Jude]

Hello,

MarkRH is right, i sign up with http://imgur.com and i  reproduced the same  bug again. It does uploads 2 images with ctrl + v. i tested it with firefox 49. would someone point me to the right direction of how to fix this bug?

thanks

Comment 8

[MarkRH]

(In reply to Onek Jude from comment #7)
> Hello,
> 
> MarkRH is right, i sign up with http://imgur.com and i  reproduced the same 
> bug again. It does uploads 2 images with ctrl + v. i tested it with firefox
> 49. would someone point me to the right direction of how to fix this bug?
> 
> thanks

Note: I do not have the problem with Firefox 49 like Onek does. I see the problem in Firefox 50 beta 1, with and without clean profiles.

Comment 9

[Loic]

Are you connetced to Imgur through a social account like Facebook?

Comment 10

[MarkRH]

I don't have Imgur directly connected to any social media and am not using one of those to log into it with.

Comment 11

[MarkRH]

Same happens with 50beta2.

Comment 12

[Kanchan Kumari QA]

I am able to reproduce this issue with FF 50 and 51 on Windows 7 and Windows 10, two images are getting uploaded.
On FF 52 ctr + v is not working at all/ no image is getting uploaded.
On Ff 49 ctr + v works fine and only one image gets uploaded.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Kanchan Kumari QA from comment #12)
> I am able to reproduce this issue with FF 50 and 51 on Windows 7 and Windows
> 10, two images are getting uploaded.
> On FF 52 ctr + v is not working at all/ no image is getting uploaded.
> On Ff 49 ctr + v works fine and only one image gets uploaded.

Hi Kanchan, would you be able to get the regression window? Thanks!

Comment 14

[Kanchan Kumari QA]

Regression range: 
Last good revision: b2409b36cd07bba045bf38dd131166c36ea3da4a
First bad revision: 3d0aef4586bdbd81591811ec627c85161fc11065

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2409b36cd07bba045bf38dd131166c36ea3da4a&tochange=3d0aef4586bdbd81591811ec627c85161fc11065

Comment 15

[Alfonso Martinez]

Same thing happens in https://web.telegram.org/ or other places that read the images from the clipboard like http://www.martinezdelizarrondo.com/ckplugins/simpleuploads.demo4/

They don't expect two versions of the same image on the clipboard.

Comment 16

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

I think that this should be fixed in 51 because of bug 1290688

Perhaps we want to uplift?

Comment 17

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

(In reply to Michael Layzell [:mystor] from comment #16)
> I think that this should be fixed in 51 because of bug 1290688
> 
> Perhaps we want to uplift?

(Namely, we would only have to uplift part 1 which is very low risk)

Comment 18

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

Either uplift the fix or backout the regressing patch.

Comment 19

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

(In reply to Olli Pettay [:smaug] from comment #18)
> Either uplift the fix or backout the regressing patch.

The regressing patch is pretty big. It will be much easier to uplift the 2-line fix.

Comment 20

[Alice0775 White]

(In reply to Michael Layzell [:mystor] from comment #16)
> I think that this should be fixed in 51 because of bug 1290688
> 

See comment#12, Aurora 51 also affected. Nightly 52 as well(worse).

Comment 21

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

This is odd. It seems to work for me on linux. I went to google search results, copied an image, and pasted it into the imgur upload gui. It only uploads a single copy of the image on nightly.

Comment 22

[Alfonso Martinez]

Using "Copy" from an image, only one image is pasted. 
But taking a screenshot, both a png and jpg are pasted

Comment 23

[Alice0775 White]

I filed bug for the problem comment #15.
bug 1307999, bug 1308007

Comment 24

[MarkRH]

FYI, If I copy part of an image using Irfanview, I get two images uploaded. If I right-click an image on a Google image search and select copy image, I get two images when doing CTRL-V. Using 50 beta 4.

Comment 25

[Jet Villegas (inactive)]

(In reply to Michael Layzell [:mystor] from comment #21)
> This is odd. It seems to work for me on linux. I went to google search
> results, copied an image, and pasted it into the imgur upload gui. It only
> uploads a single copy of the image on nightly.

This bug was reported on Windows, and likely platform-specific depending on clipboard formats we support, now that we push formats up in a loop:

https://bugzilla.mozilla.org/attachment.cgi?id=8766012&action=diff#a/dom/events/DataTransfer.cpp_sec8

Comment 26

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

I'm making this bug be about the duplicate image issue. I believe that the no image issue is bug 1308007.

I believe that this issue should be fixable through some uplifts of bug 1290688 combined with bug 1309645.

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Stone will be helping uplifting this to aurora and beta.
Please let us know if there are more details we need to care. Thanks!

Comment 28

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

Attachment: BETA: Don't expose multiple image types through DataTransfer

This should make it such that we only expose a single type of image through the DataTransfer. This patch should be able to apply on BETA.

Comment 29

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

Attachment: AURORA: Don't expose multiple image types through DataTransfer

And the AURORA patch.

Comment 30

[Neil Deakin]

Do these patches and the original one inadvertently remove rtf?

Comment 31

[Ming-Chou Shih [:stone]]

Attachment: BETA: Don't expose multiple image types through DataTransfer (rebased)

Rebased patch. Tested on windows10 and passed.

Comment 32

[Ming-Chou Shih [:stone]]

Attachment: AURORA: Don't expose multiple image types through DataTransfer (rebased)

Rebased patch. Tested on windows10 and passed.

Comment 33

[Neil Deakin]

Comment on attachment 8802800 [details] [diff] [review]
BETA: Don't expose multiple image types through DataTransfer (rebased)

Let's file a bug to fix the missing rtf separately (and probably combine the two lists).

Comment 34

[Ming-Chou Shih [:stone]]

Tried results on Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ae4d52a2f892d60eaec52269051ac7f6d1dc18

Tried results on Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74ee16883129b61257d8085d9bcddd9c18c46142

There are some failures but looks like unrelated to this bug.

Comment 35

[Ming-Chou Shih [:stone]]

Hi Michael,
Could you help to create the uplift requests if the tried results are ok for you? Because I have no idea about how to fill the required information. Thanks.

Comment 36

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

Comment on attachment 8802800 [details] [diff] [review]
BETA: Don't expose multiple image types through DataTransfer (rebased)

Approval Request Comment
[Feature/regressing bug #]: bug 906420
[User impact if declined]: Attempts to copy/paste images will cause multiple images to be present in the paste.
[Describe test coverage new/current, TreeHerder]: DataTransfer generally has poor test coverage.
[Risks and why]: This patch generally doesn't change high risk parts of the codebase, instead simply causing certain image mime types to not be considered for inclusion into the DataTransfer. I think this is fairly low risk.
[String/UUID change made/needed]: None

Comment 37

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

Comment on attachment 8802801 [details] [diff] [review]
AURORA: Don't expose multiple image types through DataTransfer (rebased)

Approval Request Comment
[Feature/regressing bug #]: bug 906420
[User impact if declined]: Attempts to copy/paste images will cause multiple images to be present in the paste.
[Describe test coverage new/current, TreeHerder]: DataTransfer generally has poor test coverage.
[Risks and why]: This patch generally doesn't change high risk parts of the codebase, instead simply causing certain image mime types to not be considered for inclusion into the DataTransfer. I think this is fairly low risk.
[String/UUID change made/needed]: None

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8802800 [details] [diff] [review]
BETA: Don't expose multiple image types through DataTransfer (rebased)

Recent visible regression in a core scenario, Beta50+.

Hi Andrei, could you please do some exploratory testing around the STRs? This fix may need verification on our side to be sure we haven't caused any unexpected regressions.

Comment 39

[Carsten Book [:Tomcat]]

still has problems uplifting to aurora:

renamed 1305163 -> bug-1305163-aurora-v3.patch
applying bug-1305163-aurora-v3.patch
patching file dom/events/DataTransfer.cpp
Hunk #4 FAILED at 1413
1 out of 4 hunks FAILED -- saving rejects to file dom/events/DataTransfer.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1305163-aurora-v3.patch

Comment 40

[Ming-Chou Shih [:stone]]

Attachment: AURORA: Don't expose multiple image types through DataTransfer (rebased, v4)

Comment 41

[Ming-Chou Shih [:stone]]

Attachment: imgur-bad-rendering.png

When I re-tested the patch, I found the rendering results seems NG. I don't know if it's a known issue so still update here. The screen shot "imgur-bad-rendering.png" is uploaded.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/127130680c1d

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/73b6275d8274

Comment 44

[Andrei Vaida [:avaida]]

(In reply to Ritu Kothari (:ritu) from comment #38)
> Comment on attachment 8802800 [details] [diff] [review]
> BETA: Don't expose multiple image types through DataTransfer (rebased)
> 
> Recent visible regression in a core scenario, Beta50+.
> 
> Hi Andrei, could you please do some exploratory testing around the STRs?
> This fix may need verification on our side to be sure we haven't caused any
> unexpected regressions.

We'll make sure this is included in our 50.0b11 sign off. Keeping the ni? until we're done.

Comment 45

[Bogdan Maris, Desktop QA]

I was able to reproduce the issue described in comment 0 *but* only if I pasted the image stored in clipboard after I took a screenshot or copied from websites directly. I was not able to copy/paste images from local drive (using 49 I could).

I verified that using Firefox 50 beta 11 on Windows 10 64bit, only one image will be pasted in imgur but again, from screenshot or copied from websites directly, images from local drive don't work. Not sure if this is something intended (it should not be IMHO). 
Ming-Chou Shih maybe you can clear this up?

Comment 46

[Ming-Chou Shih [:stone]]

Looks like it's not related to this patch. I rollback the patch and still unable to paste images from local drive.

Comment 47

[Bogdan Maris, Desktop QA]

(In reply to Ming-Chou Shih [:stone] from comment #46)
> Looks like it's not related to this patch. I rollback the patch and still
> unable to paste images from local drive.

Well, then I'm gonna mark as verified on 50 and 51 (I also tested on latest Developer Edition) since the double items issue is fixed (bug description) and for the other issue with local files I think bug 1308007 reflects this behavior, the only difference is that it does not reproduce only on e10s.

Also on latest Developer Edition I was able to see the graphic issue mentioned in comment 41.

Comment 48

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

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #45)
> I was able to reproduce the issue described in comment 0 *but* only if I
> pasted the image stored in clipboard after I took a screenshot or copied
> from websites directly. I was not able to copy/paste images from local drive
> (using 49 I could).
> 
> I verified that using Firefox 50 beta 11 on Windows 10 64bit, only one image
> will be pasted in imgur but again, from screenshot or copied from websites
> directly, images from local drive don't work. Not sure if this is something
> intended (it should not be IMHO). 

This is the expected behavior because of bug 1308007. We're hoping to clear it up at some point, but for now copy and paste of files doesn't work with e10s enabled. You were probablty able to make it work on 49 because you don't have e10s enabled.

Comment 49

[MarkRH]

Just tried 50beta11 and I only get one image when CTRL-V at imgur.com now. This was from doing a ALT-PrintScreen and copying part of an image from Irfanview.  So, this seems to have solved the issue in my case. Thanks for everyone's work.

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

According to the comment49 and tracking flags, time to close this as Resolved/Fixed.