1381458 - Unfinalized async statement if executed just before AsyncClose

Status: RESOLVED INVALID

(Toolkit :: Storage, defect, P1)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't know if this is a recent regression, but while working on bug 1354032, the code looked like this:

  nsCOMPtr<mozIStorageAsyncStatement> optimizeStmt;
  nsresult rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
    "PRAGMA optimize(0x02)"
  ), getter_AddRefs(optimizeStmt));
  MOZ_ASSERT(NS_SUCCEEDED(rv));
  if (NS_SUCCEEDED(rv)) {
    nsCOMPtr<mozIStoragePendingStatement> ps;
    MOZ_ALWAYS_SUCCEEDS(optimizeStmt->ExecuteAsync(nullptr, getter_AddRefs(ps)));
  }

  (void)mMainConn->AsyncClose(connectionShutdown);

This should work, the async statement is executed before asyncClose, and finalization should happen when async statement execution removes its refcount, that should happen before the asyncClose runnable.

But this caused the unfinalized statement assertion on Unix machines (Windows was fine, that makes me think to the different thread scheduler behavior):

[task 2017-07-15T13:15:51.702520Z] 13:15:51     INFO - GECKO(1654) | [1654] WARNING: SQL statement 'PRAGMA optimize(0x02)' (7f3a6b4de8a0) should have been finalized before closing the connection: file /home/worker/workspace/build/src/storage/mozStorageConnection.cpp, line 1062
[task 2017-07-15T13:15:51.720047Z] 13:15:51     INFO - GECKO(1654) | Assertion failure: false (Had to forcibly close the database connection because not all the statements have been finalized.), at /home/worker/workspace/build/src/storage/mozStorageConnection.cpp:1088

I didn't have the time to look into this yet, for now I've just replaced the call with executeSimpleSqlAsync (that is what I should have used regardless). I'm rebuilding a Linux VM to check if I can debug something out of it in the next days.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

I think for what you're doing to be safe you'd need to ensure that you've dropped your reference before invoking AsyncClose.  Looking at https://hg.mozilla.org/integration/autoland/rev/03b8979ccf32 that does not appear to be the case; optimizeStmt's reference is dropped when the method returns, which is strictly after the call to AsyncClose().  The block you added wants to be wrapped in curly braces to force the ref to be dropped.

It seems reasonable to lose that race at least some of the time; the PRAGMA is going to parse and execute quickly, and it's believable that AsyncExecuteStatements::notifyComplete() would reach the mStatements.Clear(); call in some cases.  This is especially true given that Connection::setClosedState() called by AsyncClose() could end up finding that sharedAsyncExecutionMutex is held by the async thread, causing the main thread to sleep at that point.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ugh, you are right, it is just an ownership problem :(