379239 - [cookies] implement countCookiesFromHost() in backend

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

mconnor recently mentioned the following UI code, which trawls through the entire local cookie list to determine if a given host has set any cookies:
http://lxr.mozilla.org/mozilla/source/browser/base/content/pageinfo/security.js#297

this is more efficiently done in the backend, with a simple hashtable lookup. we already have the code to do this, it just needs refactoring a little. patch forthcoming.

Comment 1

[[e-mail]]

Attachment: patch v1

something like this... includes backend & js (pageinfo ui) fix.

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 263243 [details] [diff] [review]
patch v1

General notes:

1) Tabs are evil, kill them all.
2) Comments are good, please be more liberal in their use    


+interface nsILoginInfo : nsISupports {
+    attribute AString hostname;
+    attribute AString formSubmitURL;
+    attribute AString httpRealm;
+
+    attribute AString username;
+    attribute AString usernameField;
+
+    attribute AString password;
+    attribute AString passwordField;
+
+    void init(in AString hostname, in AString formSubmitURL, in AString httpRealm,
+		in AString username, in AString password, in AString usernameField, in AString passwordField);
+    boolean equals(in nsILoginInfo that);
+    boolean equalsIgnorePassword(in nsILoginInfo that);
+};

all of these, and the interface in general, need documentations, doxygen-style (we actually have people using those!)

Index: netwerk/base/public/nsIPasswordManager.idl

+interface nsIURI;
+interface nsILoginInfo;
+interface nsIAutoCompleteResult;
+interface nsIDOMHTMLInputElement;
+
+[scriptable, uuid(cb9e0de8-3598-4ed7-857b-827f011ad5d8)]
+
+interface nsIPasswordManager : nsISupports {
+
+    void addLogin(in nsILoginInfo login);
+    void removeLogin(in nsILoginInfo login);
+    void modifyLogin(in nsILoginInfo oldLogin, in nsILoginInfo newLogin);
+
+    void getAllLogins(out unsigned long count, [retval, array, size_is(count)] out nsILoginInfo logins);
+    // Crap, can't use AString here... Error: [domstring], [utf8string], [cstring], [astring] types cannot be used in array parameters
+    void getAllDisabledLogins(out unsigned long count, [retval, array, size_is(count)] out wstring hostname);
+
+    void disableSavingLoginsFor(in AString host);
+    void enableSavingLoginsFor(in AString host);
+    boolean canSaveLoginsFor(in AString host);
+
+    void findLogins(out unsigned long count, in AString hostname, in AString actionURL, in AString httpRealm, [retval, array, size_is(count)] out nsILoginInfo logins);
+
+    // XXX Ugh. This interface is directly clled by FormFillController. This ought to be done some other way.
+    nsIAutoCompleteResult autoCompleteSearch(in AString aSearchString, in nsIAutoCompleteResult aPreviousResult, in nsIDOMHTMLInputElement aElement);

Pretty sure we should call this nsIToolkitPasswordManager since the API is totally different now.  Also needs docs for the API etc

Index: netwerk/base/public/nsIPasswordManagerStorage.idl

+[scriptable, uuid(e09e4ca6-276b-4bb4-8b71-0635a3a2a007)]
+
+interface nsIPasswordManagerStorage : nsISupports {
+    void init();
+    AString getAttribute(in AString propertyName);
+    void setAttribute(in AString propertyName, in AString propertyValue);
+
+    void addLogin(in nsILoginInfo login);
+    void removeLogin(in nsILoginInfo login);
+    void modifyLogin(in nsILoginInfo oldLogin, in nsILoginInfo newLogin);
+
+    void getAllLogins(out unsigned long count, [retval, array, size_is(count)] out nsILoginInfo logins); 
+    void getAllDisabledLogins(out unsigned long count, [retval, array, size_is(count)] out wstring hostname); 
+
+    void disableSavingLoginsFor(in AString hostname);
+    void enableSavingLoginsFor(in AString hostname);

why have two methods here, instead of a single method with a boolean?

Also, I hate naming like that, we don't tend to use it.

+    boolean canSaveLoginsFor(in AString hostname);
+
+    void findLogins(out unsigned long count, in AString hostname, in AString actionURL, in AString httpRealm, [retval, array, size_is(count)] out nsILoginInfo logins);

This feels awkward, why not just return an nsISimpleEnumerator of nsILoginInfos?


@@ -801,25 +801,27 @@ nsFtpState::R_pass() {
     }
     if (mResponseCode/100 == 5 || mResponseCode==421) {
         // There is no difference between a too-many-users error,
         // a wrong-password error, or any other sort of error
         // So we need to tell wallet to forget the password if we had one,
         // and error out. That will then show the error message, and the
         // user can retry if they want to
 
-        if (!mPassword.IsEmpty()) {
+        if (PR_FALSE && !mPassword.IsEmpty()) {
+            // XXX I think the original code here is wrong. A server with a
+            // temporary glitch shouldn't nuke our stored login.

file a followup here and leave the behaviour intact.

             nsCOMPtr<nsIPasswordManager> pm =
                     do_GetService(NS_PASSWORDMANAGER_CONTRACTID);
             if (pm) {
                 nsCAutoString prePath;
                 nsresult rv = mChannel->URI()->GetPrePath(prePath);
                 NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to get prepath");
-                if (NS_SUCCEEDED(rv))
-                    pm->RemoveUser(prePath, EmptyString());
+                //if (NS_SUCCEEDED(rv))
+                //    pm->RemoveUser(prePath, EmptyString());

delete, don't comment out, as a general rule.

nsPasswordManager.js next!

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 263243 [details] [diff] [review]
patch v1

netwerk/cookie/public/nsICookieManager2.idl
+   * @param aHost
+   *        the host string to look for

That's the raw host string, right? might make sense to explicitly say that here

(what about IDN, is this punycode-encoded?)

extensions/cookie/nsCookiePermission.cpp

your changes here have some strange indentation... also, style here has the { after the if on the same line


A unit test would be nice too :-)

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 263243 [details] [diff] [review]
patch v1

>Index: extensions/cookie/nsCookiePermission.cpp
>@@ -362,13 +362,19 @@ nsCookiePermission::CanSetCookie(nsIURI 
>       if (NS_SUCCEEDED(rv))
>-        rv = cookieManager->FindMatchingCookie(aCookie, &countFromHost, &foundCookie);
>+        rv = cookieManager->CookieExists(aCookie, &foundCookie);
>+      if (NS_SUCCEEDED(rv))

Tiny optimization: you only need to call thi snext block when foundCookie is true.
Also check the indenting in the next block.

r=mvl with that.

Comment 5

[[e-mail]]

Attachment: patch as checked in

Comment 6

[[e-mail]]

Attachment: unit tests for nsICookieManager{2} interfaces

i've checked in some unit tests to cover all the methods on nsICookieManager and nsICookieManager2, per this patch. these tests include coverage of the code added in this bug.

Comment 7

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/netwerk/test&command=DIFF_FRAMESET&file=TestCookie.cpp&rev1=1.8&rev2=1.9&root=/cvsroot