1231429 - Dragging and dropping an image to Windows Explorer does not save the image.

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/40b58759c962bf1a8dbf8b56a7227778dfdcfa50
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 ID:20151209030228

This problem happens only without e10s.

Steps To Reproduce:
0. Disable e10s
1. Open twitter e.g https://twitter.com/FirefoxNightly (not need log-in)
2. Dragging and dropping an image to Explorer

Actual Results:
Nothing happens

Expected Results:
The image should be saved.


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=39172e1b1480f7c425d6de4285fe50025b9fd0c3&tochange=7bd651ae54dd6b090555d4420f34525de3fda931

Suspect: Bug 1192945

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Jonas, Jim, before landing Bug 1192945 have discussed potential regressions within Comment 5 and Comment 6 of Bug 1192945. It seems that dragging an image out of content is failing now. Most likely because we are performing a CheckLoadURI-check now which we haven't done before. If we want to fix this problem, we have to pass a different security flag here [1]. I am not sure if we want though. How do you feel about it?

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd651ae54dd6b090555d4420f34525de3fda931

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The first question we should ask is what security checks do we want to do for a drag'n'drop?

Ideally we wouldn't do a network request at all when a drag'n'drop happens. And we'd always just use the data that we already have and use to render the image. That way we are sure that no security issues can happen, and we're also sure that the user saves what they see.

But if we in some cases do need to do a network request, then we should decide which security checks to do on that request. We could decide to not do any checks, do the same checks as when the image was originally loaded, or something else.

I'll leave it to the security team to make that decision.

Comment 3

[Jim Mathies [:jimm]]

I'm curious when we would need to do network requests. The one case I can think of might be private browsing, but I'm not sure. I'd prefer we just use whatever we have in the cache and not initiate a network request at all.

FWIW I tested dragging an image out to the desktop using nightly and the drag image was visible for both an e10s and non-e10s window.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jim Mathies [:jimm] from comment #3)
> I'm curious when we would need to do network requests. The one case I can
> think of might be private browsing, but I'm not sure. I'd prefer we just use
> whatever we have in the cache and not initiate a network request at all.
> 
> FWIW I tested dragging an image out to the desktop using nightly and the
> drag image was visible for both an e10s and non-e10s window.

I'll have a look soon. In the meantime: Alice, can you still reproduce this issue?

Comment 5

[Alice0775 White]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> (In reply to Jim Mathies [:jimm] from comment #3)
> > I'm curious when we would need to do network requests. The one case I can
> > think of might be private browsing, but I'm not sure. I'd prefer we just use
> > whatever we have in the cache and not initiate a network request at all.
> > 
> > FWIW I tested dragging an image out to the desktop using nightly and the
> > drag image was visible for both an e10s and non-e10s window.
> 
> I'll have a look soon. In the meantime: Alice, can you still reproduce this
> issue?


Yes, I can reproduce the problem on latest Nightly46.0a1 when disabled e10s.

https://hg.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20151216030229

Comment 6

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

Tracking for FF44 at the moment but I would not block the release on this one. As there are other ways of saving images as a workaround. Though I will be more than happy to uplift a fix in 44 if it is ready in time.

Comment 7

[Alice0775 White]

Please back out bug 1192945 unless dev fix the regression.
I do not understand, I reported this bug prior to release 43(so still in beta). but the dev ignored this.
I hate rapid release, because Beta version is meaningless.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Ritu Kothari (:ritu) from comment #6)
> Tracking for FF44 at the moment but I would not block the release on this
> one. As there are other ways of saving images as a workaround. Though I will
> be more than happy to uplift a fix in 44 if it is ready in time.

IMO, broken drag and drop out of content should always block. We need to get someone on this, maybe start with QA to figure out which platforms are affected.

Comment 9

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

Jimm, if you insist, I can reconsider :) If you can suggest a dev who I can ping for investigation/fix, please let me know.

Hi Florin, can someone from the QE team help determine which platforms is this functionality broken on? The bug report is about windows.

Comment 10

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting ni to Andrei, so we can get someone to investigate which platforms this reproduces on (as soon as time allows us).

Comment 12

[Loic]

Good link to test provided from the dupe.

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Attachment: Screenshot with error

Tested with the following builds:
- Firefox 43.0.4 build 3
- Firefox 44 beta 7
- Latest Developer Edition 45.0a2
- Latest Nightly 46.0a1

Tested on the following platforms:
- Windows 10 64 desktop - broken across builds
- Windows 8.1 64 desktop - broken across builds
- Windows 10 64 Dell XPS 12 ultrabook - broken across builds
- Windows XP 32 desktop - broken across builds
- Ubuntu 14.04 64 - Unaffected! Works across builds
- Mac OS X 10.10.5 - Unaffected! Works across builds

Notes:
- Based on my testing this is Windows Only!
- On Windows XP there is no notification or something else after dragging the image, but on other platforms there is an alert: Error Moving File or Folder. - see attachment.

Comment 14

[Loic]

On Win 7, I didn't see any error notification from Windows, it fails silently.
Probably proper to Win 8/10.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

It's really unfortunate that this bug slipped through the cracks. Anyway, I am debugging the problem at the moment with jimm and it seems that the problem is specific to CSP and it doesn't occur on pages that are not serving CSP.

Now that we are using asyncOpen2 we are consulting CSP when dragging an image out of content. Since that new channel using TYPE_OTHER as the contenttype the load is governed by the CSP directive default-src hence we see the following error in the console:

Content Security Policy: The page's settings blocked the loading of a resource at https://pbs.twimg.com/profile_images/378800000086053092/0e4495d8e33ec2d72fcfb786661ce7f7_400x400.png ("default-src https://twitter.com").


At this point I am not sure how to fix the issue. One potential fix would be to use the systemPrincipal, but I rather would not use the systemPrincipal for that purpose. Using TYPE_IMAGE is also not right, because I suppose that channel can also be used dragging other resources out of content. At least we have identified the problem. Open for suggestions.

Comment 17

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

It's poor timing that this issue was not fixed in Fx44, I am about to gtb Beta9 (last desktop beta) of Beta44. This is a wontfix. While I do believe this is a mainline scenario, I don't think this is a release blocker. Let's hope we can continue the investigation and land a fix promptly in Aurora45.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ritu Kothari (:ritu) from comment #17)
> It's poor timing that this issue was not fixed in Fx44, I am about to gtb
> Beta9 (last desktop beta) of Beta44. This is a wontfix. While I do believe
> this is a mainline scenario, I don't think this is a release blocker. Let's
> hope we can continue the investigation and land a fix promptly in Aurora45.

As disccussed over IRC with ritu and jimm, we are going to back out the change within:
https://bugzilla.mozilla.org/show_bug.cgi?id=1192945#c10
to fix the regression within that bug.

Comment 19

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

Hi Florin, here is another fix (backout) mostly we took in 44.0b9. Could you please do some focused testing to ensure all is well? Many thanks!

Comment 20

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

I backed out bug 1192945 on beta, aurora and fx-team, so this should be fixed now.

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Ritu Kothari (:ritu) from comment #19)
> Hi Florin, here is another fix (backout) mostly we took in 44.0b9. Could you
> please do some focused testing to ensure all is well? Many thanks!

Bogdan is going to verify this on 44.0b9 as well.

Comment 22

[Bogdan Maris, Desktop Test Engineering]

I tested using Firefox 44 beta 9 on Windows 10 64-bit, Windows 8.1 64-bit, Windows 7 64-bit and Windows XP 32-bit, and I can confirm that the issue is fixed. Leaving this open until its verified on FF 45 and 46.

Comment 23

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

Tracking for 45+ as this is a recent regression. 
Bogdan, if you can verify on 45 and 46 that would be great, some time next week before the merge. Thanks :)

Comment 24

[Bogdan Maris, Desktop Test Engineering]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Tracking for 45+ as this is a recent regression. 
> Bogdan, if you can verify on 45 and 46 that would be great, some time next
> week before the merge. Thanks :)

Sure! Just finished verifying the fix on latest Developer Edition 45.0a2 and latest Nightly 46.0a1 using the same OS's as in comment 22.