Closed Bug 769283 Opened 8 years ago Closed 8 years ago

Support concurrent private/public auth caches

Categories

(Core :: Networking: HTTP, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → chrislord.net
@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.
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.
Depends on: 756717
(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...
(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...
Assignee: chrislord.net → josh
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?
Attachment #664775 - Attachment is obsolete: true
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.
Attachment #664776 - Flags: feedback?(honzab.moz)
Blocks: mobilepb
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 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.
Attachment #664776 - Flags: feedback?(honzab.moz) → feedback+
I didn't take the ClearAll suggestion since it's actually not called by anybody.
Attachment #675258 - Flags: review?(honzab.moz)
Attachment #664776 - Attachment is obsolete: true
Whoops, the test passes this time.
Attachment #675270 - Flags: review?(honzab.moz)
Attachment #675258 - Attachment is obsolete: true
Attachment #675258 - Flags: review?(honzab.moz)
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 !
Attachment #675270 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/ce8e092cb079
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.