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)
Firefox
Private Browsing
Tracking
()
VERIFIED
FIXED
Firefox 58
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)
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
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.
Reporter | ||
Comment 1•7 years ago
|
||
I believe this is a recent regression, probably started breaking < 2 weeks ago.
Whiteboard: regression
Updated•7 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Do you have a convenient URL to test this on?
Comment 4•7 years ago
|
||
I couldn't reproduce this clicking random large links from http://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/.
Reporter | ||
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
We should keep an eye on this; breaking private browsing generally requires a dot release.
tracking-firefox57:
--- → ?
Reporter | ||
Comment 8•7 years ago
|
||
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 10•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox58:
--- → ?
Reporter | ||
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
Comment 13•7 years ago
|
||
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)
Reporter | ||
Comment 14•7 years ago
|
||
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)
Reporter | ||
Comment 15•7 years ago
|
||
(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 17•7 years ago
|
||
This looks like a perfect fit for Paolo.
Flags: needinfo?(past) → needinfo?(paolo.mozmail)
Reporter | ||
Comment 18•7 years ago
|
||
I now have enough understanding of the conditions that trigger this that I may be able to produce a regression range.
Comment 19•7 years ago
|
||
(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)
Reporter | ||
Comment 20•7 years ago
|
||
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.
Reporter | ||
Comment 21•7 years ago
|
||
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.
Reporter | ||
Comment 22•7 years ago
|
||
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.
Reporter | ||
Comment 23•7 years ago
|
||
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... :-/
Reporter | ||
Comment 24•7 years ago
|
||
Confirmed. Disabling HTTP2 makes it reproducible on my server. Going to try bisecting again!
Reporter | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=604fd3302562dc2059a8de3231818a1b618b8ffe&tochange=bf7793529f82ab9cb885a62446ed0f3e18c51634 Bug 1360406 regressed this.
Blocks: 1360406
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Keywords: regressionwindow-wanted → regression
Reporter | ||
Comment 26•7 years ago
|
||
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.
Assignee | ||
Comment 27•7 years ago
|
||
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
Assignee | ||
Comment 28•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 30•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 31•7 years ago
|
||
Thanks Felipe! Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d17802fd84705a61994c5d145915dfa7c62498e
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 32•7 years ago
|
||
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
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0f88980b79c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 34•7 years ago
|
||
Potential privacy leak - please request Beta approval on this when you get a chance.
Flags: needinfo?(moz-ian)
Assignee | ||
Comment 35•7 years ago
|
||
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+
Flags: qe-verify+
Comment 37•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/be62e8e756ba
Comment 38•7 years ago
|
||
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•7 years ago
|
||
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.
Updated•6 years ago
|
Flags: in-qa-testsuite?
Updated•6 years ago
|
Flags: in-qa-testsuite? → in-qa-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•