Closed
Bug 769283
Opened 12 years ago
Closed 12 years ago
Support concurrent private/public auth caches
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: jdm, Assigned: jdm)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → chrislord.net
Comment 1•12 years ago
|
||
@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•12 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•12 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•12 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•12 years ago
|
Assignee: chrislord.net → josh
Assignee | ||
Comment 5•12 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 | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #664775 -
Attachment is obsolete: true
![]() |
||
Comment 7•12 years ago
|
||
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)
Comment 8•12 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 9•12 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]: ----------------------------------------------------------------- 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•12 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•12 years ago
|
Attachment #664776 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Whoops, the test passes this time.
Attachment #675270 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•12 years ago
|
Attachment #675258 -
Attachment is obsolete: true
Attachment #675258 -
Flags: review?(honzab.moz)
![]() |
||
Comment 14•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8e092cb079
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce8e092cb079
Status: NEW → RESOLVED
Closed: 12 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.
Description
•