Closed Bug 1149373 Opened 5 years ago Closed 5 years ago

Async mozStorage threads are not being shut down again

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bent.mozilla, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 802239 for the first incarnation.

How do we make sure this doesn't break again in the future?
Looks like this was broken again in bug 874814.
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.
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.
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.
Alternatively, we could make mAsyncExecutionThread be hold by a nsMainThreadPtrHandle, but make it be an extended nsThread class that calls Shutdown on destruction.
I think the easiest is along the lines of comment 3...
Attached patch patch v1 (obsolete) — Splinter Review
Did you have something like this in mind?
Attachment #8591044 - Flags: review?(bent.mozilla)
Attachment #8591044 - Flags: feedback?(dteller)
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()`.
Attachment #8591044 - Flags: feedback?(dteller) → feedback+
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.
Attachment #8591044 - Flags: review?(bent.mozilla) → review+
Attached patch patch v1.1Splinter Review
yep, good idea making this debug-only.
Assignee: nobody → mak77
Attachment #8591044 - Attachment is obsolete: true
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/9f86c427fb94
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/9f86c427fb94
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.