Support concurrent private/public auth caches

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
mozilla19
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

7 years ago
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.

Updated

7 years ago
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.

Comment 2

7 years ago
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

Comment 3

7 years ago
(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

7 years ago
(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

Updated

7 years ago
Assignee: chrislord.net → josh
Assignee

Comment 5

7 years ago
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?
Assignee

Updated

7 years ago
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)
Assignee

Updated

7 years ago
Blocks: mobilepb

Comment 8

7 years ago
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+
Assignee

Comment 10

7 years ago
I didn't take the ClearAll suggestion since it's actually not called by anybody.
Attachment #675258 - Flags: review?(honzab.moz)
Assignee

Updated

7 years ago
Attachment #664776 - Attachment is obsolete: true
Assignee

Comment 12

7 years ago
Whoops, the test passes this time.
Attachment #675270 - Flags: review?(honzab.moz)
Assignee

Updated

7 years ago
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: 7 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.