Last Comment Bug 775119 - e10s: implement nsILoadContext in ParentChannels, remove PrivateBrowsingConsumer
: e10s: implement nsILoadContext in ParentChannels, remove PrivateBrowsingConsumer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
Depends on: 722845
Blocks: 756648
  Show dependency treegraph
 
Reported: 2012-07-18 08:41 PDT by Jason Duell [:jduell] (needinfo? me)
Modified: 2012-07-22 19:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
+


Attachments
make e10s version of test_cacheflags.js (1.93 KB, patch)
2012-07-18 08:51 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
Simple test of new private browsing API (3.42 KB, patch)
2012-07-18 08:53 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
convert HTTPChannelParent to provide nsILoadContext (17.21 KB, patch)
2012-07-18 08:56 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
a few more tweaks to HTTP code (14.17 KB, patch)
2012-07-19 18:59 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
Changes to FTP (14.77 KB, patch)
2012-07-19 19:01 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
Wyciwyg channel changes (16.14 KB, patch)
2012-07-19 19:02 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
Websockets changes (11.23 KB, patch)
2012-07-19 19:08 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review
Remove PrivateBrowsingConsumer (5.16 KB, patch)
2012-07-19 19:09 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review

Description Jason Duell [:jduell] (needinfo? me) 2012-07-18 08:41:39 PDT
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 [:jduell] (needinfo? me) 2012-07-18 08:51:48 PDT
Created attachment 643399 [details] [diff] [review]
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 [:jduell] (needinfo? me) 2012-07-18 08:53:50 PDT
Created attachment 643400 [details] [diff] [review]
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 [:jduell] (needinfo? me) 2012-07-18 08:56:49 PDT
Created attachment 643401 [details] [diff] [review]
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] 2012-07-18 08:59:56 PDT
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] 2012-07-18 09:04:37 PDT
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] 2012-07-18 09:07:21 PDT
Comment on attachment 643401 [details] [diff] [review]
convert HTTPChannelParent to provide nsILoadContext

This looks good to me.
Comment 7 Jason Duell [:jduell] (needinfo? me) 2012-07-19 18:59:46 PDT
Created attachment 644126 [details] [diff] [review]
a few more tweaks to HTTP code

Goes on top of previous HTTP patch
Comment 8 Jason Duell [:jduell] (needinfo? me) 2012-07-19 19:01:31 PDT
Created attachment 644127 [details] [diff] [review]
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 [:jduell] (needinfo? me) 2012-07-19 19:02:20 PDT
Created attachment 644128 [details] [diff] [review]
Wyciwyg channel changes
Comment 10 Jason Duell [:jduell] (needinfo? me) 2012-07-19 19:08:35 PDT
Created attachment 644133 [details] [diff] [review]
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 [:jduell] (needinfo? me) 2012-07-19 19:09:12 PDT
Created attachment 644135 [details] [diff] [review]
Remove PrivateBrowsingConsumer

This is the last of it.
Comment 12 Josh Matthews [:jdm] 2012-07-20 10:33:35 PDT
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] 2012-07-20 10:38:26 PDT
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] 2012-07-20 10:38:54 PDT
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] 2012-07-20 10:45:27 PDT
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 [:jduell] (needinfo? me) 2012-07-22 15:37:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d63b0129ba
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-07-22 19:04:18 PDT
https://hg.mozilla.org/mozilla-central/rev/84d63b0129ba

Note You need to log in before you can comment on or make changes to this bug.