1399429 - Downloads in Private Browsing window end up in main window Download List

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Currently Nightly on Linux:

1) New Private Window
2) Start multiple downloads from the same site (I believe the max is 6?)
3) Observe that *most* downloads go to the download button in the private window
4) Observe some of the downloads go "missing"
5) Bring normal browsing window to front
6) Observe that the missing downloads are listed on the normal window
7) Observe that they are listed in the main history

This effectively breaks the privacy of private browsing.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

I believe this is a recent regression, probably started breaking < 2 weeks ago.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I'm not entirely sure what triggers the "download goes to the wrong window", so I'm not sure if I can get a reliable regression range.

Comment 3

[Josh Matthews [:jdm]]

Do you have a convenient URL to test this on?

Comment 4

[Josh Matthews [:jdm]]

I couldn't reproduce this clicking random large links from http://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

I could reproduce it after downloading a pile of stuff from:

https://archlinux.cu.be/iso/
https://archlinux.cu.be/community-staging/os/

10 Downloads went to Private Browsing, 2 ended up in the main window

Comment 6

[Josh Matthews [:jdm]]

I'm on the 9/10 macOS nightly and still can't reproduce the problem; it reaches the point where I've got ~11 simultaneous files downloading according to the private window's download manager and none of the other links respond until one of them is finished or cancelled.

Comment 7

[Josh Matthews [:jdm]]

We should keep an eye on this; breaking private browsing generally requires a dot release.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

I've got a theory about this bug.

I noticed when this happened again that my system drive (which has /tmp) was close to filling up. Looking closer showed that the files that were downloaded in private browsing were being stored in /tmp/mozilla_username0, whereas the downloads that incorrectly were happening in the main window were not (so they're presumably downloaded directly to the selected dir).

Is it possible that we try to store the files in /tmp, see they're not going to fit, and that failure isn't handled correctly and the files end up going to the main window downloads instead?

(As far as I'm concerned, the fact that those downloads go via /tmp is a serious bug in itself, especially as in this case of downloading many huge "Linux ISO's" it's filling an SSD repeatedly to the brink with many gigabytes of data that don't actually need to be there to begin with)

This would also explain:

a) Why you can't reproduce it with the STR
b) Why this behaved like a regression: my system drive has been filling up more.

Also, at least in one case I've seen the files left behind in /tmp/mozilla_username0, which would also be an issue as I believe many desktop Linux distro's don't actually clean /tmp any more to deal with faulty apps.

Comment 9

[Julien Cristau [:jcristau]]

per comment 7

Comment 10

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

gcp, ehsan, given that this may be a privacy issue, can you help find someone to work on this? 
Maybe more realistic to tackle for 58, but if we land a fix, it could still make it into 57.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Ehsan and Josh own the component and they're in this bug.

FWIW, I re-partitioned my drive to increase the space on my root (which has /tmp), but this didn't change anything. It seems that the download "moves" to the main window if the number of active downloads for a given host > 6.

Comment 12

[(no longer active)]

It seems to me that the immediate next step is for someone in QA to try to use the information in comment 8 and comment 11 to come up with steps to reproduce, and based on that with a regression window.  Ryan, do you mind finding someone who can run this through some tests please?

Comment 13

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Hello,

I've tried to reproduce the issue on Mac 10.12, Arch Linux x64, Ubuntu 15.04 using the latest Nightly 58.0a1, Build ID 20171010220102, but I have not managed to do so.

While in a private window I have downloaded dummy files from https://www.thinkbroadband.com/download and actual files from https://archlinux.cu.be/iso/. I've had 9 concomitant downloads from the same source and none of them leaked in my normal window.

While on Ubuntu and Arch, I've also observed that none of the downloads that I have made in a private window went to "/tmp/mozilla_username0". 

All of the above tests were made on systems which had very little storage left (less than 2GB).

@gcp, have you changed the default download location by any chance? When I used new profiles my downloads always defaulted to  "/Users/username/Downloads" for Mac and "/home/user/Downloads" on Linux.
Also, do you have any addons installed? If yes, can I have a list of them?
And lastly, it's a long shot, but have you tried reproducing the issue on a new profile?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

I have Downloads set to "Always Ask" for location. 

AddOns are uBlock Origin, wxIF and Yet Another Twitter Link Expander. I wrote the last 2 and I can guarantee you they're unrelated, and I doubt uBlock Origin is the cause (how could it be? They're all WebExtensions with no access to any of the download related code!),

It's notable to me that your downloads did not go to the /tmp directory. Maybe that's affected by the "Always Ask" settings?

It's also interesting to me that you claim that you had 9 simultaneous downloads from the same server. As far as I know, our network layer caps at 6 (Not sure about the setting but I think it's "network.http.max-persistent-connections-per-server=6" or something like it?).

Comment 15

[Gian-Carlo Pascutto [:gcp]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #14)
> I have Downloads set to "Always Ask" for location. 

Whoops, that's not true!

I am right-clicking on the links and selecting "Save Link As" from the context menu.

Sorry for not making this clear. This workflow is automatic for me.

Comment 16

[Andrew Overholt [:overholt]]

Panos, can someone take a look here?

Comment 17

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

This looks like a perfect fit for Paolo.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

I now have enough understanding of the conditions that trigger this that I may be able to produce a regression range.

Comment 19

[:Paolo Amadini]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #14)
> how could it be? They're all WebExtensions with
> no access to any of the download related code!

I think some extensions might request special permissions to alter network requests. This could have an effect, or possibly the extension could trigger a bug in the WebExtensions API implementations. So, it's not to rule out entirely.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #15)
> I am right-clicking on the links and selecting "Save Link As" from the
> context menu.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> Looking closer showed that the files that were
> downloaded in private browsing were being stored in /tmp/mozilla_username0,
> whereas the downloads that incorrectly were happening in the main window
> were not (so they're presumably downloaded directly to the selected dir).

That's quite important, the context menu is timing dependent and can result in two different code paths being followed to download the file. The bug may affect just one of them. You can put breakpoints here to see which one is being used:

https://dxr.mozilla.org/mozilla-central/rev/3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf/browser/base/content/nsContextMenu.js#1102-1104,1113-1114

When going through the external helper applications service, the privacy status of the channel is checked here:

https://dxr.mozilla.org/mozilla-central/rev/bc7a5be76b723cf6aac1a919156e74997c5f4902/uriloader/exthandler/nsExternalHelperAppService.cpp#2147

I found that bug 1367581 was related to the function that checked the origin attributes. You can put a breakpoint here to see what gets actually passed when the download is initialized:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#246-254
https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2147

I remember we also had a very rare instance with C++/JavaScript glue code on 64 bit that caused a similar issue because the wrong boolean value was passed to the function, though I can't find the bug report now, I only found the more recent and incomplete bug 1340455.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

I'm bisecting this with mozregression now: 

238:33.61 INFO: Narrowed nightly regression window from [2017-07-28, 2017-08-12] (15 days) to [2017-08-05, 2017-08-12] (7 days) (~2 steps left)
238:33.61 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=933a04a91ce3bd44b230937083a835cb60637084&tochange=80ff3f300e05f38f96c385b03d1973a966a2bd35

This seems to implicate bug 1388145 or bug 1382899.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

278:02.88 INFO: Last good revision: 64f5e3f40f0e7accb12cd90080e58eff0be46383 (2017-08-11)
278:02.88 INFO: First bad revision: 80ff3f300e05f38f96c385b03d1973a966a2bd35 (2017-08-12)
278:02.88 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=64f5e3f40f0e7accb12cd90080e58eff0be46383&tochange=80ff3f300e05f38f96c385b03d1973a966a2bd35

Hmm, I don't see anything that seems relevant in that log. Either I got fooled because it's not 100%, or it's something not so obvious.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

The bisection ended up at the somewhat nonsensical:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ebd299dd19d6f53ad7a22b55bd283911f77e0a88&tochange=3265cc2501b6d23f2ece3cc27eaa6d87401bc7bc

So I either made a mistake or it's not as reliable to trigger as I thought.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

I tried to setup a test area on my own webserver to more easily and reliably test this, but I ran into the problem that I was able to start many simultaneous downloads without problems. I think that this is because I support HTTP2. On many other servers the maximum number of downloads is capped at 6, and as said, I think it's related to the setting mentioned earlier, so probably this needs a server that supports HTTP KeepAlive but not HTTP2, or something... :-/

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Confirmed. Disabling HTTP2 makes it reproducible on my server. Going to try bisecting again!

Comment 25

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=604fd3302562dc2059a8de3231818a1b618b8ffe&tochange=bf7793529f82ab9cb885a62446ed0f3e18c51634

Bug 1360406 regressed this.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Okay, so the exact sequence is this:

1) Open Firefox
2) Open Private Browsing window
3) Go to a server supporting HTTP KeepAlive, but not using HTTP2. (This is fairly typical)
4) Find some big downloads
5) Right click the downloads, select Save As
6) This will work 6 times. The 7th time, it will:
6a) In a working Firefox, simply fail silently, because the maximum connection limit is reached.
6b) In a broken Firefox, it will start the download in the main window.

There might an implication on the network layer here as well, because broken Firefoxes are clearly not respecting that preference limit. Which might be no harm or don't-care, but worth pointing out.

Comment 27

[Ian Moody [:Kwan] (UTC+0)]

Slightly simpler STR:

1) Open Firefox
2) Open a Private Browsing window
3) Go to http://kwan.perix.co.uk/mozilla/cpow/pbTest.html
4) Right-click the link, Save Link As... (wait a little)
5a) In 56 it starts in the PB window
5b) In nightly58 it appears in the main window

Comment 28

[Ian Moody [:Kwan] (UTC+0)]

So it seems like the PB contentWindow detection added in bug 1360406 never worked, but fortunately I don't think it's ever used _except_ for when we wait too long for response headers for the download and give up (though I could be wrong about that).
This[1]:

let isContentWindowPrivate = this.isRemote ? this.ownerDoc.isPrivate : undefined;

is always undefined, because this.ownerDoc.isPrivate seems to always be undefined, which seems to come from here[2]:

isPrivate: context.target.ownerDocument.isPrivate,

because context.target.ownerDocument.isPrivate is always undefined.  I don't know if it's supposed to be, but fortunately PBUtils is included in ContextMenu.jsm (and never used), so we can just do PrivateBrowsingUtils.isBrowserPrivate() on the ownerDocument to get what we want.  Patch to do so incoming soonish.

This is only broken in e10s.

[1] https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/browser/base/content/nsContextMenu.js#1204
[2] https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/browser/modules/ContextMenu.jsm#662
[3] https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/browser/base/content/nsContextMenu.js#1219

Comment 29

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1399429 - Properly determine if the content window is private for the contextMenu.

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

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8918409 [details]
Bug 1399429 - Properly determine if the content window is private for the contextMenu.

https://reviewboard.mozilla.org/r/189236/#review194540

Comment 31

[Ian Moody [:Kwan] (UTC+0)]

Thanks Felipe!

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d17802fd84705a61994c5d145915dfa7c62498e

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0f88980b79c
Properly determine if the content window is private for the contextMenu. r=Felipe

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a0f88980b79c

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Potential privacy leak - please request Beta approval on this when you get a chance.

Comment 35

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch for beta with bug 1408670 correction folded in

Approval Request Comment
[Feature/Bug causing the regression]: bug 1360406
[User impact if declined]: A website viewed in a private browsing (PB) window can set cookies in a user's non-private window by getting the user to right-click > "Save As" a link (and the download will appear in the regular download history)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes.
1) Open a regular Firefox window
2) Open a Private Browsing window
3) Go to http://kwan.perix.co.uk/mozilla/cpow/pbTest.html
4) Right-click the second link, Save Link As... (wait a little for the window to appear), then save to disk
5a) In a fixed build the download starts in the PB window
5b) In a broken build it appears in the non-PB window, and sets a cookie in the non-private store
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minor code change, from trying to check PB state by checking a non-existent property on content window, to calling specialised PB checking function.
[String changes made/needed]: none

Comment 36

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

Comment on attachment 8919892 [details] [diff] [review]
Patch for beta with bug 1408670 correction folded in

Must fix, Beta57+

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/be62e8e756ba

Comment 38

[roxleitan]

Reproduced the issue on Nightly 57.0a1 (2017-09-13) using the STR from comment 35.

Verified fixed using the latest Nightly 58.0a1 (2017-10-18) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.

Comment 39

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 35 using an affected Firefox 57.0a1 (2017-09-13) build.

This issue is verified fixed using Firefox 57.0b10 (BuildId:20171019140425) on Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit.