1340039 - "Copy Image" places invalid PNG into the %tmp% directory

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[toe_head2001]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Navigate to:
https://raw.githubusercontent.com/TeamNewPipe/NewPipe/master/assets/new_pipe_icon_5.png

Right Click -> Copy Image


Actual results:

Firefox places serveral formats onto the clipboard, including a reference to a copy of the PNG in the %tmp% directory. However, this PNG file is 0 bytes, and thus I am not able to paste the image.


Expected results:

The PNG file in %tmp% should be the actual PNG image.

Comment 1

[Loic]

Hector, as you worked on the recent bug 664717, what are your thoughts about this issue?

Comment 2

[Hector Zhao [:hectorz]]

(In reply to Loic from comment #1)
> Hector, as you worked on the recent bug 664717, what are your thoughts about
> this issue?

Seems to be CSP related, trying to prepare a patch now, will ping others if that doesn't work.

Comment 3

[Hector Zhao [:hectorz]]

My initial attemp doesn't work, :ckerschb, can you help with finding a more qualified assignee for this bug? Thanks.

Browser console:

> Content Security Policy: The page’s settings blocked the loading of a resource at https://raw.githubusercontent.com/TeamNewPipe/NewPipe/master/assets/new_pipe_icon_5.png (“default-src 'none'”).  (unknown)

Local debug build:

> 
> [Parent 3300] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file d:/mozilla/gecko-dev/dom/security/nsContentSecurityManager.cpp, line 479
> [Parent 3300] WARNING: 'NS_FAILED(rv)', file d:/mozilla/gecko-dev/netwerk/protocol/http/nsHttpChannel.cpp, line 5945
> [Parent 3300] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file d:/mozilla/gecko-dev/widget/windows/nsDataObj.cpp, line 86
> [Parent 3300] WARNING: NS_ENSURE_TRUE(pStream) failed: file d:/mozilla/gecko-dev/widget/windows/nsDataObj.cpp, line 1622
>

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Freddy, I am traveling at the moment, any chance you could take a look at this one? Worst case I take a look once I get back home.

Comment 6

[fireattack [:fireattack]]

Someone please add "regression" tag and block of bug 664717.

Comment 7

[fireattack [:fireattack]]

Regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=46045ec8a4aa09a341b7209170089ade3c27c1a5&tochange=71c8d652e638dd67c5c56879df83d76b30033458

Comment 8

[Gerry Chang [:gchang]]

Too late for 51 and we've built 52 RC. Mark 51 won't fix.

Comment 9

[Frederik Braun [:freddy]]

Sorry, no spare cycles currently. Can you take this yourself, Christoph?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Cristoph, maybe for the 55/54 timeframe?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Cristoph, maybe for the 55/54 timeframe?

Finally I got time to look at this problem. It seems the page is setting a CSP of:
> default-src 'none'; style-src 'unsafe-inline'
but I couldn't find any indication that CSP blocks the actual load, there are no internal calls into the CSP code and there is also no error logged to the console that CSP would block that load as indicated in comment 3. However, copying the image does *not* work, so there must be some other problem that blocks the 'copy image' mechanism.

Hector, I guess I have to push that back onto your plate, since your code introduced the regression. If there are more CSP related problems I am happy to assist to get this fixed.

Comment 12

[fireattack [:fireattack]]

Any chance to higher the priority of this bug? It's VERY annoying in everyday use, especially for Chinese users: this bug essentially means people can't copy an image from Firefox to QQ, a very popular IM software over there, and I've got at least 3 friends complained this to me.

Comment 13

[Gerry Chang [:gchang]]

Mark 54 fix-optional as there are no actions for the moment but still happy to have the fix in 54.

Comment 14

[Hector Zhao [:hectorz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> 
> Finally I got time to look at this problem. It seems the page is setting a
> CSP of:
> > default-src 'none'; style-src 'unsafe-inline'
> but I couldn't find any indication that CSP blocks the actual load, there
> are no internal calls into the CSP code and there is also no error logged to
> the console that CSP would block that load as indicated in comment 3.

I just refreshed my local m-c clone and tested my debug build again.

One detail I failed to mention before: when you copy the image, the error message in browser console and the command window won't be there; they're only shown when you try to paste the image.

fireattack also reported seeing similar error message in bug 1342740 comment 3.

> 
> Hector, I guess I have to push that back onto your plate, since your code
> introduced the regression. If there are more CSP related problems I am happy
> to assist to get this fixed.

Hopefully with information above, you'll be able to reproduce it? Thanks.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Hector Zhao [:hectorz] from comment #14)
> Hopefully with information above, you'll be able to reproduce it? Thanks.

Thanks for the additional information. I just checked out the latest revision from mozilla-central on Linux and successfully copied the image using 'right-click->copy-image' into LibreOffice. Seems to work. I also can copy and paste the image from Bug 1342740.

Did something change in the meantime or are we missing something?

Comment 16

[Hector Zhao [:hectorz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> (In reply to Hector Zhao [:hectorz] from comment #14)
> > Hopefully with information above, you'll be able to reproduce it? Thanks.
> 
> Thanks for the additional information. I just checked out the latest
> revision from mozilla-central on Linux and successfully copied the image
> using 'right-click->copy-image' into LibreOffice. Seems to work. I also can
> copy and paste the image from Bug 1342740.
> 
> Did something change in the meantime or are we missing something?

My patch in bug 664717 only introduced the file promise flavor on Windows, and this bug doesn't exist on Linux.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Hector Zhao [:hectorz] from comment #16)
> My patch in bug 664717 only introduced the file promise flavor on Windows,
> and this bug doesn't exist on Linux.

Ah, I see, then we need someone with a Windows machine to take a look at this one. Kamil, any chance you could take a look? Happy to guide you through.

Comment 18

[Kamil Jozwiak [:kjozwiak]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Ah, I see, then we need someone with a Windows machine to take a look at
> this one. Kamil, any chance you could take a look? Happy to guide you
> through.

I'm seeing the same errors that Hector mentioned in comment#3 when going through the STR from comment#0. Whenever I paste the image, I get the following CSP error under the browser console:

"Content Security Policy: The page’s settings blocked the loading of a resource at https://raw.githubusercontent.com/TeamNewPipe/NewPipe/master/assets/new_pipe_icon_5.png (“default-src 'none'”)."

While using a local debug build, I'm getting the following error whenever I paste the image once it's been copied:

[Parent 1340] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file c:/code/mozilla-central/dom/security/nsContentSecurityManager.cpp, line 478
[Parent 1340] WARNING: 'NS_FAILED(rv)', file c:/code/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 6128
[Parent 1340] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file c:/code/mozilla-central/widget/windows/nsDataObj.cpp, line 85
[Parent 1340] WARNING: NS_ENSURE_TRUE(pStream) failed: file c:/code/mozilla-central/widget/windows/nsDataObj.cpp, line 1689
[Parent 1340] WARNING: NS_ENSURE_TRUE(mCacheEntry) failed: file c:/code/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 5011

Even with the above errors, pasting the image worked without any issues under both Google Docs and Microsofts WordPad. However, when attempting to paste the image into LibreOffice, I'm getting the following error on both machines:

"Requested clipboard format is not available."

Copying/Pasting the same image within Chrome into LibreOffice works without any issues so there might be a problem with how FX is storing that particular image in the clipboard before it's pasted which is causing issues with LirbeOffice.

Christoph, want me to see if I can find a regression range?

Comment 19

[fireattack [:fireattack]]

..the regression range has already be posted.

Comment 20

[Kamil Jozwiak [:kjozwiak]]

(In reply to fireattack from comment #19)
> ..the regression range has already be posted.

Ahh thanks :) Missed that comment when I was going through the thread. Clearing the ni?.

Christoph, let me know if there's anything else that I can do here.

Comment 21

[fireattack [:fireattack]]

Let me provide some information I gathered during my investigation before, because it seems no one seem to mention it thoroughly here.

First of all, the bug can be reproduced by the image provided by OP, but it has another weird aspect I want to mention. Sometimes, you can't copy an image in a webpage, but it works totally fine when you open the same image directly.

So below, I'm going to use this URL as an example: https://twitter.com/MacciattoS2/status/835711163681783808 (the main image), and also compare it with the same direct image, https://pbs.twimg.com/media/C5kK1u3UwAAWHJk.jpg

As you already knew, it doesn't cause problem when pasting on all programs because they all deal with pasting differently. Thanks for the mentioning LibreOffice, that's a better program to help people reproduce the bug (than the one I used before, QQ). I will use it below as well. I also used a freeware called "Clipboard Format Spy" (http://delphidabbler.com/software/cfs/main) to help me investigate the clipboard.

1) Good behavior (before 2016-08-02 build, I tested on 2016-08-01 build via mozregression):

a) Copy the main image by "Copy image" on https://twitter.com/MacciattoS2/status/835711163681783808 

What the clipboard looks like: http://i.imgur.com/iR4iRD6.png

It has only CF_DIB and CF_DIBv5 (which are the bitmap images). If you try to copy it to LibreOffice, it works perfectly.

b) Copy the image by "Copy image" on the direct URL, https://pbs.twimg.com/media/C5kK1u3UwAAWHJk.jpg

What the clipboard looks like: http://i.imgur.com/RjDrLny.png They're basically same as a). So needless to say, you can copy this to the LibreOffice without problem too.

2) Bad behavior (I used the newest Nightly to test):

a) Copy the main image by "Copy image" on https://twitter.com/MacciattoS2/status/835711163681783808 

What the clipboard looks like: http://i.imgur.com/Oep9GO2.png

There is a new one in clipboard, CF_HDROP. If I understand it correctly, it gives you the path/reference to a certain local file, instead of the bitmap itself. Just like what will happen when you copy a file in file explorer. Those affected programs such as LibreOffice, will prefer this over CF_DIB and CF_DIBv5. 

The problem here, though, is that the image doesn't actually exist. If you double-click that row in Clipboard Format Spy, it shows the file path is "C:\Users\ADMINI~1\AppData\Local\Temp\C5kK1u3UwAAWHJk.jpg", but there is no such file on my disk. 

Therefore, if you try to paste it in LibreOffice, it shows error.

b) Copy the image by "Copy image" on the direct URL, https://pbs.twimg.com/media/C5kK1u3UwAAWHJk.jpg

What the clipboard looks like: http://i.imgur.com/QgDPMXg.png

While this is the same as a), but this time, the image (C:\Users\ADMINI~1\AppData\Local\Temp\C5kK1u3UwAAWHJk.jpg) actually exists on my disk (I can find it and open it). So, it can be successfully pasted into LibreOffice.

So, apparently, to achieve animated GIF copy, Mr. Zhao did the following workaround: everytime you copy an image, Firefox will try to create a cache in %temp% folder. And then, it copies the file path (in addition to the bitmap), so it can be pasted as an animated GIF, instead of just the first frame. However, for some reason, the process of creating temp file sometimes is blocked by that "Content Security Policy" error/warning. As a result, when you try to paste an invalid file into some program, it doesn't work. Some problems work better because they either prefer CF_DIB to begin with, or they're more robustly built so when CF_HDROP fails they will fallback to CF_DIB or CF_DIBv5.

Comment 22

[:Gijs (he/him)]

Renewing the needinfo given the latest comments. Christoph, if your plate is full, can you escalate to someone else to find an owner, given that this is tracking 54? :-)

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Francois, Wennie mentioned you might be able to take a look at this one. If not, please let me know!

Comment 24

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)
> Francois, Wennie mentioned you might be able to take a look at this one. If
> not, please let me know!

The earliest I can dig into this would be sometime next week.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to François Marier [:francois] from comment #24)
> The earliest I can dig into this would be sometime next week.

Wennie, can we have someone else look into this problem?

Comment 26

[Wennie]

Francois is looking into this bug.

Comment 27

[François Marier [:francois]]

(In reply to Wennie from comment #26)
> Francois is looking into this bug.

The one I'm looking at is bug 1273241, not this one.

Comment 28

[Mike Taylor [:miketaylr]]

Seems like this bug has sorta fallen off the radar. Is this important for 57?

Comment 29

[fireattack [:fireattack]]

If you don't mind an user story.. I'd say this is "crucial", and should be fixed ASAP.

First of all, this is a regression. And it affects a very common operation which could happen daily to users: copy an image from Firefox to other program.

This is particularly true for the users in China, since it sometimes prevents users to copy an image to the most popular IM program there, QQ. I've got multiple friends complaining this problem to me :/

Comment 30

[fireattack [:fireattack]]

Also it's worth to mention, this regression is caused by a patch that tried to fix copy animated GIF problem.

While it's nice to be able to copy animated GIF, that problem had been in Firefox for at least 10 years (and still existing in almost all non-Microsoft browsers). So IMO that's not urgent. 

If we can't come up with a fix to solve both in time, I'd say reverting back to the original status (can't copy animated GIF, but no bug otherwise) is better than status quo.

Comment 31

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
20180112220334

Could someone also look at bug 1430390? It looks like a possible duplicate based on the actual results. But unlike this STR, it's only reproducible from the Google Images results, not when the image is opened directly.

As for this bug, I can't reproduce it: the image is pasted properly to the desktop.

Comment 33

[Hector Zhao [:hectorz]]

(In reply to Hector Zhao [:hectorz] from comment #3)
> My initial attemp doesn't work

I think I finally have a fix for this bug, will push my patch for review in a moment.

Comment 34

[Hector Zhao [:hectorz]]

Attachment: Bug 1340039 - Set contentPolicyType when copying image, and pass it between processes.

Review commit: https://reviewboard.mozilla.org/r/227664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227664/

Comment 35

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

Hector, could you explain the patch a bit? The patch looks reasonable, but it isn't clear to me where the right contentpolicytype helps.

Comment 36

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

Is it because of https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/widget/windows/nsDataObj.cpp#75? And something somewhere compares the content policy type, and if it doesn't match, it fails?

Comment 37

[Hector Zhao [:hectorz]]

(In reply to Olli Pettay [:smaug] from comment #36)
> Is it because of
> https://dxr.mozilla.org/mozilla-central/rev/
> 6ff60a083701d08c52702daf50f28e8f46ae3a1c/widget/windows/nsDataObj.cpp#75?
> And something somewhere compares the content policy type, and if it doesn't
> match, it fails?

Yes, it looks like this bug was caused by not correctly marking the channel as loading an image, and it being checked against other CSP directives. For example, on Twitter, it's complaining about `default-src`, where there're more specific `img-src` defined allowing the load.

Comment 38

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

Comment on attachment 8958741 [details]
Bug 1340039 - Set contentPolicyType when copying image, and pass it between processes.

https://reviewboard.mozilla.org/r/227664/#review233426

Comment 39

[Hector Zhao [:hectorz]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=963192431785bca5945b7cc4ad5e289a92047e00

1) failure of M-e10s(bc1) on osx 10.10 opt seems to be intermittent, both of my two retriggered jobs succeeded,
2) failures of M-e10s(cl)/R-e10s jobs seems to be results of my specifying test paths with `./mach try fuzzy`. I did another try push w/o test paths, it's not complete yet, but the completed M-e10s(cl)/R-e10s jobs are all green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3421edd0d509eae56a0c3574904b0c385234b70f

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/445ec9da938f
Set contentPolicyType when copying image, and pass it between processes. r=smaug

Comment 41

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/445ec9da938f

Comment 42

[Yanfang Liu]

Verified with Nightly 61.0a1 build 20180318220108 on Windows 10 64 bit.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Did you want to let this ride the 61 train to release or nominate it for Beta approval?

Comment 44

[Hector Zhao [:hectorz]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Did you want to let this ride the 61 train to release or nominate it for
> Beta approval?

Thanks for reminding, I'll nominate it for Beta since 60 will become ESR.

Comment 45

[Hector Zhao [:hectorz]]

Comment on attachment 8958741 [details]
Bug 1340039 - Set contentPolicyType when copying image, and pass it between processes.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 664717 (Fx 51)
[User impact if declined]: Bug where users cannot copy image from certain CSP enabled webpages into other native applications exists in next ESR, Fx 60
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, see comment 42
[Needs manual test from QE? If yes, steps to reproduce]: Not from global QE team
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Manually verified. Logic similar to existing code(isPrivateData & requestingPrincipal of nsITransferable), more like a correctness fix
[String changes made/needed]: None

Comment 46

[Julien Cristau [:jcristau]]

Comment on attachment 8958741 [details]
Bug 1340039 - Set contentPolicyType when copying image, and pass it between processes.

fix copying images across processes, beta60+

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/485d4b89c0cd

Comment 48

[Yanfang Liu]

Verified with Firefox 60.0b7 build 20180326164103 on Windows 10 64 bit.

Comment 49

[fireattack [:fireattack]]

This is *still* broken in some rare cases. 

Example (SFW): https://e-hentai.org/s/b4cb449778/1217096-4

Comment 50

[fireattack [:fireattack]]

The cause is still the same as I described in bug 1342740: it places a invalid file in CF_DHROP of clipboard.

Comment 51

[Hector Zhao [:hectorz]]

(In reply to fireattack from comment #49)
> This is *still* broken in some rare cases. 
> 
> Example (SFW): https://e-hentai.org/s/b4cb449778/1217096-4

(In reply to fireattack from comment #50)
> The cause is still the same as I described in bug 1342740: it places a
> invalid file in CF_DHROP of clipboard.

This doesn't look like it's blocked by CSP, maybe it's because the image is considered mixed content. Please file another bug, thanks.

Comment 52

[fireattack [:fireattack]]

Filed bug 1458453.