Closed Bug 1199841 Opened 4 years ago Closed 4 years ago

Torrents downloaded in private mode from rarbg.to are added in the Firefox download history

Categories

(Firefox :: Private Browsing, defect)

x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
e10s m8+ ---
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- verified
firefox46 --- verified

People

(Reporter: marco, Assigned: jduell.mcbugs)

References

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
1) Open a normal window and a private window
2) In the private window, navigate to a rarbg.to page (for example, https://rarbg.to/torrent/lvtgcm4)
3) Download the torrent (select to save the file somewhere)

The download isn't added to the private window downloads list, but to the normal window one.
I.e. if you click on the downloads button in the toolbar of the private window, you don't see anything. If you switch back to the normal window you can see the file you've just downloaded.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
I've tried closing the private window and restarting Firefox, the download is still in the downloads history.
I can reproduce on Linux as well.
OS: Mac OS X → All
I can reproduce on Windows as well (in Nightly but not Aurora).
I can't reproduce if I disable e10s.
Blocks: e10s
Assignee: nobody → felipc
Status: NEW → ASSIGNED
tracking-e10s: --- → m8+
So the problem here is not exactly the same as in bug 1210617.  In that case, we are dealing with an interface that didn't implement nsIPrivateBrowsingChannel.  In this case, we have a nsHttpChannel, which inherits HttpChannelBase which inherits PrivateBrowsingChannel<T>.

It has its mPrivateBrowsing bit set correctly, but it doesn't have mPrivateBrowsingOverriden (as expected?), so NS_UsePrivateBrowsing doesn't return mPrivateBrowsing.

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp?rev=8694c40c0a2a#1216

It then goes on the next section, and here's where the behavior differs from e10s and non-e10s. In both cases we have a nsHttpChannel, but in the e10s case, it doesn't find any loadContext set as the notification callbacks, and therefore returns false.


Any suggestions on what to look for now? I imagine it's missing some SetNotificationCallbacks in the e10s path, but I don't know where it would be different in the e10s case
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(mrbkap)
After looking at this again, I wonder why it doesn't work. We set the notification callbacks on the channel in the parent here at <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp?rev=54a4387f2db9#459> and even deal with mPBOverride. Is that not the right channel? Or is there another channel that we need to do this on?
Flags: needinfo?(mrbkap)
Flags: needinfo?(jduell.mcbugs)
Attached patch fix-pbrowsing-download2 (obsolete) — Splinter Review
Ok, bear with me, here's what is going on:

The channel that we have get its notificationCallbacks correctly set by the code that Blake points out in comment 6. The problem is that, by the time that nsExternalHelperAppService runs <http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp?rev=54a4387f2db9#1388>, the channel has already closed, and its notification callback has been cleared by HttpBaseChannel::ReleaseListeners.

Just commenting out that part from ReleaseListeners (to see what happens) was not enough though, because HttpChannelParentListener.cpp also clears its reference to the listener, which makes the crucial GetInterface to nsILoadContext to not work here <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParentListener.cpp#133>.

So it seems that the notificationCallback is gone for good at that point. However, whenever it is set in the channel, the channel caches the value we're interested in mPrivateBrowsing, which we can use.

It also feels like the prudent thing for NS_UsePrivateBrowsing to do in the case it can't find a load context: try to use the value stored in the channel instead of defaulting to false.
Attachment #8682781 - Flags: review?(jduell.mcbugs)
Attachment #8682781 - Flags: feedback?(mrbkap)
Comment on attachment 8682781 [details] [diff] [review]
fix-pbrowsing-download2

Review of attachment 8682781 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/PrivateBrowsingChannel.h
@@ +56,5 @@
>    {
>        NS_ENSURE_ARG_POINTER(aValue);
>        NS_ENSURE_ARG_POINTER(aResult);
>        *aResult = mPrivateBrowsingOverriden;
> +      *aValue = mPrivateBrowsing;

It turns out that this works for your case, because mPrivateBrowsing happens to be true (we set it semi-randomly in HttpBaseChannel::SetupReplacementChannel(): it normally doesn't do anything to set it unless mPBOverridden is also set.

I think I can fix this more clearly and cleanly by restructuring the PBChannel code.  I'm going to give that a go.
Attachment #8682781 - Flags: review?(jduell.mcbugs)
Attachment #8682781 - Flags: feedback?(mrbkap)
Attachment #8682781 - Flags: feedback-
jdm:

So we need to be able to query post-OnStopRequest whether a channel was used for PB or not. Right now we fail because we drop the ref to the channel callbacks, so we have nothing to query and return PB=false.

I thought of adding some sort of "check at OnStopRequest and save the result" logic, but that seemed kludgy.  It actually seems much cleaner to make the PBChannel class itself the repository of PB info, and have its mPrivateBrowsing variable updated any time the loadContext info may have changed (i.e. whenever we update LoadGroup or notificationCallbacks).

I'm also assuming 1) once a channel has been marked for PB that should always stay true (no going back to non-private), per bug 775119 comment 13.
Assignee: felipc → jduell.mcbugs
Attachment #8690005 - Flags: review?(josh)
Comment on attachment 8690005 [details] [diff] [review]
v2: restructure PB code to remember status after callbacks go away

Review of attachment 8690005 [details] [diff] [review]:
-----------------------------------------------------------------

It took me a while to think through the interaction between manual overrides and and new changes, but I believe this should all be correct.
Attachment #8690005 - Flags: review?(josh) → review+
Jason, is this ready to land?
Flags: needinfo?(jduell.mcbugs)
Blassey--thanks for reminder, I'd forgotten about this.

One last try run looked good:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2a9696bb96b

So to inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/e47415eaddef
Flags: needinfo?(jduell.mcbugs)
https://hg.mozilla.org/mozilla-central/rev/e47415eaddef
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Jason, can you get this uplifted to 45 for our e10s beta experiment?
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [e10s-45-uplift]
Attachment #8682781 - Attachment is obsolete: true
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8690005 [details] [diff] [review]
v2: restructure PB code to remember status after callbacks go away

We want this for our e10s beta experiment---see comment 16

Approval Request Comment
[Feature/regressing bug #]:  long-standing
[User impact if declined]:  privacy leak
[Describe test coverage new/current, TreeHerder]: has been in nightly for several weeks.
[Risks and why]: low: fairly simple code change.
[String/UUID change made/needed]: none
Attachment #8690005 - Flags: approval-mozilla-aurora?
Comment on attachment 8690005 [details] [diff] [review]
v2: restructure PB code to remember status after callbacks go away

OK, let's take it.
Attachment #8690005 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
I was able to reproduce this issue on Firefox 45.0a2 (2016-01-18) and on Windows 10 x86.
The issue is no longer reproducible on Firefox 46.0a2 (2016-01-27) across platforms[1].
During tests, I've encountered a new issue: downloaded files from a Firefox window are not displayed in the private window downloads list.
Should I file a new bug for this issue?

[1]Windows 10x86, Mac OS X 10.11, Ubuntu 14.04 x86
Flags: needinfo?(jduell.mcbugs)
I don't actually know the answer to comment 20.  Marco, should a private window's downloads list contain downloads from non-private windows, or are they completely separate?
Flags: needinfo?(jduell.mcbugs) → needinfo?(mcastelluccio)
I don't know either. My feeling is that the lists should be completely separate, but I'm not sure.
Flags: needinfo?(mcastelluccio)
Any idea who else can provide information about the issue described in Comment 20?
from a PB window:
- the panel only shows things downloaded in PB mode
- about:downloads only shows things downloaded in PB mode
- the Library Downloads view shows everything

We don't mixup normal and private things in default views to avoid confusing users, so we only show PB downloads. The Library is a special case cause it's sort-of the "find everything" destination.

from a normal window:
- the panel only shows things downloaded in normal mode
- about:downloads only shows things downloaded in normal mode
- the Library Downloads view shows things downloaded in normal mode

The Library is trickier in the sense what is showing depends from which window you open it from.
Thanks Marco for the clarification.
I've also tested this issue on Firefox 45.0b2 and on Latest Nightly 47.0a1 (2016-02-03) across platforms and I confirm that this issue is not reproducible on the mentioned builds.
According to Comment 20 and the results from the latest tests, I am marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1456742
You need to log in before you can comment on or make changes to this bug.