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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8630246 [details] [diff] [review]
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.
I have a patch, but it has a few rough edges still.
Assignee: nobody → khuey
Created attachment 8630854 [details] [diff] [review]
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.
Attachment #8630246 - Attachment is obsolete: true
Attachment #8630854 - Flags: review?(amarchesini)
Attachment #8630854 - Flags: review?(Jan.Varga)
Attachment #8630854 - Flags: feedback?(bent.mozilla)

Comment 3

3 years ago
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()
Attachment #8630854 - Flags: review?(Jan.Varga) → review+
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...
Attachment #8630854 - Flags: review?(amarchesini) → review+
(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?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
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
Attachment #8630854 - Flags: feedback?(bent.mozilla) → feedback+
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

Updated

3 years ago
Blocks: 1160459
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.
Flags: needinfo?(khuey)
I relanded a modified version.  Lets see how it goes.
Flags: needinfo?(khuey)
https://hg.mozilla.org/mozilla-central/rev/236004185e7f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.