In bug 910226, in his great wisdom, asuth said > So, it seems like the death problem is definitely that > > Statement::internalFinalize called by Statement::~Statement does: > > int srv = ::sqlite3_finalize(mDBStatement); > mDBStatement = nullptr; > > regardless of whether the connection is alive or not. It needs to check the > connection's closed state. (The connection must still be alive.) > > It seems like we can add a unit test to verify this becomes a non-crasher: > - Create sync connection > - Create sync statement, keep it alive > - Run asyncClose on connection, wait for the close to notify as complete. > - Finalize sync statement. Don't crash.

Note that we'll need to be careful to ensure that we have no race condition between asyncClose and internalFinalize.

Attaching a first version.

Try: https://tbpl.mozilla.org/?tree=Try&rev=182f890696d3

Comment on attachment 798525 [details] [diff] [review] Making sure that we don't finalize a statement after the connection is closed Looks, good, but I think we need a comment/readability fix. Specifically, this fixes the following case too that a naive reading of the code/patch suggests it should not: 1) Create sync statement. (Do not create any async statements.) 2) Close connection. Now succeeds where previously it failed, leaving an orphaned statement. 3) Finalize sync statement. I just skimmed the unit tests, and I don't think we have a test for the former, so we should probably add it too. The above synchronous case should work out fine because we set 'mAsyncExecutionThreadShuttingDown' to true, with proper mutex holding, in Connection::setClosedState which is called by Connection::Close. I suggest that we rename isAsyncClosing to just isClosing to avoid the confusion and add some comments at mAsyncExecutionThreadShuttingDown's definition site to indicate that it is also set by the synchronous close process as well. (Which makes sense, since we don't want async things to start happening to a closed connection.) Clearing the review request of :mak since I'm assuming this was a case where you just wanted whomever would review it first, but please re-add mak if you did want his review too. (And :mak, feel free to chime in either way! :)

Applied feedback, added more doc.

Comment on attachment 798781 [details] [diff] [review] Making sure that we don't finalize a statement after the connection is closed, v2 r=asuth. I love the comments!

Try: https://tbpl.mozilla.org/?tree=Try&rev=4b7d01048f70 Thanks :)

Comment on attachment 798781 [details] [diff] [review] Making sure that we don't finalize a statement after the connection is closed, v2 Review of attachment 798781 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/src/mozStorageStatement.cpp @@ +358,5 @@ > return NS_OK; > > + int srv = SQLITE_OK; > + > + if (!mDBConnection->isClosing(true)) { How did we determine that the connection is closing at this point (reason for which we pass a true argument)? internalFinalize can be invoked at any time, even with a well alive connection, just by invoking finalize(). Maybe it's just the argument naming that is strange? Basically here the async thread is not shutting down yet and connectionReady is still returning true (I think this is somehow related to bug 726990), you added an argument that basically should be named aIgnoreConnectionReady. I'm pretty sure to be missing something, but I don't easily understand the approach. I wonder if we should rather make connectionReady a bit more strict (like, once asyncClose has been invoked, it returns false). That said, it's not a big deal to keep it as is, I just find that the new isClosing argument looks strange. If I'm somehow right, it would be fine to clenaup the thing in a new separate bug.

(In reply to Marco Bonardo [:mak] from comment #11) > Comment on attachment 798781 [details] [diff] [review] > Making sure that we don't finalize a statement after the connection is > closed, v2 > > Review of attachment 798781 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: storage/src/mozStorageStatement.cpp > @@ +358,5 @@ > > return NS_OK; > > > > + int srv = SQLITE_OK; > > + > > + if (!mDBConnection->isClosing(true)) { > > How did we determine that the connection is closing at this point (reason > for which we pass a true argument)? > internalFinalize can be invoked at any time, even with a well alive > connection, just by invoking finalize(). > Maybe it's just the argument naming that is strange? The |true| only asks |isClosing| to return |true| if the connection is either closing or closed, which is what we are interested in. Passing |false| or nothing keeps the existing default behavior, i.e. return |true| if the connection is closing, |false| if it is already closed. > Basically here the async thread is not shutting down yet and connectionReady > is still returning true (I think this is somehow related to bug 726990), you > added an argument that basically should be named aIgnoreConnectionReady. Well, yes I did. > I'm pretty sure to be missing something, but I don't easily understand the > approach. I wonder if we should rather make connectionReady a bit more > strict (like, once asyncClose has been invoked, it returns false). > > That said, it's not a big deal to keep it as is, I just find that the new > isClosing argument looks strange. If I'm somehow right, it would be fine to > clenaup the thing in a new separate bug. Fine with me.

(In reply to David Rajchenbach Teller [:Yoric] from comment #12) > The |true| only asks |isClosing| to return |true| if the connection is > either closing or closed, which is what we are interested in. > Passing |false| or nothing keeps the existing default behavior, i.e. return > |true| if the connection is closing, |false| if it is already closed. passing |true| means "ignore che connection ready status, just return true unless the async thread is gone" afaict, there is no check that the connection is closing or closed.

The async thread naming there is somewhat of a misnomer for mAsyncExecutionThreadShuttingDown since a fully sync connection's call to close will set the same flags thanks to internalClose. I talked about this a little in comment 5, I believe.