769283 - Support concurrent private/public auth caches

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Josh Matthews [:jdm]]

The PB service clears the auth cache on the private-browsing notification. We need to support multiple auth caches instead, and clear the private one on last-pb-context-exited.

Comment 1

[Saurabh Anand [:sawrubh]]

@Ehsan,Josh I am approaching the end of my assigned PBnGen bugs and I would like more. Since all remaining PBnGen bugs are assigned to Chris, can I take up a few of his bugs and help him.

I would like to work on this one, if it's ok.

Thanks.

Comment 2

[Jason Duell]

We're about to make separate appcaches possible for B2G (see bug 756717).  After that bug lands this one can use the infrastructure to support this bug.

Comment 3

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #2)
> We're about to make separate appcaches possible for B2G (see bug 756717). 
> After that bug lands this one can use the infrastructure to support this bug.

How close is that?  There doesn't seem to be a lot of activity on that bug right now...

Comment 4

[(no longer active)]

(In reply to Saurabh Anand [:sawrubh] from comment #1)
> @Ehsan,Josh I am approaching the end of my assigned PBnGen bugs and I would
> like more. Since all remaining PBnGen bugs are assigned to Chris, can I take
> up a few of his bugs and help him.
> 
> I would like to work on this one, if it's ok.

Let's leave this one for now as it depends on bug 756717 making its way into the tree...

Comment 5

[Josh Matthews [:jdm]]

Attachment: Add a separate HTTP auth cache for private channels.

Here's a rough simultaneous private/public authcache implementation. This obviously doesn't tie in with your ideas about the appcache; Jason, could you elaborate on the overlap you imagined here?

Comment 6

[Josh Matthews [:jdm]]

Attachment: Add a separate HTTP auth cache for private channels.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 664776 [details] [diff] [review]
Add a separate HTTP auth cache for private channels.

Adding f? flag to my self to don't forget to check on this patch.

Comment 8

[Jason Duell]

Comment on attachment 664776 [details] [diff] [review]
Add a separate HTTP auth cache for private channels.

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

jdm: ignore what I said in comment 2.  This patch seems ok to me, from a quick lookover.  But I don't have much dept in the auth code: honza is prob best reviewer here.

::: netwerk/protocol/http/nsHttpAuthManager.cpp
@@ +118,2 @@
>  {
> +  return (aIsPrivate ? mPrivateAuthCache : mAuthCache)->ClearAll();

Given that the call sites for !isPrivate (currently "profile-change-net-teardown" and "net:clear-active-logins" events) want to always clear both caches, it might make sense to make this arg "aPrivateOnly" and clear both if false.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 664776 [details] [diff] [review]
Add a separate HTTP auth cache for private channels.

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

f+, I'd say it's also ready for r.

::: browser/components/privatebrowsing/test/unit/test_httpauth.js
@@ +61,5 @@
>      // make sure the added auth entry is no longer accessible
>      domain = {value: kEmpty}, user = {value: kEmpty}, pass = {value: kEmpty};
>      try {
>        // should throw
> +      am.getAuthIdentity(kHTTP, kHost2, kPort, kBasic, kRealm, kEmpty, domain, user, pass, false);

I don't get just from looking at the test why you pass false here?  Didn't you want to go with true?

::: netwerk/protocol/http/nsHttpAuthManager.cpp
@@ +118,2 @@
>  {
> +  return (aIsPrivate ? mPrivateAuthCache : mAuthCache)->ClearAll();

Agree with jason's comment here.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Add a separate HTTP auth cache for private channels.

I didn't take the ClearAll suggestion since it's actually not called by anybody.

Comment 11

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=06aab303bd35

Comment 12

[Josh Matthews [:jdm]]

Attachment: Add a separate HTTP auth cache for private channels.

Whoops, the test passes this time.

Comment 13

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=957b461ce444

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 675270 [details] [diff] [review]
Add a separate HTTP auth cache for private channels.

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

r=honzab but please address all review comments before landing.

::: browser/components/privatebrowsing/test/unit/test_httpauth.js
@@ +23,5 @@
>    const kPassword = "pass";
>    const kPassword2 = "pass2";
>    const kEmpty = "";
> +  
> +  let isPrivate = false;

Please, rather define consts like PRIVATE = true and NOT_PRIVATE = false and use them in the test.  It is hard to track where you are changing isPrivate value, if I follow the test correctly.

White spaces.

@@ +58,5 @@
>      do_check_eq(domain.value, kDomain);
>      do_check_eq(user.value, kUser2);
>      do_check_eq(pass.value, kPassword2);
> +
> +    try {      

White spaces.

::: netwerk/protocol/http/nsIHttpAuthManager.idl
@@ -92,5 @@
> -
> -    /**
> -     * Clear all auth cache.
> -     */
> -    void clearAll();

Change IID !

Comment 15

[Josh Matthews [:jdm]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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