Closed Bug 1399429 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Private Browsing, defect)

Product:
Firefox
Component:
Private Browsing
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + verified
firefox58 --- verified

People

(Reporter: gcp, Assigned: Kwan)

References

Details

(Keywords: regression, Whiteboard: regression)

Attachments

(2 files)

Bug 1399429 - Properly determine if the content window is private for the contextMenu.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Patch for beta with bug 1408670 correction folded in
1.24 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
I believe this is a recent regression, probably started breaking < 2 weeks ago.
Whiteboard: regression
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.
Do you have a convenient URL to test this on?
I couldn't reproduce this clicking random large links from http://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/.
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
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.
We should keep an eye on this; breaking private browsing generally requires a dot release.
tracking-firefox57: --- → ?
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.
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.
Flags: needinfo?(gpascutto)
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(gpascutto)
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?
Flags: needinfo?(ehsan) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
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?
Flags: needinfo?(rares.bologa) → needinfo?(gpascutto)
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?).
Flags: needinfo?(gpascutto)
(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.
Panos, can someone take a look here?
Flags: needinfo?(past)
This looks like a perfect fit for Paolo.
Flags: needinfo?(past) → needinfo?(paolo.mozmail)
I now have enough understanding of the conditions that trigger this that I may be able to produce a regression range.
(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.
Flags: needinfo?(paolo.mozmail)
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.
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.
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.
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... :-/
Confirmed. Disabling HTTP2 makes it reproducible on my server. Going to try bisecting again!
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.
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
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
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
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
Attachment #8918409 - Flags: review?(felipc) → review+
Keywords: checkin-needed
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a0f88980b79c
Properly determine if the content window is private for the contextMenu. r=Felipe
https://hg.mozilla.org/mozilla-central/rev/a0f88980b79c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1408670
Potential privacy leak - please request Beta approval on this when you get a chance.
Flags: needinfo?(moz-ian)
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
Flags: needinfo?(moz-ian)
Attachment #8919892 - Flags: approval-mozilla-beta?
Comment on attachment 8919892 [details] [diff] [review]
Patch for beta with bug 1408670 correction folded in

Must fix, Beta57+
Attachment #8919892 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
status-firefox58: fixed → verified
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Flags: in-qa-testsuite?
Flags: in-qa-testsuite? → in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: