1180978 - Leaving a database in a blocked state causes bad things to happen

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Testcase

It seems to blow up debug mochitest, parent process xpcshell, and child process xpcshell in three different ways, and I bet it does other interesting things in opt builds.  I think this is related to bug 1160459 and bug 1163365, and maybe even the shutdown hangs.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I have a patch, but it has a few rough edges still.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

So this problem arises when we're in the middle of a FactoryOp when the actor tree is torn down.  FactoryOp is all async, but the ActorDestroy methods are invoked on the whole tree without processing events in between.

If the Database actor has already been created, ActorDestroy on it will invalidate it.  We need to skip starting the version change transaction in OpenDatabaseOp::DispatchToWorkThread in this case.  The other RegisterTransaction entrypoint already guards on an invalidated database, so I added assertions to RegisterTransaction.  Cleaning up happens asynchronously though, so we have to force this to happen synchronously in FactoryOp::ActorDestroy.

I also had to relax some assertions.  In FactoryOp::Run, the asynchronous attempt to SendResults does happen eventually after ActorDestroy is called, so we can now get here in State_Completed, in which case we should do nothing.  We can also get into OpenDatabaseOp::NoteDatabaseClosed in State_WaitingForTransactionsToComplete, which we should treat just like State_DatabaseWorkVersionChange.

It's ugly, but it works.  Flagging baku and janv for review.  Maybe bent will want to look at it eventually too.

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8630854 [details] [diff] [review]
Patch

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

::: dom/indexedDB/ActorsParent.cpp
@@ +18901,5 @@
> +    // Let's see if this is right ...
> +    MOZ_RELEASE_ASSERT(mState == State_WaitingForTransactionsToComplete);
> +    // We didn't get an opportunity to clean up.  Do that now.
> +    mState = State_SendingResults;
> +    mResultCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

I think we call |IDB_REPORT_INTERNAL_ERR()| in all other places where we set mResultCode to NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR

@@ +19500,5 @@
>  {
>    AssertIsOnOwningThread();
>    MOZ_ASSERT(aDatabase);
>    MOZ_ASSERT(mState == State_WaitingForOtherDatabasesToClose ||
> +             mState == State_WaitingForTransactionsToComplete ||

This makes sense to me since we have to allow all states that can happen after a database actor is created.

@@ +19568,5 @@
>    MOZ_ASSERT(mMaybeBlockedDatabases.IsEmpty());
>  
>    if (NS_WARN_IF(QuotaClient::IsShuttingDownOnNonMainThread()) ||
> +      IsActorDestroyed() ||
> +      mDatabase->IsInvalidated()) {

I wonder if other places where we have the same check should be updated too, for example OpenDatabaseOp::SendUpgradeNeeded()

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8630854 [details] [diff] [review]
Patch

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

Ok, it seems that 'SendResults' is not actually sending results when the actor is destroyed.
Maybe a better name would be nice.

::: dom/indexedDB/ActorsParent.cpp
@@ +18902,5 @@
> +    MOZ_RELEASE_ASSERT(mState == State_WaitingForTransactionsToComplete);
> +    // We didn't get an opportunity to clean up.  Do that now.
> +    mState = State_SendingResults;
> +    mResultCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +    SendResults();

this seems wrong...

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrea Marchesini (:baku) from comment #4)
> ::: dom/indexedDB/ActorsParent.cpp
> @@ +18902,5 @@
> > +    MOZ_RELEASE_ASSERT(mState == State_WaitingForTransactionsToComplete);
> > +    // We didn't get an opportunity to clean up.  Do that now.
> > +    mState = State_SendingResults;
> > +    mResultCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> > +    SendResults();
> 
> this seems wrong...

What do you mean?

Comment 6

[Andrea Marchesini [:baku]]

the 'sending' thing I wrote in the review. Just a wrong name I guess. Keep in mind that I'm not familiar with this code so probably I don't get the whole picture of how the actor is used.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d29782c360fe

Comment 8

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

Comment on attachment 8630854 [details] [diff] [review]
Patch

This looks generally fine to me, hopefully you can get rid of the MOZ_RELEASE_ASSERT someday

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing a smattering of "Assertion failure: mState == State_WaitingForTransactionsToComplete, at /builds/slave/m-in-m64-000000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:18910" across various different jobs.

https://treeherder.mozilla.org/logviewer.html#?job_id=11565067&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11566426&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1738cbd55d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

What can we do to move this along. Assuming this is the underlying issue to bug 1160459 (and therefore bug 1121145), we're on the verge of hiding multiple e10s test suites over it.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/236004185e7f

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I relanded a modified version.  Lets see how it goes.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/236004185e7f