Closed Bug 536978 Opened 10 years ago Closed 10 years ago

Cookies should write asynchronously

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 11 obsolete files)

42.88 KB, patch
dwitte
: review+
Details | Diff | Splinter Review
The cookie service should write asynchronously instead of synchronously like it currently does.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #419310 - Attachment is patch: true
Attachment #419310 - Attachment mime type: application/octet-stream → text/plain
Neat. Is this ready for review?
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!
Attachment #419310 - Attachment is obsolete: true
Attachment #427435 - Flags: review?(dwitte)
Attachment #419311 - Attachment is obsolete: true
Attachment #427436 - Flags: review?(dwitte)
Attachment #427438 - Flags: review?(dwitte)
Priority: -- → P1
Whiteboard: [needs feedback dwitte]
Target Milestone: --- → mozilla1.9.3a1
Target Milestone: mozilla1.9.3a1 → mozilla1.9.3a2
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.
Attachment #427435 - Flags: review?(dwitte) → review-
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.
Attachment #427436 - Flags: review?(dwitte) → review-
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.
Attachment #427438 - Flags: review?(dwitte) → review-
Blocks: 547031
Whiteboard: [needs feedback dwitte] → [needs new patch]
Depends on: 496019
(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!)
Attachment #427435 - Attachment is obsolete: true
Attachment #431709 - Flags: review?(dwitte)
(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.
Attachment #427436 - Attachment is obsolete: true
Attachment #431734 - Flags: review?(dwitte)
(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.
Attachment #427438 - Attachment is obsolete: true
Attachment #431743 - Flags: review?(dwitte)
Whiteboard: [needs new patch] → [needs review dwitte]
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.
Attachment #431709 - Flags: review?(dwitte) → review+
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
Attachment #431734 - Flags: review?(dwitte) → review+
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 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 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
Attachment #431743 - Flags: review?(dwitte) → review+
(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.
(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?
Adding an NS_ASSERT_SUCCESS macro in this file and using it in all the cases you pointed out...
(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.
Whiteboard: [needs review dwitte]
Target Milestone: mozilla1.9.3a2 → mozilla1.9.3a3
Fixing the other two parts will be pretty mechanical, but I want to make sure you like these changes first.
Attachment #431709 - Attachment is obsolete: true
Attachment #431915 - Flags: review?(dwitte)
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 :(
Attachment #431915 - Attachment is obsolete: true
Attachment #431915 - Flags: review?(dwitte)
Attached patch Everything v1.0 (obsolete) — Splinter Review
Attachment #431734 - Attachment is obsolete: true
Attachment #431743 - Attachment is obsolete: true
Attachment #431919 - Flags: review?(dwitte)
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.
Attachment #431919 - Flags: review?(dwitte) → review+
(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.
(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.
Attached patch Everything v1.1 (obsolete) — Splinter Review
Addresses comments.  This is what I'll be landing once the tree looks OK.
Attachment #431919 - Attachment is obsolete: true
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
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).
(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.
(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.
Depends on: 552003
Depends on: 552092
Attached patch Everything v1.2Splinter Review
Per API changes that we discussed.
Attachment #431933 - Attachment is obsolete: true
Attachment #432291 - Flags: review?(dwitte)
Comment on attachment 432291 [details] [diff] [review]
Everything v1.2

r=dwitte
Attachment #432291 - Flags: review?(dwitte) → review+
http://hg.mozilla.org/mozilla-central/rev/3c689de8ee14
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a3 → mozilla1.9.3a4
Depends on: 541356
You need to log in before you can comment on or make changes to this bug.