775119 - e10s: implement nsILoadContext in ParentChannels, remove PrivateBrowsingConsumer

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jason Duell]

This is needed for basecamp.

We kludged in passing usingPrivateBrowsing into necko channels via PrivateBrowsingConsumer in bug 722845.  We're going to be passing more information (notably appIDs and isMozBrowser) for Web Apps so we can make app-specific cookiejars, etc. and it makes more sense to just implement nsILoadContext in the parent (even though it has a few DOMWindow fields we won't be able to support), rather than more kludgery.

Comment 1

[Jason Duell]

Attachment: make e10s version of test_cacheflags.js

Only tweak needed was to switch to using nsICacheInfoChannel, which is the Child-friendly subset of nsICachingChannel.

Comment 2

[Jason Duell]

Attachment: Simple test of new private browsing API

jdm: do we have any other tests (mochi?) of the PB-nextgen?   If not I'm somewhat appalled :)   This adds a very simple test--it makes sure that a PB load followed by a non-PB one doesn't result in a cache hit.   Simple, but I've verified that it checks whether PB is working at least minimally.

Comment 3

[Jason Duell]

Attachment: convert HTTPChannelParent to provide nsILoadContext

Passes test_cacheflags.js.

I'm now working on FTP, Wyciwyg, and websockets (looks like nextgen-PB is currently broken for websockets--we don't propagate the PB info from Child to Parent).  Patches coming soon, but this patch shows the basic idea here.  So stop me now if there's a basic problem here.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 643400 [details] [diff] [review]
Simple test of new private browsing API

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

This is a good test addition, thanks.

Comment 5

[Josh Matthews [:jdm]]

So, I was pretty sure that there were tests, but those might simply be in other unfinished bugs that depend on the necko infrastructure (specifically, I was surprised to find only one use of nsIPrivateBrowsingConsumer in the tree).

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 643401 [details] [diff] [review]
convert HTTPChannelParent to provide nsILoadContext

This looks good to me.

Comment 7

[Jason Duell]

Attachment: a few more tweaks to HTTP code

Goes on top of previous HTTP patch

Comment 8

[Jason Duell]

Attachment: Changes to FTP

I tried to centralized some of the code here (have a common class that all the ChannelParents could inherit from to get nsILoadChannel spoofing) but ran into some funky XPCOM multiple inheritance issues, so punted.

Comment 9

[Jason Duell]

Attachment: Wyciwyg channel changes

Comment 10

[Jason Duell]

Attachment: Websockets changes

So we didn't have nextgen-PB working with websockets.  The Child doesn't open an HTTPChannel, we do it on the parent, so we need to make sure the WebSocketChannelParent spoofs nsILoadContext (GetInterace call chain is nsHttpChannel->WebSocketChannel->WebSocketChannelParent).

Comment 11

[Jason Duell]

Attachment: Remove PrivateBrowsingConsumer

This is the last of it.

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 644126 [details] [diff] [review]
a few more tweaks to HTTP code

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

I like this, except for the caching.

::: netwerk/base/public/nsNetUtil.h
@@ +1299,5 @@
>  
>  /**
> + * Returns true if channel is using Private Browsing, or false if not.
> + *
> + * Note: you may get a false negative if you call this before AsyncOpen has been

Is this something we might want to warn/assert about?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +273,5 @@
>    mCallbacks = aCallbacks;
>    mProgressSink = nsnull;
> +
> +  // Will never change unless SetNotificationCallbacks called again, so cache
> +  mPrivateBrowsing = NS_UsePrivateBrowsing(this);

This is not necessarily true. There's nothing to stop an addon creating a menu item to toggle whether a tab is private or not, so I'm not really comfortable caching this value.

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 644127 [details] [diff] [review]
Changes to FTP

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

Having read this following my concerns about caching in the previous patch, it occurs to me that we do not currently support changing the privacy status of a child load context in a cross-process environment. That suggests to me that we should just not bother supporting dynamic privacy changes after a channel has been opened in general.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 644126 [details] [diff] [review]
a few more tweaks to HTTP code

r=me based on my thoughts from the last comment.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 644135 [details] [diff] [review]
Remove PrivateBrowsingConsumer

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

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +2183,4 @@
>      NS_ASSERTION(mRequest, "This should never be called with a null request");
> +    nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
> +    if (channel) {
> +      inPrivateBrowsing = NS_UsePrivateBrowsing(channel);

I'm partial to replacing this block with |bool inPrivateBrowsing = channel && NS_UsePrivateBrowsing(channel)|, personally.

Comment 16

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d63b0129ba

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84d63b0129ba