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

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Marco Castelluccio [:marco]]

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.

Comment 1

[Marco Castelluccio [:marco]]

I've tried closing the private window and restarting Firefox, the download is still in the downloads history.

Comment 2

[Marco Castelluccio [:marco]]

I can reproduce on Linux as well.

Comment 3

[Marco Castelluccio [:marco]]

I can reproduce on Windows as well (in Nightly but not Aurora).

Comment 4

[Marco Castelluccio [:marco]]

I can't reproduce if I disable e10s.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: fix-pbrowsing-download2

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.

Comment 8

[Jason Duell]

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.

Comment 9

[Jason Duell]

Attachment: v2: restructure PB code to remember status after callbacks go away

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.

Comment 10

[Jason Duell]

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

Comment 11

[Josh Matthews [:jdm]]

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.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Jason, is this ready to land?

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e47415eaddef

Comment 14

[Jason Duell]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Jason, can you get this uplifted to 45 for our e10s beta experiment?

Comment 17

[Jason Duell]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8690005 [details] [diff] [review]
v2: restructure PB code to remember status after callbacks go away

OK, let's take it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3b2d6aea00c

Comment 20

[Mihai Boldan, Desktop QA [:mboldan]]

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

Comment 21

[Jason Duell]

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?

Comment 22

[Marco Castelluccio [:marco]]

I don't know either. My feeling is that the lists should be completely separate, but I'm not sure.

Comment 23

[Mihai Boldan, Desktop QA [:mboldan]]

Any idea who else can provide information about the issue described in Comment 20?

Comment 24

[Marco Bonardo [:mak]]

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.

Comment 25

[Mihai Boldan, Desktop QA [:mboldan]]

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.