Marco Castelluccio [:marco] Reporter
	Description • 9 years ago

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.

	Marco Castelluccio [:marco] Reporter
	Comment 1 • 9 years ago

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

	Marco Castelluccio [:marco] Reporter
	Comment 2 • 9 years ago

I can reproduce on Linux as well.

	Marco Castelluccio [:marco] Reporter
	Comment 3 • 9 years ago

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

	Marco Castelluccio [:marco] Reporter
	Comment 4 • 9 years ago

I can't reproduce if I disable e10s.

	:Felipe Gomes (needinfo for replies!)
	Comment 5 • 9 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 6 • 9 years ago

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?

	:Felipe Gomes (needinfo for replies!)
	Comment 7 • 9 years ago

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.

	Jason Duell Assignee
	Comment 8 • 9 years ago

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.

	Jason Duell Assignee
	Comment 9 • 9 years ago

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.

	Jason Duell Assignee
	Comment 10 • 9 years ago

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

	Josh Matthews [:jdm]
	Comment 11 • 9 years ago

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.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 12 • 9 years ago

Jason, is this ready to land?

	Pulsebot
	Comment 13 • 9 years ago

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

	Jason Duell Assignee
	Comment 14 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 8 years ago
bugherder

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

	Brad Lassey [:blassey] (use needinfo?)
	Comment 16 • 8 years ago

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

	Jason Duell Assignee
	Comment 17 • 8 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 18 • 8 years ago

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

OK, let's take it.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 8 years ago
bugherder uplift

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

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 20 • 8 years ago

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

	Jason Duell Assignee
	Comment 21 • 8 years ago

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?

	Marco Castelluccio [:marco] Reporter
	Comment 22 • 8 years ago

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

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 23 • 8 years ago

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

	Marco Bonardo [:mak]
	Comment 24 • 8 years ago

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.

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 25 • 8 years ago

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.