536978 - Cookies should write asynchronously

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - Insert new cookies async v1.0

The cookie service should write asynchronously instead of synchronously like it currently does.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - Update cookies async v1.0

Comment 2

[[e-mail]]

Neat. Is this ready for review?

Comment 3

[Shawn Wilsher :sdwilsh]

The first two parts could probably be reviewed.  There's still one more query that I need to write to do asynch.  It looked to be the hardest, so I haven't done it yet.  Probably on my plane ride next week!

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - Insert new cookies async v1.1

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - Update cookies async v1.1

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - delete cookies async v1.0

Comment 7

[[e-mail]]

Comment on attachment 427435 [details] [diff] [review]
Part 1 - Insert new cookies async v1.1

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

>+namespace {

Why the anonymous namespace?

>+  NS_IMETHOD HandleError(mozIStorageError* aError)
>+  {

Since you've got basically the same implementation in all three patches, factor the common bits into a function? See e.g. LogEvicted(); something like LogStorageError(mozIStorageError* aError, const char* aOpType).

>+#if defined(PR_LOGGING) || defined(DEBUG)
>+    PRInt32 result;
>+    nsresult rv = aError->GetResult(&result);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCAutoString message;
>+    rv = aError->GetMessage(message);
>+    NS_ENSURE_SUCCESS(rv, rv);

These functions can't fail, and even if they did, returning early without logging it seems bad. I'd say ditch the early returns and init 'result' to -1 so if GetResult does fail, we see a crazy number in the log.

>+    nsCAutoString warnMsg;
>+    warnMsg.Append("An error occured while inserting a cookie into the database: ");
>+    warnMsg.Append(result);

I think you want AppendInt(); Append() will cast 'result' to a char.

>+    warnMsg.Append(" ");
>+    warnMsg.Append(message);
>+    NS_WARNING(warnMsg.get());
>+
>+    COOKIE_LOGSTRING(PR_LOG_WARNING, (warnMsg.get()));

Using a format string here instead would be nice.

COOKIE_LOGSTRING(PR_LOG_WARNING, ("Error %d occurred %s a cookie: %s\n", result, aOpType, message.get()));

>@@ -670,6 +719,7 @@ nsCookieService::CloseDB()
> 
>   // finalize our statements and close the db connection for the default state.
>   // since we own these objects, nulling the pointers is sufficient here.
>+  // XXX using async statements means we may have a problem with just nulling it out
>   mDefaultDBState.stmtInsert = nsnull;
>   mDefaultDBState.stmtDelete = nsnull;
>   mDefaultDBState.stmtUpdate = nsnull;

So will we get storage assertions on shutdown? Not sure what to say about this, I'll leave it to you to figure out :)

> static nsresult
> bindCookieParameters(mozIStorageStatement *aStmt, const nsCookie *aCookie)
> {
>+  NS_ASSERTION(aStmt, "Null statement passed to bindCookieParameters!");
>+  NS_ASSERTION(aCookie, "Null cookie passed to bindCookieParameters!");
>   nsresult rv;
> 
>-  rv = aStmt->BindInt64Parameter(0, aCookie->CreationID());
>+  // Using the asynchronous binding methods to ensure that we do not acquire the
>+  // database lock.

s/Using/Use/

>+  nsCOMPtr<mozIStorageBindingParamsArray> arr;
>+  rv = aStmt->NewBindingParamsArray(getter_AddRefs(arr));

So, I think we want to hoist this up a few levels. Put one in SetCookieStringInternal() and one in ImportCookies(), which process multiple cookies at once, and propagate it through SetCookieInternal, AddInternal, AddCookieToList, to here. If that param is null, then create the array here. Then pass null in the other callers of AddInternal and AddCookieToList.

You can do this as a part 4 if you want.

There are also transaction scopers in those functions, and probably elsewhere, which need to be nuked?

>@@ -2707,13 +2774,11 @@ nsCookieService::AddCookieToList(const n
> 
>   // if it's a non-session cookie and hasn't just been read from the db, write it out.
>   if (aWriteToDB && !aCookie->IsSession() && mDBState->dbConn) {
>-    // use our cached sqlite "insert" statement
>-    mozStorageStatementScoper scoper(mDBState->stmtInsert);
>-
>     nsresult rv = bindCookieParameters(mDBState->stmtInsert, aCookie);
>     if (NS_SUCCEEDED(rv)) {
>-      PRBool hasResult;
>-      rv = mDBState->stmtInsert->ExecuteStep(&hasResult);
>+      nsCOMPtr<mozIStoragePendingStatement> handle;
>+      rv = mDBState->stmtInsert->ExecuteAsync(&sInsertCookieDBListener,
>+                                              getter_AddRefs(handle));
>     }
> 
>     if (NS_FAILED(rv)) {

Just below this there's a logging string "AddCookieToList(): adding to db gave error %x". Can you change "adding to db" to something more representative?

Looks good, but want to see another patch.

Comment 8

[[e-mail]]

Comment on attachment 427436 [details] [diff] [review]
Part 2 - Update cookies async v1.1

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

>@@ -1497,15 +1544,44 @@ nsCookieService::GetCookieInternal(nsIUR

>   if (stale) {
>-    // start a transaction on the storage db, to optimize updates.
>-    // transaction will automically commit on completion.
>-    mozStorageTransaction transaction(mDBState->dbConn, PR_TRUE);
>+    // Create an array of parameters to bind to our update statement.
>+    nsCOMPtr<mozIStorageBindingParamsArray> arr;
>+    mozIStorageStatement* stmt = mDBState->stmtUpdate;
>+    nsresult rv = stmt->NewBindingParamsArray(getter_AddRefs(arr));

Check rv here...

>-    for (PRInt32 i = 0; i < count; ++i) {
>+    bool updateNeeded = false;

... delete updateNeeded, since we already know 'stale' is true ...

>+    for (PRInt32 i = 0; i < count && NS_SUCCEEDED(rv); ++i) {

.. delete NS_SUCCEEDED() here ...

>       cookie = foundCookieList.ElementAt(i);
> 
>-      if (currentTimeInUsec - cookie->LastAccessed() > kCookieStaleThreshold)
>-        UpdateCookieInList(cookie, currentTimeInUsec);
>+      if (currentTimeInUsec - cookie->LastAccessed() > kCookieStaleThreshold) {
>+        // Create our params holder.
>+        nsCOMPtr<mozIStorageBindingParams> params;
>+        nsresult rv = arr->NewBindingParams(getter_AddRefs(params));
>+        if (NS_FAILED(rv)) continue;
>+
>+        // Bind our parameters.
>+        rv = params->BindInt64ByIndex(0, currentTimeInUsec);
>+        if (NS_FAILED(rv)) continue;
>+        rv = params->BindInt64ByIndex(1, cookie->CreationID());
>+        if (NS_FAILED(rv)) continue;
>+
>+        // Add our bound parameters to the array.
>+        rv = arr->AddParams(params);
>+        if (NS_FAILED(rv)) continue;
>+        updateNeeded = true;
>+      }

... and 'return' instead of 'continue' on failure. For "hard" failures like OOM, it's fine to just fail.

>+    // If we had an update, update the database now.
>+    if (updateNeeded) {
>+      rv = stmt->BindParameters(arr);
>+      if (NS_SUCCEEDED(rv)) {
>+        nsCOMPtr<mozIStoragePendingStatement> handle;
>+        rv = stmt->ExecuteAsync(&sUpdateCookieDBListener, getter_AddRefs(handle));
>+        if (NS_FAILED(rv)) {
>+          NS_WARNING("db update failed!");
>+          COOKIE_LOGSTRING(PR_LOG_WARNING, ("UpdateCookieInList(): updating db gave error %x", rv));
>+        }
>+      }

All this stuff should probably be factored back in to an UpdateCookieInList() function, not least to save on stack variable destructors on early return and such. You can create the array here, and pass it into UpdateCookieInList. Also, could change the log message per comment in last patch.

Should probably make BindParameters and ExecuteAsync fail hard here. (Their failure mode is basically OOM, right?)

Also, this will all crash in PB or if mDBState->stmtUpdate is otherwise null.

Also, I think you lost the hashtable update bits that UpdateCookieInList() used to do. And the IsSession check.

Comment 9

[[e-mail]]

Comment on attachment 427438 [details] [diff] [review]
Part 3 - delete cookies async v1.0

>@@ -2557,8 +2613,16 @@ nsCookieService::PurgeCookies(PRInt64 aC
>   if (!removedList)
>     return;
> 
>+  mozIStorageStatement *stmt = mDBState->stmtDelete;
>+  mozStorageStatementScoper scoper(stmt);
>+  nsCOMPtr<mozIStorageBindingParamsArray> paramsArray;
>+  nsresult rv = stmt->NewBindingParamsArray(getter_AddRefs(paramsArray));
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }

Single-line 'if (NS_FAILED(rv)) return;' here. This also crashes in PB -- need to wrap things appropriately.

>@@ -2587,7 +2651,17 @@ nsCookieService::PurgeCookies(PRInt64 aC
>+  rv = stmt->BindParameters(paramsArray);
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }
>+  nsCOMPtr<mozIStoragePendingStatement> handle;
>+  rv = stmt->ExecuteAsync(&sRemoveCookieDBListener, getter_AddRefs(handle));
>+  if (NS_FAILED(rv)) {
>+    return;
>   }

Single lines here too.

>@@ -2751,21 +2825,68 @@ nsCookieService::RemoveCookieFromList(co
>   // if it's a non-session cookie, remove it from the db
>   if (!aIter.Cookie()->IsSession() && mDBState->dbConn) {
>     // use our cached sqlite "delete" statement
>-    mozStorageStatementScoper scoper(mDBState->stmtDelete);
>+    mozIStorageStatement *stmt = mDBState->stmtDelete;
>+    mozStorageStatementScoper scoper(stmt);

What's the purpose of scopers now, btw?

>-    PRInt64 creationID = aIter.Cookie()->CreationID();
>-    nsresult rv = mDBState->stmtDelete->BindInt64Parameter(0, creationID);
>+    // Using the asynchronous binding methods to ensure that we do not acquire
>+    // the database lock.
>+    // XXX how much logging to we want here on failure?

If they're all fail-hard, then just an NS_WARNING is OK. If that's the case, you can unwind all the nested NS_SUCCEEDED(...) bits too, and just use early returns.

>+  // XXX should this get roped into its own function?  It is used twice, but I
>+  // cannot think of a good name for it.

Nah, not worth it.

>+void
>+nsCookieService::RemoveCookieFromListBatched(const nsListIter &aIter,
>+                                             mozIStorageBindingParamsArray *aParamsArr)
>+{
>+  NS_ASSERTION(aParamsArr, "Must pass non-null array of parameters");
>+
>+  // Ff it is a non-session cookie, remove it from the database.

'If'.

>+  if (!aIter.Cookie()->IsSession() && mDBState->dbConn) {
>+    // Append this cookie to our array of parameters.
>+    nsCOMPtr<mozIStorageBindingParams> params;
>+    nsresult rv = aParamsArr->NewBindingParams(getter_AddRefs(params));
>+    // XXX how much logging to we want here on failure?

Same as above - check rv's and return early; NS_WARNING on failure.

I think you could have RemoveCookieFromList just create the params array, call RemoveCookieFromListBatched, and execute, but I don't mind either way.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Why the anonymous namespace?
Just habit from other places.  If it's not exposed to the outside world, make it anonymous so they can't get to you in a hacky way either.

> So will we get storage assertions on shutdown? Not sure what to say about this,
> I'll leave it to you to figure out :)
Do not have to do anything magical for this.  The code as it was works fine.

> So, I think we want to hoist this up a few levels. Put one in
> SetCookieStringInternal() and one in ImportCookies(), which process multiple
> cookies at once, and propagate it through SetCookieInternal, AddInternal,
> AddCookieToList, to here. If that param is null, then create the array here.
> Then pass null in the other callers of AddInternal and AddCookieToList.
Wow this turned into a lot of work, but addressed.

> Just below this there's a logging string "AddCookieToList(): adding to db gave
> error %x". Can you change "adding to db" to something more representative?
Fixed.

New patch RSN!  (passes unit tests!)

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - Insert new cookies async v1.2

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Check rv here...
This should only fail on memory allocation failure, which we don't need to worry about anymore, so...

> ... and 'return' instead of 'continue' on failure. For "hard" failures like
> OOM, it's fine to just fail.
returning now, but we don't have to worry about OOM on mozilla-central.

> All this stuff should probably be factored back in to an UpdateCookieInList()
> function, not least to save on stack variable destructors on early return and
> such. You can create the array here, and pass it into UpdateCookieInList. Also,
> could change the log message per comment in last patch.
Done.  Don't know what I was thinking before...

> Should probably make BindParameters and ExecuteAsync fail hard here. (Their
> failure mode is basically OOM, right?)
ExecuteAsync can have a few other possible ones (like API misuse), but BindParameters should be OK.  Will hard fail here.

> Also, this will all crash in PB or if mDBState->stmtUpdate is otherwise null.
> 
> Also, I think you lost the hashtable update bits that UpdateCookieInList() used
> to do. And the IsSession check.
Fixed.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - Update cookies async v1.1

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Single-line 'if (NS_FAILED(rv)) return;' here. This also crashes in PB -- need
> to wrap things appropriately.
Fixed.

> Single lines here too.
Fixed.

> What's the purpose of scopers now, btw?
Nothing anymore.  Removed now.

> I think you could have RemoveCookieFromList just create the params array, call
> RemoveCookieFromListBatched, and execute, but I don't mind either way.
I ended up making this more like previous patch iterations.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - delete cookies async v1.1

Comment 16

[[e-mail]]

Comment on attachment 431709 [details] [diff] [review]
Part 1 - Insert new cookies async v1.2

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

>+class InsertCookieDBListener : public DBListenerErrorHandler
>+{
>+protected:
>+  virtual const char *GetOpType() { return "INSERT"; }
>+
>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  NS_IMETHOD HandleResult(mozIStorageResultSet*)
>+  {
>+    NS_NOTREACHED("Unexpected call to InsertCookieDBListener::HandleResult");
>+    return NS_OK;
>+  }
>+  NS_IMETHOD HandleCompletion(PRUint16 aReason)
>+  {
>+    return NS_OK;
>+  }
>+};
>+NS_IMETHODIMP_(nsrefcnt) InsertCookieDBListener::AddRef() { return 2; }
>+NS_IMETHODIMP_(nsrefcnt) InsertCookieDBListener::Release() { return 1; }
>+NS_IMPL_QUERY_INTERFACE1(InsertCookieDBListener, mozIStorageStatementCallback)

Hmm. Howcome all five of these virtual implementations can't live in DBListenerErrorHandler itself? (I might be missing something here, but it seems like all InsertCookieDBListener needs to do is override GetOpType(), and nothing else.)

>@@ -842,32 +899,44 @@ nsCookieService::SetCookieStringInternal

>+  // We may be adding a bunch of cookies to the DB, so we use async batching
>+  // with storage to make this super fast.

Nit: needs more 'super'. :)

r=dwitte with virtual question resolved.

Comment 17

[[e-mail]]

Comment on attachment 431734 [details] [diff] [review]
Part 2 - Update cookies async v1.1

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

> void
>-nsCookieService::UpdateCookieInList(nsCookie *aCookie, PRInt64 aLastAccessed)
>+nsCookieService::UpdateCookieInList(nsCookie                      *aCookie,
>+                                    PRInt64                        aLastAccessed,
>+                                    mozIStorageBindingParamsArray *aParamsArray)
> {
>-  // update the lastAccessed timestamp
>+  NS_ASSERTION(aCookie, "Passing a null cookie to UpdateCookieInList!");
>+
>+  // udpate the lastAccessed timestamp

'update'.

r=dwitte

Comment 18

[[e-mail]]

Comment on attachment 431734 [details] [diff] [review]
Part 2 - Update cookies async v1.1

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

>@@ -1523,33 +1552,46 @@ nsCookieService::GetCookieInternal(nsIUR

>+    // Update the database now if we had a parameters array.
>+    if (paramsArray) {
>+      nsresult rv = stmt->BindParameters(paramsArray);
>+      nsCOMPtr<mozIStoragePendingStatement> handle;
>+      rv = stmt->ExecuteAsync(&sUpdateCookieDBListener, getter_AddRefs(handle));

Actually, nit: you're not doing anything with the first 'rv' here. Assert or ditch it? (Probably just assert, right?)

>-nsCookieService::UpdateCookieInList(nsCookie *aCookie, PRInt64 aLastAccessed)
>+nsCookieService::UpdateCookieInList(nsCookie                      *aCookie,
>+                                    PRInt64                        aLastAccessed,
>+                                    mozIStorageBindingParamsArray *aParamsArray)
> {

>-    nsresult rv = mDBState->stmtUpdate->BindInt64Parameter(0, aLastAccessed);
>-    if (NS_SUCCEEDED(rv)) {
>-      rv = mDBState->stmtUpdate->BindInt64Parameter(1, aCookie->CreationID());
>-      if (NS_SUCCEEDED(rv)) {
>-        PRBool hasResult;
>-        rv = mDBState->stmtUpdate->ExecuteStep(&hasResult);
>-      }
>-    }
>+    // Bind our parameters.
>+    nsresult rv = params->BindInt64ByIndex(0, aLastAccessed);
>+    if (NS_FAILED(rv)) return;
>+    rv = params->BindInt64ByIndex(1, aCookie->CreationID());
>+    if (NS_FAILED(rv)) return;

Same here: BindInt64ByIndex can only fail on OOM or if an internal invariant is broken, based on my reading, so let's just assert that 'rv' succeeded. (Or if BindInt64ByIndex already asserts on failure, then don't do anything here. Returning early is bad, though, because we skip the logging below.)

Comment 19

[[e-mail]]

Comment on attachment 431709 [details] [diff] [review]
Part 1 - Insert new cookies async v1.2

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

> static nsresult
>-bindCookieParameters(mozIStorageStatement *aStmt, const nsCookie *aCookie)
>+bindCookieParameters(mozIStorageBindingParamsArray *aParamsArray,
>+                     const nsCookie *aCookie)
> {
>+  NS_ASSERTION(aParamsArray, "Null params array passed to bindCookieParameters!");
>+  NS_ASSERTION(aCookie, "Null cookie passed to bindCookieParameters!");
>   nsresult rv;
> 
>-  rv = aStmt->BindInt64Parameter(0, aCookie->CreationID());
>+  // Use the asynchronous binding methods to ensure that we do not acquire the
>+  // database lock.
>+  nsCOMPtr<mozIStorageBindingParams> params;
>+  rv = aParamsArray->NewBindingParams(getter_AddRefs(params));
>   if (NS_FAILED(rv)) return rv;
> 
>-  rv = aStmt->BindUTF8StringParameter(1, aCookie->Name());
>+  // Bind our values to params
>+  rv = params->BindInt64ByIndex(0, aCookie->CreationID());
>   if (NS_FAILED(rv)) return rv;

Same with all this: these can't fail in any reasonable way, right? So ditch all the rv checking? And have bindCookieParameters() return void?

Comment 20

[[e-mail]]

Comment on attachment 431743 [details] [diff] [review]
Part 3 - delete cookies async v1.1

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

>@@ -2546,25 +2584,37 @@ nsCookieService::PurgeCookies(PRInt64 aC

>+  if (paramsArray) {
>+    nsresult rv = stmt->BindParameters(paramsArray);
>+    if (NS_FAILED(rv)) return;

It looks like BindParameters can only fail in a static way, i.e. if there's a coding error. (Either it's not initialized, or the array owner is wrong.) Can we just assert success here?

> void
>-nsCookieService::RemoveCookieFromList(const nsListIter &aIter)
>+nsCookieService::RemoveCookieFromList(const nsListIter              &aIter,
>+                                      mozIStorageBindingParamsArray *aParamsArray)
> {

>+    nsresult rv = params->BindInt64ByIndex(0, aIter.Cookie()->CreationID());
>+    if (NS_FAILED(rv)) return;

Can't fail except by coding error?

>+    rv = paramsArray->AddParams(params);
>+    if (NS_FAILED(rv)) return;

Same?

Either way, these two shouldn't early return.

>+    // If we weren't given a params array, we'll need to remove it ourselves.
>+    if (aParamsArray) {
>+      rv = stmt->BindParameters(paramsArray);

Same here.

r=dwitte

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Hmm. Howcome all five of these virtual implementations can't live in
> DBListenerErrorHandler itself? (I might be missing something here, but it seems
> like all InsertCookieDBListener needs to do is override GetOpType(), and
> nothing else.)
I did it like this for two reasons:
1) better error messages for NOTREACHED (which one was not reached)
2) I thought we may want to add logging when the writing is complete in the future (or even now).

> Nit: needs more 'super'. :)
See bug 507414.

Comment 22

[[e-mail]]

(In reply to comment #21)
> I did it like this for two reasons:
> 1) better error messages for NOTREACHED (which one was not reached)
> 2) I thought we may want to add logging when the writing is complete in the
> future (or even now).

OK. But can AddRef/Release/QI live in the base class?

Comment 23

[Shawn Wilsher :sdwilsh]

Adding an NS_ASSERT_SUCCESS macro in this file and using it in all the cases you pointed out...

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> OK. But can AddRef/Release/QI live in the base class?
I'll see if the compiler is happy with that.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - Insert new cookies async v1.3

Fixing the other two parts will be pretty mechanical, but I want to make sure you like these changes first.

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 431915 [details] [diff] [review]
Part 1 - Insert new cookies async v1.3

bah, I screwed up my patch queue.  Going to have to upload a patch for all three parts :(

Comment 27

[Shawn Wilsher :sdwilsh]

Attachment: Everything v1.0

Comment 28

[[e-mail]]

Comment on attachment 431919 [details] [diff] [review]
Everything v1.0

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp

> void
> nsCookieService::CloseDB()
> {

>-  mDefaultDBState.dbConn = nsnull;
>+  if (mDefaultDBState.dbConn) {
>+    mDefaultDBState.dbConn->AsyncClose(NULL);
>+  }

Don't we still need to null it? Or do we need to keep it alive or something? (And in that case, shouldn't it AddRef() itself and Release() when the close is complete?)

>@@ -842,32 +968,49 @@ nsCookieService::SetCookieStringInternal

>+    rv = mDBState->stmtInsert->ExecuteAsync(&sInsertCookieDBListener,
>+                                            getter_AddRefs(handle));
>+    NS_ASSERT_SUCCESS(rv);
>+    if (NS_FAILED(rv)) {
>+      NS_WARNING("db insert failed!");
>+      COOKIE_LOGSTRING(PR_LOG_WARNING, ("SetCookieStringInternal(): executing insert gave error %x", rv));
>+    }

No need to log, now that the only failure mode is static. Same change in 5 other places.

>+nsCookieService::AddCookieToList(const nsCString               &aBaseDomain,
>+                                 nsCookie                      *aCookie,
>+                                 mozIStorageBindingParamsArray *aParamsArray,
>+                                 PRBool                         aWriteToDB)
> {
>+  NS_ASSERTION(!aWriteToDB || (!mDBState->dbConn || aParamsArray),
>+               "Have a params array but not supposed to write?");

Probably want to split this into two assertions, for clarity:

NS_ASSERTION(!(dbConn && !aWriteToDB && aParamsArray), "Not writing to DB but have a params array?");
NS_ASSERTION(!(!dbConn && !aParamsArray), "Don't have a DB connection but have a params array?");

r=dwitte with fixes.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> Don't we still need to null it? Or do we need to keep it alive or something?
> (And in that case, shouldn't it AddRef() itself and Release() when the close is
> complete?)
It handles the AddRef/Release bits, but we do need to null it out.

> No need to log, now that the only failure mode is static. Same change in 5
> other places.
Zap.

> NS_ASSERTION(!(dbConn && !aWriteToDB && aParamsArray), "Not writing to DB but
> have a params array?");
> NS_ASSERTION(!(!dbConn && !aParamsArray), "Don't have a DB connection but have
> a params array?");
Done.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> NS_ASSERTION(!(!dbConn && !aParamsArray), "Don't have a DB connection but have
> a params array?");
This should actually be this:
NS_ASSERTION(!(!dbConn && aParamsArray), "Don't have a DB connection but have a params array?");
We want aParamsArray to be true in the condition, not false.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: Everything v1.1

Addresses comments.  This is what I'll be landing once the tree looks OK.

Comment 32

[Marco Bonardo [:mak]]

pushed http://hg.mozilla.org/mozilla-central/rev/2d1342ee572b
but
backed out http://hg.mozilla.org/mozilla-central/rev/c465e05b4e88

due to an assertion in Storage params iterators.

###!!! ASSERTION: Obtaining an iterator to the beginning with no elements!: 'length() != 0', file /builds/slave/mozilla-central-linux64-debug/build/storage/src/mozStorageBindingParamsArray.h, line 121
mozilla::storage::BindingParamsArray::begin() (/builds/slave/mozilla-central-linux64-debug/build/obj-firefox/storage/src/../../dist/include/nsAutoPtr.h:0)
mozilla::storage::AsyncExecuteStatements::bindExecuteAndProcessStatement(mozilla::storage::StatementData&, bool) (/builds/slave/mozilla-central-linux64-debug/build/storage/src/mozStorageAsyncStatementExecution.cpp:244)
mozilla::storage::AsyncExecuteStatements::Run() (/builds/slave/mozilla-central-linux64-debug/build/storage/src/mozStorageAsyncStatementExecution.cpp:551)
nsThread::ProcessNextEvent(int, int*) (/builds/slave/mozilla-central-linux64-debug/build/xpcom/threads/nsThread.cpp:527)
NS_ProcessNextEvent_P(nsIThread*, int) (/builds/slave/mozilla-central-linux64-debug/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:250)
nsThread::ThreadFunc(void*) (/builds/slave/mozilla-central-linux64-debug/build/xpcom/threads/nsThread.cpp:253)
_pt_root (/builds/slave/mozilla-central-linux64-debug/build/nsprpub/pr/src/pthreads/ptthread.c:231)
start_thread (/usr/src/debug/glibc-2.5-20061008T1257/nptl/pthread_create.c:296)
clone+0x0000006D  (/lib64/libc.so.6)
TEST-UNEXPECTED-FAIL | automation.py | Exited with code -6 during test run

Comment 33

[Marco Bonardo [:mak]]

a couple logs:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268408272.1268409408.26131.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268407783.1268408711.24066.gz

Comment 34

[Shawn Wilsher :sdwilsh]

We are passing an empty array to the async storage.  I'm not sure if we want to make storage handle that, or if cookies needs to do some checking (dwitte made me take out the place where I was doing this in review).

Comment 35

[(no longer active)]

(In reply to comment #34)
> We are passing an empty array to the async storage.  I'm not sure if we want to
> make storage handle that, or if cookies needs to do some checking (dwitte made
> me take out the place where I was doing this in review).

I might not be an expert here, but why not make storage handle empty arrays gracefully, and treat them as a no-op?  That would be a sane thing to expect from an API, I guess.

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> I might not be an expert here, but why not make storage handle empty arrays
> gracefully, and treat them as a no-op?  That would be a sane thing to expect
> from an API, I guess.
It would mean we'd have to add a bunch of sanity checking in the background thread, which I don't really want to do.  I did just file bug 552003 which would have caught this and is a bug in storage.

Comment 37

[Shawn Wilsher :sdwilsh]

Attachment: Everything v1.2

Per API changes that we discussed.

Comment 38

[[e-mail]]

Comment on attachment 432291 [details] [diff] [review]
Everything v1.2

r=dwitte

Comment 39

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/3c689de8ee14