From a67d6f7873536ed1c94a4548c83e0d8f354327f3 Mon Sep 17 00:00:00 2001 From: Dan Witte Date: Tue, 19 Oct 2010 17:24:52 -0700 Subject: [PATCH] Bug 591447 - Cookie rowids may collide if PR_Now() winds backward. Part 2: add triple index. r=sdwilsh, a=betaN+ --- netwerk/cookie/nsCookie.cpp | 36 +++-- netwerk/cookie/nsCookie.h | 25 ++- netwerk/cookie/nsCookieService.cpp | 243 ++++++++++++++++++++--------- netwerk/cookie/nsCookieService.h | 2 +- netwerk/test/TestCookie.cpp | 5 +- 5 files changed, 201 insertions(+), 110 deletions(-) diff --git a/netwerk/cookie/nsCookie.cpp b/netwerk/cookie/nsCookie.cpp index 522b7b9e7735..7c30aa4737fc 100644 --- a/netwerk/cookie/nsCookie.cpp +++ b/netwerk/cookie/nsCookie.cpp @@ -75,26 +75,27 @@ StrBlockCopy(const nsACString &aSource1, * creation helper ******************************************************************************/ -// This is a counter that keeps track of the last used creation id, each time we -// create a new nsCookie. The creation id is nominally the time (in microseconds) -// the cookie was created. This id also corresponds to the row id used in the -// sqlite database, which must be unique. However, since it's possible two cookies -// may be created at the same time, or the system clock isn't monotonic, we must -// check each id to enforce monotonicity. -static PRInt64 gLastCreationID; +// This is a counter that keeps track of the last used creation time, each time +// we create a new nsCookie. This is nominally the time (in microseconds) the +// cookie was created, but is guaranteed to be monotonically increasing for +// cookies added at runtime after the database has been read in. This is +// necessary to enforce ordering among cookies whose creation times would +// otherwise overlap, since it's possible two cookies may be created at the same +// time, or that the system clock isn't monotonic. +static PRInt64 gLastCreationTime; PRInt64 -nsCookie::GenerateCreationID(PRInt64 aCreationTime) +nsCookie::GenerateUniqueCreationTime(PRInt64 aCreationTime) { // Check if the creation time given to us is greater than the running maximum // (it should always be monotonically increasing). - if (aCreationTime > gLastCreationID) { - gLastCreationID = aCreationTime; + if (aCreationTime > gLastCreationTime) { + gLastCreationTime = aCreationTime; return aCreationTime; } // Make up our own. - return ++gLastCreationID; + return ++gLastCreationTime; } nsCookie * @@ -104,7 +105,7 @@ nsCookie::Create(const nsACString &aName, const nsACString &aPath, PRInt64 aExpiry, PRInt64 aLastAccessed, - PRInt64 aCreationID, + PRInt64 aCreationTime, PRBool aIsSession, PRBool aIsSecure, PRBool aIsHttpOnly) @@ -125,13 +126,14 @@ nsCookie::Create(const nsACString &aName, StrBlockCopy(aName, aValue, aHost, aPath, name, value, host, path, end); - // If the creationID given to us is higher than the running maximum, update it. - if (aCreationID > gLastCreationID) - gLastCreationID = aCreationID; + // If the creationTime given to us is higher than the running maximum, update + // our maximum. + if (aCreationTime > gLastCreationTime) + gLastCreationTime = aCreationTime; // construct the cookie. placement new, oh yeah! return new (place) nsCookie(name, value, host, path, end, - aExpiry, aLastAccessed, aCreationID, + aExpiry, aLastAccessed, aCreationTime, aIsSession, aIsSecure, aIsHttpOnly); } @@ -153,7 +155,7 @@ NS_IMETHODIMP nsCookie::GetIsSecure(PRBool *aIsSecure) { *aIsSecure = IsSecu NS_IMETHODIMP nsCookie::GetIsHttpOnly(PRBool *aHttpOnly) { *aHttpOnly = IsHttpOnly(); return NS_OK; } NS_IMETHODIMP nsCookie::GetStatus(nsCookieStatus *aStatus) { *aStatus = 0; return NS_OK; } NS_IMETHODIMP nsCookie::GetPolicy(nsCookiePolicy *aPolicy) { *aPolicy = 0; return NS_OK; } -NS_IMETHODIMP nsCookie::GetCreationTime(PRInt64 *aCreation){ *aCreation = CreationID(); return NS_OK; } +NS_IMETHODIMP nsCookie::GetCreationTime(PRInt64 *aCreation){ *aCreation = CreationTime(); return NS_OK; } NS_IMETHODIMP nsCookie::GetLastAccessed(PRInt64 *aTime) { *aTime = LastAccessed(); return NS_OK; } // compatibility method, for use with the legacy nsICookie interface. diff --git a/netwerk/cookie/nsCookie.h b/netwerk/cookie/nsCookie.h index dde12e178def..2ce3adf50d55 100644 --- a/netwerk/cookie/nsCookie.h +++ b/netwerk/cookie/nsCookie.h @@ -71,7 +71,7 @@ class nsCookie : public nsICookie2 const char *aEnd, PRInt64 aExpiry, PRInt64 aLastAccessed, - PRInt64 aCreationID, + PRInt64 aCreationTime, PRBool aIsSession, PRBool aIsSecure, PRBool aIsHttpOnly) @@ -82,7 +82,7 @@ class nsCookie : public nsICookie2 , mEnd(aEnd) , mExpiry(aExpiry) , mLastAccessed(aLastAccessed) - , mCreationID(aCreationID) + , mCreationTime(aCreationTime) , mIsSession(aIsSession != PR_FALSE) , mIsSecure(aIsSecure != PR_FALSE) , mIsHttpOnly(aIsHttpOnly != PR_FALSE) @@ -90,10 +90,9 @@ class nsCookie : public nsICookie2 } public: - // Generate a unique creationID. This will usually be the same as the - // creation time, but with a guarantee of monotonicity such that it can - // be used as a sqlite rowid. - static PRInt64 GenerateCreationID(PRInt64 aCreationTime); + // Generate a unique and monotonically increasing creation time. See comment + // in nsCookie.cpp. + static PRInt64 GenerateUniqueCreationTime(PRInt64 aCreationTime); // public helper to create an nsCookie object. use |operator delete| // to destroy an object created by this method. @@ -103,7 +102,7 @@ class nsCookie : public nsICookie2 const nsACString &aPath, PRInt64 aExpiry, PRInt64 aLastAccessed, - PRInt64 aCreationID, + PRInt64 aCreationTime, PRBool aIsSession, PRBool aIsSecure, PRBool aIsHttpOnly); @@ -118,7 +117,7 @@ class nsCookie : public nsICookie2 inline const nsDependentCString Path() const { return nsDependentCString(mPath, mEnd); } inline PRInt64 Expiry() const { return mExpiry; } // in seconds inline PRInt64 LastAccessed() const { return mLastAccessed; } // in microseconds - inline PRInt64 CreationID() const { return mCreationID; } // in microseconds + inline PRInt64 CreationTime() const { return mCreationTime; } // in microseconds inline PRBool IsSession() const { return mIsSession; } inline PRBool IsDomain() const { return *mHost == '.'; } inline PRBool IsSecure() const { return mIsSecure; } @@ -128,9 +127,9 @@ class nsCookie : public nsICookie2 inline void SetExpiry(PRInt64 aExpiry) { mExpiry = aExpiry; } inline void SetLastAccessed(PRInt64 aTime) { mLastAccessed = aTime; } inline void SetIsSession(PRBool aIsSession) { mIsSession = (PRPackedBool) aIsSession; } - // set the creation id manually, overriding the monotonicity checks in Create(). - // use with caution! - inline void SetCreationID(PRInt64 aID) { mCreationID = aID; } + // Set the creation time manually, overriding the monotonicity checks in + // Create(). Use with caution! + inline void SetCreationTime(PRInt64 aTime) { mCreationTime = aTime; } protected: // member variables @@ -145,9 +144,7 @@ class nsCookie : public nsICookie2 const char *mEnd; PRInt64 mExpiry; PRInt64 mLastAccessed; - // creation id is unique for each cookie and approximately represents the cookie - // creation time, in microseconds. - PRInt64 mCreationID; + PRInt64 mCreationTime; PRPackedBool mIsSession; PRPackedBool mIsSecure; PRPackedBool mIsHttpOnly; diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp index f2f3cc1dcb38..20c0f8dfc417 100644 --- a/netwerk/cookie/nsCookieService.cpp +++ b/netwerk/cookie/nsCookieService.cpp @@ -101,7 +101,7 @@ static nsCookieService *gCookieService; static const char kHttpOnlyPrefix[] = "#HttpOnly_"; static const char kCookieFileName[] = "cookies.sqlite"; -#define COOKIES_SCHEMA_VERSION 3 +#define COOKIES_SCHEMA_VERSION 4 static const PRInt64 kCookieStaleThreshold = 60 * PR_USEC_PER_SEC; // 1 minute in microseconds static const PRInt64 kCookiePurgeAge = 30 * 24 * 60 * 60 * PR_USEC_PER_SEC; // 30 days in microseconds @@ -285,10 +285,9 @@ LogCookie(nsCookie *aCookie) PR_LOG(sCookieLog, PR_LOG_DEBUG, ("expires: %s%s", timeString, aCookie->IsSession() ? " (at end of session)" : "")); - PR_ExplodeTime(aCookie->CreationID(), PR_GMTParameters, &explodedTime); + PR_ExplodeTime(aCookie->CreationTime(), PR_GMTParameters, &explodedTime); PR_FormatTimeUSEnglish(timeString, 40, "%c GMT", &explodedTime); - PR_LOG(sCookieLog, PR_LOG_DEBUG, - ("created: %s (id %lld)", timeString, aCookie->CreationID())); + PR_LOG(sCookieLog, PR_LOG_DEBUG,("created: %s", timeString)); PR_LOG(sCookieLog, PR_LOG_DEBUG,("is secure: %s\n", aCookie->IsSecure() ? "true" : "false")); PR_LOG(sCookieLog, PR_LOG_DEBUG,("is httpOnly: %s\n", aCookie->IsHttpOnly() ? "true" : "false")); @@ -752,6 +751,9 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) rv = mDBState->dbConn->GetSchemaVersion(&dbSchemaVersion); NS_ENSURE_SUCCESS(rv, rv); + // Start a transaction for the whole migration block. + mozStorageTransaction transaction(mDBState->dbConn, PR_TRUE); + switch (dbSchemaVersion) { // upgrading. // every time you increment the database schema, you need to implement @@ -767,8 +769,6 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) case 2: { - mozStorageTransaction transaction(mDBState->dbConn, PR_TRUE); - // Add the baseDomain column and index to the table. rv = mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( "ALTER TABLE moz_cookies ADD baseDomain TEXT")); @@ -784,7 +784,7 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) nsCOMPtr update; rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( - "UPDATE moz_cookies SET baseDomain = ?1 WHERE id = ?2"), + "UPDATE moz_cookies SET baseDomain = :baseDomain WHERE id = :id"), getter_AddRefs(update)); NS_ENSURE_SUCCESS(rv, rv); @@ -806,9 +806,11 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) mozStorageStatementScoper scoper(update); - rv = update->BindUTF8StringByIndex(0, baseDomain); + rv = update->BindUTF8StringByName(NS_LITERAL_CSTRING("baseDomain"), + baseDomain); NS_ASSERT_SUCCESS(rv); - rv = update->BindInt64ByIndex(1, id); + rv = update->BindInt64ByName(NS_LITERAL_CSTRING("id"), + id); NS_ASSERT_SUCCESS(rv); rv = update->ExecuteStep(&hasResult); @@ -819,13 +821,59 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) rv = mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( "CREATE INDEX moz_basedomain ON moz_cookies (baseDomain)")); NS_ENSURE_SUCCESS(rv, rv); + } + // Fall through to the next upgrade. - // Update the schema version. - rv = mDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION); + case 3: + { + // Add the creationTime column to the table. + rv = mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "ALTER TABLE moz_cookies ADD creationTime INTEGER")); + NS_ENSURE_SUCCESS(rv, rv); + + // Read in the rowids from the database... + nsCOMPtr select; + rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT id FROM moz_cookies"), getter_AddRefs(select)); + NS_ENSURE_SUCCESS(rv, rv); + + // ... and use them to generate the creationTime. + nsCOMPtr update; + rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( + "UPDATE moz_cookies SET creationTime = :id WHERE id = :id"), + getter_AddRefs(update)); + NS_ENSURE_SUCCESS(rv, rv); + + PRBool hasResult; + while (1) { + rv = select->ExecuteStep(&hasResult); + NS_ENSURE_SUCCESS(rv, rv); + + if (!hasResult) + break; + + mozStorageStatementScoper scoper(update); + + PRInt64 id = select->AsInt64(0); + rv = update->BindInt64ByName(NS_LITERAL_CSTRING("id"), id); + NS_ASSERT_SUCCESS(rv); + + rv = update->ExecuteStep(&hasResult); + NS_ENSURE_SUCCESS(rv, rv); + } + + // Create a unique index on (name, host, path) to allow fast lookup. + rv = mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "CREATE UNIQUE INDEX moz_uniqueid " + "ON moz_cookies (name, host, path)")); NS_ENSURE_SUCCESS(rv, rv); } // Fall through to the next upgrade. + // No more upgrades. Update the schema version. + rv = mDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION); + NS_ENSURE_SUCCESS(rv, rv); + case COOKIES_SCHEMA_VERSION: break; @@ -863,6 +911,7 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) "path, " "expiry, " "lastAccessed, " + "creationTime, " "isSecure, " "isHttpOnly " "FROM moz_cookies"), getter_AddRefs(stmt)); @@ -886,7 +935,6 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) // cache frequently used statements (for insertion, deletion, and updating) rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( "INSERT INTO moz_cookies (" - "id, " "baseDomain, " "name, " "value, " @@ -894,19 +942,33 @@ nsCookieService::TryInitDB(PRBool aDeleteExistingDB) "path, " "expiry, " "lastAccessed, " + "creationTime, " "isSecure, " "isHttpOnly" - ") VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)"), + ") VALUES (" + ":baseDomain, " + ":name, " + ":value, " + ":host, " + ":path, " + ":expiry, " + ":lastAccessed, " + ":creationTime, " + ":isSecure, " + ":isHttpOnly" + ")"), getter_AddRefs(mDBState->stmtInsert)); NS_ENSURE_SUCCESS(rv, rv); rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( - "DELETE FROM moz_cookies WHERE id = ?1"), + "DELETE FROM moz_cookies " + "WHERE name = :name AND host = :host AND path = :path"), getter_AddRefs(mDBState->stmtDelete)); NS_ENSURE_SUCCESS(rv, rv); rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING( - "UPDATE moz_cookies SET lastAccessed = ?1 WHERE id = ?2"), + "UPDATE moz_cookies SET lastAccessed = :lastAccessed " + "WHERE name = :name AND host = :host AND path = :path"), getter_AddRefs(mDBState->stmtUpdate)); NS_ENSURE_SUCCESS(rv, rv); @@ -955,8 +1017,10 @@ nsCookieService::CreateTable() "path TEXT, " "expiry INTEGER, " "lastAccessed INTEGER, " + "creationTime INTEGER, " "isSecure INTEGER, " - "isHttpOnly INTEGER" + "isHttpOnly INTEGER, " + "CONSTRAINT moz_uniqueid UNIQUE (name, host, path)" ")")); if (NS_FAILED(rv)) return rv; @@ -1359,7 +1423,7 @@ nsCookieService::Add(const nsACString &aHost, nsCookie::Create(aName, aValue, host, aPath, aExpiry, currentTimeInUsec, - nsCookie::GenerateCreationID(currentTimeInUsec), + nsCookie::GenerateUniqueCreationTime(currentTimeInUsec), aIsSession, aIsSecure, aIsHttpOnly); @@ -1391,8 +1455,7 @@ nsCookieService::Remove(const nsACString &aHost, host, PromiseFlatCString(aName), PromiseFlatCString(aPath), - matchIter, - PR_Now() / PR_USEC_PER_SEC)) { + matchIter)) { nsRefPtr cookie = matchIter.Cookie(); RemoveCookieFromList(matchIter); NotifyChanged(cookie, NS_LITERAL_STRING("deleted").get()); @@ -1428,13 +1491,13 @@ nsCookieService::Read() nsCOMPtr stmt; nsresult rv = mDefaultDBState.dbConn->CreateStatement(NS_LITERAL_CSTRING( "SELECT " - "id, " "name, " "value, " "host, " "path, " "expiry, " "lastAccessed, " + "creationTime, " "isSecure, " "isHttpOnly, " "baseDomain " @@ -1459,29 +1522,27 @@ template nsCookie* nsCookieService::GetCookieFromRow(T &aRow) { // Skip reading 'baseDomain' -- up to the caller. - PRInt64 creationID = aRow->AsInt64(0); - nsCString name, value, host, path; - nsresult rv = aRow->GetUTF8String(1, name); + nsresult rv = aRow->GetUTF8String(0, name); NS_ASSERT_SUCCESS(rv); - rv = aRow->GetUTF8String(2, value); + rv = aRow->GetUTF8String(1, value); NS_ASSERT_SUCCESS(rv); - rv = aRow->GetUTF8String(3, host); + rv = aRow->GetUTF8String(2, host); NS_ASSERT_SUCCESS(rv); - rv = aRow->GetUTF8String(4, path); + rv = aRow->GetUTF8String(3, path); NS_ASSERT_SUCCESS(rv); - PRInt64 expiry = aRow->AsInt64(5); - PRInt64 lastAccessed = aRow->AsInt64(6); + PRInt64 expiry = aRow->AsInt64(4); + PRInt64 lastAccessed = aRow->AsInt64(5); + PRInt64 creationTime = aRow->AsInt64(6); PRBool isSecure = 0 != aRow->AsInt32(7); PRBool isHttpOnly = 0 != aRow->AsInt32(8); - // Create a new nsCookie and assign the data. We are guaranteed that the - // creationID is unique, since we're reading it from the db itself. + // Create a new nsCookie and assign the data. return nsCookie::Create(name, value, host, path, expiry, lastAccessed, - creationID, + creationTime, PR_FALSE, isSecure, isHttpOnly); @@ -1591,13 +1652,13 @@ nsCookieService::EnsureReadDomain(const nsCString &aBaseDomain) // Cache the statement, since it's likely to be used again. rv = mDefaultDBState.syncConn->CreateStatement(NS_LITERAL_CSTRING( "SELECT " - "id, " "name, " "value, " "host, " "path, " "expiry, " "lastAccessed, " + "creationTime, " "isSecure, " "isHttpOnly " "FROM moz_cookies " @@ -1659,13 +1720,13 @@ nsCookieService::EnsureReadComplete() nsCOMPtr stmt; nsresult rv = mDefaultDBState.syncConn->CreateStatement(NS_LITERAL_CSTRING( "SELECT " - "id, " "name, " "value, " "host, " "path, " "expiry, " "lastAccessed, " + "creationTime, " "isSecure, " "isHttpOnly, " "baseDomain " @@ -1819,9 +1880,8 @@ nsCookieService::ImportCookies(nsIFile *aCookieFile) if (NS_FAILED(rv)) continue; - // Create a new nsCookie and assign the data. - // We don't know the cookie creation time, so just use the current time - // to generate a unique creationID. + // Create a new nsCookie and assign the data. We don't know the cookie + // creation time, so just use the current time to generate a unique one. nsRefPtr newCookie = nsCookie::Create(Substring(buffer, nameIndex, cookieIndex - nameIndex - 1), Substring(buffer, cookieIndex, buffer.Length() - cookieIndex), @@ -1829,7 +1889,7 @@ nsCookieService::ImportCookies(nsIFile *aCookieFile) Substring(buffer, pathIndex, secureIndex - pathIndex - 1), expires, lastAccessedCounter, - nsCookie::GenerateCreationID(currentTimeInUsec), + nsCookie::GenerateUniqueCreationTime(currentTimeInUsec), PR_FALSE, Substring(buffer, secureIndex, expiresIndex - secureIndex - 1).EqualsLiteral(kTrue), isHttpOnly); @@ -1884,8 +1944,8 @@ class CompareCookiesForSending public: PRBool Equals(const nsCookie* aCookie1, const nsCookie* aCookie2) const { - // CreationID is unique, so two id's can never be equal. - return PR_FALSE; + return aCookie1->CreationTime() == aCookie2->CreationTime() && + aCookie2->Path().Length() == aCookie1->Path().Length(); } PRBool LessThan(const nsCookie* aCookie1, const nsCookie* aCookie2) const @@ -1899,8 +1959,7 @@ public: // required for backwards compatibility since some websites erroneously // depend on receiving cookies in the order in which they were sent to the // browser! see bug 236772. - // note: CreationID is unique, so two id's can never be equal. - return aCookie1->CreationID() < aCookie2->CreationID(); + return aCookie1->CreationTime() < aCookie2->CreationTime(); } }; @@ -2153,7 +2212,7 @@ nsCookieService::SetCookieInternal(nsIURI *aHostURI, cookieAttributes.path, cookieAttributes.expiryTime, currentTimeInUsec, - nsCookie::GenerateCreationID(currentTimeInUsec), + nsCookie::GenerateUniqueCreationTime(currentTimeInUsec), cookieAttributes.isSession, cookieAttributes.isSecure, cookieAttributes.isHttpOnly); @@ -2214,30 +2273,35 @@ nsCookieService::AddInternal(const nsCString &aBaseDomain, nsListIter matchIter; PRBool foundCookie = FindCookie(aBaseDomain, aCookie->Host(), - aCookie->Name(), aCookie->Path(), matchIter, currentTime); + aCookie->Name(), aCookie->Path(), matchIter); nsRefPtr oldCookie; if (foundCookie) { oldCookie = matchIter.Cookie(); - // if the old cookie is httponly, make sure we're not coming from script - if (!aFromHttp && oldCookie->IsHttpOnly()) { + // If the old cookie is httponly, make sure we're not coming from script -- + // but, if the old cookie has already expired, pretend like it didn't exist. + if (!aFromHttp && oldCookie->IsHttpOnly() && + oldCookie->Expiry() > currentTime) { COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie is httponly; coming from script"); return; } + // Remove the old cookie, regardless of whether it's expired or not, and + // notify. RemoveCookieFromList(matchIter); - // check if the cookie has expired - if (aCookie->Expiry() <= currentTime) { - COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie was deleted"); - NotifyChanged(oldCookie, NS_LITERAL_STRING("deleted").get()); - return; - } + COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie was deleted"); + NotifyChanged(oldCookie, NS_LITERAL_STRING("deleted").get()); - // preserve creation time of cookie + // If the new cookie has expired -- i.e. the intent was simply to delete + // the old cookie -- then we're done. + if (aCookie->Expiry() <= currentTime) + return; + + // Preserve creation time of cookie for ordering purposes. if (oldCookie) - aCookie->SetCreationID(oldCookie->CreationID()); + aCookie->SetCreationTime(oldCookie->CreationTime()); } else { // check if cookie has already expired @@ -2888,18 +2952,18 @@ class CompareCookiesByAge { public: PRBool Equals(const nsListIter &a, const nsListIter &b) const { - // CreationID is unique, so two id's can never be equal. - return PR_FALSE; + return a.Cookie()->LastAccessed() == b.Cookie()->LastAccessed() && + a.Cookie()->CreationTime() == b.Cookie()->CreationTime(); } PRBool LessThan(const nsListIter &a, const nsListIter &b) const { - // compare by LastAccessed time, and tiebreak by CreationID. + // compare by lastAccessed time, and tiebreak by creationTime. PRInt64 result = a.Cookie()->LastAccessed() - b.Cookie()->LastAccessed(); if (result != 0) return result < 0; - return a.Cookie()->CreationID() < b.Cookie()->CreationID(); + return a.Cookie()->CreationTime() < b.Cookie()->CreationTime(); } }; @@ -3066,8 +3130,7 @@ nsCookieService::CookieExists(nsICookie2 *aCookie, NS_ENSURE_SUCCESS(rv, rv); nsListIter iter; - *aFoundCookie = FindCookie(baseDomain, host, name, path, iter, - PR_Now() / PR_USEC_PER_SEC); + *aFoundCookie = FindCookie(baseDomain, host, name, path, iter); return NS_OK; } @@ -3166,8 +3229,7 @@ nsCookieService::FindCookie(const nsCString &aBaseDomain, const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, - nsListIter &aIter, - PRInt64 aCurrentTime) + nsListIter &aIter) { EnsureReadDomain(aBaseDomain); @@ -3179,8 +3241,7 @@ nsCookieService::FindCookie(const nsCString &aBaseDomain, for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) { nsCookie *cookie = cookies[i]; - if (cookie->Expiry() > aCurrentTime && - aHost.Equals(cookie->Host()) && + if (aHost.Equals(cookie->Host()) && aPath.Equals(cookie->Path()) && aName.Equals(cookie->Name())) { aIter = nsListIter(entry, i); @@ -3209,7 +3270,16 @@ nsCookieService::RemoveCookieFromList(const nsListIter &aIter, nsCOMPtr params; paramsArray->NewBindingParams(getter_AddRefs(params)); - nsresult rv = params->BindInt64ByIndex(0, aIter.Cookie()->CreationID()); + nsresult rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("name"), + aIter.Cookie()->Name()); + NS_ASSERT_SUCCESS(rv); + + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("host"), + aIter.Cookie()->Host()); + NS_ASSERT_SUCCESS(rv); + + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("path"), + aIter.Cookie()->Path()); NS_ASSERT_SUCCESS(rv); rv = paramsArray->AddParams(params); @@ -3255,34 +3325,44 @@ bindCookieParameters(mozIStorageBindingParamsArray *aParamsArray, NS_ASSERT_SUCCESS(rv); // Bind our values to params - rv = params->BindInt64ByIndex(0, aCookie->CreationID()); + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("baseDomain"), + aBaseDomain); NS_ASSERT_SUCCESS(rv); - rv = params->BindUTF8StringByIndex(1, aBaseDomain); + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("name"), + aCookie->Name()); NS_ASSERT_SUCCESS(rv); - rv = params->BindUTF8StringByIndex(2, aCookie->Name()); + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("value"), + aCookie->Value()); NS_ASSERT_SUCCESS(rv); - rv = params->BindUTF8StringByIndex(3, aCookie->Value()); + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("host"), + aCookie->Host()); NS_ASSERT_SUCCESS(rv); - rv = params->BindUTF8StringByIndex(4, aCookie->Host()); + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("path"), + aCookie->Path()); NS_ASSERT_SUCCESS(rv); - rv = params->BindUTF8StringByIndex(5, aCookie->Path()); + rv = params->BindInt64ByName(NS_LITERAL_CSTRING("expiry"), + aCookie->Expiry()); NS_ASSERT_SUCCESS(rv); - rv = params->BindInt64ByIndex(6, aCookie->Expiry()); + rv = params->BindInt64ByName(NS_LITERAL_CSTRING("lastAccessed"), + aCookie->LastAccessed()); NS_ASSERT_SUCCESS(rv); - rv = params->BindInt64ByIndex(7, aCookie->LastAccessed()); + rv = params->BindInt64ByName(NS_LITERAL_CSTRING("creationTime"), + aCookie->CreationTime()); NS_ASSERT_SUCCESS(rv); - rv = params->BindInt32ByIndex(8, aCookie->IsSecure()); + rv = params->BindInt32ByName(NS_LITERAL_CSTRING("isSecure"), + aCookie->IsSecure()); NS_ASSERT_SUCCESS(rv); - rv = params->BindInt32ByIndex(9, aCookie->IsHttpOnly()); + rv = params->BindInt32ByName(NS_LITERAL_CSTRING("isHttpOnly"), + aCookie->IsHttpOnly()); NS_ASSERT_SUCCESS(rv); // Bind the params to the array. @@ -3350,9 +3430,20 @@ nsCookieService::UpdateCookieInList(nsCookie *aCookie, aParamsArray->NewBindingParams(getter_AddRefs(params)); // Bind our parameters. - nsresult rv = params->BindInt64ByIndex(0, aLastAccessed); + nsresult rv = params->BindInt64ByName(NS_LITERAL_CSTRING("lastAccessed"), + aLastAccessed); NS_ASSERT_SUCCESS(rv); - rv = params->BindInt64ByIndex(1, aCookie->CreationID()); + + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("name"), + aCookie->Name()); + NS_ASSERT_SUCCESS(rv); + + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("host"), + aCookie->Host()); + NS_ASSERT_SUCCESS(rv); + + rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("path"), + aCookie->Path()); NS_ASSERT_SUCCESS(rv); // Add our bound parameters to the array. diff --git a/netwerk/cookie/nsCookieService.h b/netwerk/cookie/nsCookieService.h index f1f353abdc84..139ec327623a 100644 --- a/netwerk/cookie/nsCookieService.h +++ b/netwerk/cookie/nsCookieService.h @@ -247,7 +247,7 @@ class nsCookieService : public nsICookieService static PRBool GetExpiry(nsCookieAttributes &aCookie, PRInt64 aServerTime, PRInt64 aCurrentTime); void RemoveAllFromMemory(); void PurgeCookies(PRInt64 aCurrentTimeInUsec); - PRBool FindCookie(const nsCString& aBaseDomain, const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, nsListIter &aIter, PRInt64 aCurrentTime); + PRBool FindCookie(const nsCString& aBaseDomain, const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, nsListIter &aIter); PRUint32 CountCookiesFromHostInternal(const nsCString &aBaseDomain, nsEnumerationData &aData); void NotifyRejected(nsIURI *aHostURI); void NotifyChanged(nsISupports *aSubject, const PRUnichar *aData); diff --git a/netwerk/test/TestCookie.cpp b/netwerk/test/TestCookie.cpp index b3ad5c560dad..d9b70c75d3b8 100644 --- a/netwerk/test/TestCookie.cpp +++ b/netwerk/test/TestCookie.cpp @@ -738,10 +738,11 @@ main(PRInt32 argc, char *argv[]) rv[13] = NS_SUCCEEDED(cookieMgr2->CookieExists(newDomainCookie, &found)) && !found; // sleep four seconds, to make sure the second cookie has expired PR_Sleep(4 * PR_TicksPerSecond()); - // check CountCookiesFromHost() and CookieExists() don't count the expired cookie + // check CountCookiesFromHost() doesn't count the expired cookie... rv[14] = NS_SUCCEEDED(cookieMgr2->CountCookiesFromHost(NS_LITERAL_CSTRING("cookiemgr.test"), &hostCookies)) && hostCookies == 1; - rv[15] = NS_SUCCEEDED(cookieMgr2->CookieExists(expiredCookie, &found)) && !found; + // ... but that it's still there using CookieExists() + rv[15] = NS_SUCCEEDED(cookieMgr2->CookieExists(expiredCookie, &found)) && found; // double-check RemoveAll() using the enumerator rv[16] = NS_SUCCEEDED(cookieMgr->RemoveAll()); rv[17] = NS_SUCCEEDED(cookieMgr->GetEnumerator(getter_AddRefs(enumerator))) &&