1149373 - Async mozStorage threads are not being shut down again

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

See bug 802239 for the first incarnation.

How do we make sure this doesn't break again in the future?

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Looks like this was broken again in bug 874814.

Comment 2

[Marco Bonardo [:mak]]

Are you referring to the bug you filed in the last days, or is there more breakage?
Currently I think the only protection we have is the assert on destruction.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

To fix the issue, I believe that we should dispatch a call to `mAsyncExecutionThread->Shutdown()` to the main thread `AsyncCloseConnection::Run()`. Then, we can assert in `~Connection` that the thread has been shutdown. The ordering of events should be ok, but will require proper documentation.

Comment 4

[Marco Bonardo [:mak]]

ugh, I didn't notice we removed the ->shutdown() call and nsTrhead destructor doesn't Shutdown() it.
I think David's plan sounds like a good starting point, and yes, we need to assert that the thread has been shutdown.

Comment 5

[Marco Bonardo [:mak]]

Alternatively, we could make mAsyncExecutionThread be hold by a nsMainThreadPtrHandle, but make it be an extended nsThread class that calls Shutdown on destruction.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think the easiest is along the lines of comment 3...

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1

Did you have something like this in mind?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8591044 [details] [diff] [review]
patch v1

Review of attachment 8591044 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, right, I had forgotten that we had a `mAsyncExecutionThread.forget()`.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8591044 [details] [diff] [review]
patch v1

Review of attachment 8591044 [details] [diff] [review]:
-----------------------------------------------------------------

r=me!

::: storage/src/mozStorageConnection.cpp
@@ +366,5 @@
>  #endif // DEBUG
>  
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<nsCOMPtr<nsIThread>>
> +      (mConnection, &Connection::shutdownAsyncThread, mAsyncExecutionThread);
> +    NS_ENSURE_STATE(event);

This can't fail without aborting, so no need to check. MOZ_ASSERT if you're really paranoid :)

@@ +877,5 @@
>    return mConnectionClosed;
>  }
>  
> +void
> +Connection::shutdownAsyncThread(nsIThread *aThread) {

I'd add three asserts here:

  MOZ_ASSERT(!mAsyncExecutionThread);
  MOZ_ASSERT(mAsyncExecutionThreadIsAlive);
  MOZ_ASSERT(mAsyncExecutionThreadShuttingDown);

@@ +880,5 @@
> +void
> +Connection::shutdownAsyncThread(nsIThread *aThread) {
> +  nsresult rv = aThread->Shutdown();
> +  if (NS_SUCCEEDED(rv)) {
> +    mAsyncExecutionThreadIsAlive = false;

Not sure it's worth only doing this if Shutdown() succeeded. First it's highly unlikely that Shutdown() will fail so checking seems unnecessary. Second you're guaranteed to assert later if you don't set this to false here so might as well assert now to make it clearer?

::: storage/src/mozStorageConnection.h
@@ +310,5 @@
>     * sharedAsyncExecutionMutex.
>     */
>    bool mAsyncExecutionThreadShuttingDown;
>  
> +  bool mAsyncExecutionThreadIsAlive;

This should really be DebugOnly<> because it's not used for anything except state tracking for the purposes of assertions.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

yep, good idea making this debug-only.

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/9f86c427fb94

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f86c427fb94