1834269 - Stop checking in the Storage Service if all the connections have been closed

Status: ASSIGNED

(Core :: SQLite and Embedded Database Bindings, task, P3)

Description

[Marco Bonardo [:mak]]

We use the connections array to check all the connections are closed by their owners.
That's something we probably could avoid.

(Andrew Sutherland [:asuth] (he/him) from Bug 1682955 comment #74)

Shutdown Spinning

I believe we added the shutdown spinning in bug 744294. I think the idea came from your https://bugzilla.mozilla.org/show_bug.cgi?id=744294#c2:

Since we now have a list of all the connections in Storage (used to track memory so far), wouldn't be simpler to do this in Storage leveraging that list (i.e. on shutdown spin in one single point until all the connections in the list are properly closed) rather than in each single component?

I think we've now actually moved to each component handling its own shutdown, with most adopting use of the async shutdown service at this time (ex: SQLite.sys.mjs), or others still doing their own event loop spinning (QuotaManager uses a dedicated "profile-before-change-qm"). So this somewhat moots the original intent.

And if there is code depending on this xpcom-shutdown-threads logic... it probably shouldn't and should explicitly move to using a shutdown blocker that waits for proper shutdown? Specifically, we have the fast shutdown mechanism controlled by the toolkit.shutdown.fastShutdownStage pref which is currently mapped to CCPostLastCycleCollection but the next step (which admittedly has been stalled for quite some time) is xpcom-shutdown-threads and that will terminate the process before firing the observer notification which means anything depending on this spinning for correct/consistent behavior is going to have/cause problems.

Verifying connections are closed

It looks like we added the "make sure we closed all the connections" in bug 711076 which was motivated by bug 673017 where AFAICT it turned out there was a failure to finalize some statements and that caused a failure in closing the connection which then had some knock-on shutdown related effects. So I think the net goal was to ensure that in our DEBUG builds that we would catch logic errors that might cause problems for users in the wild because we would be unable to close our SQLite connections. But in bug 874814 we introduced logic to auto-finalize any non-finalized statements if we encounter a SQLITE_BUSY, so statement finalization behavior was no longer as much of a problem.

That would leave the logic as a hygiene check that we're actually shutting the connections down. But I think we would see 1 of 2 things happen if someone forgets to shutdown an OMT connection:

• Our leak checkers will report a leaked mozStorageConnection and friends.
• Shutdown will hang because nsThreadManager shutdown failed to shut things down and there should be a backtrace identifying the thread that was being bad.

I say OMT connection because the reality is that an (idle) mozStorageConnection created via XPConnect would potentially be torn down as a result of GC. However, since callers would likely be using Sqlite.sys.mjs with its shutdown blocker, I'm not sure that's really a concern because that would happen prior to GC.

My main concern is ensuring the journal is merged before shutdown.

Here is the current proposal:

1. stop waiting for connections to be closed in the Storage Service
2. MT connections could, in debug build, start observing xpcom-shutdown-threads on connection open, stop observing it on connection closed. MOZ_ASSERT if they receive the notification.
3. OMT connections could provide a proof of shutdown blocker, the API to do this is still TBD

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Marco Bonardo [:mak] from comment #0)

2. MT connections could, in debug build, start observing xpcom-shutdown-threads on connection open, stop observing it on connection closed. MOZ_ASSERT if they receive the notification.

This is a very good idea!

3. OMT connections could provide a proof of shutdown blocker, the API to do this is still TBD

Bug 1760855 tracks creating a C++ implementation. Work has not started there yet which is a good news/bad news thing, with the good news being that we could potentially help shape the C++ nsIAsyncShutdownService impl to this end.

One option would be that a given C++ blocker could create named RAII sub-blockers which could in turn create their own named RAII sub-blockers. This would provide for the necessary ability to report what blockers are still outstanding and what they're waiting on. These could also optionally generate MOZ_LOG entries and/or profiler markers to allow for some introspection.

So what would be given to the storage connection would be a sub-blocker that is uniquely named to identify the connection. This would then serve more than just a "proof of blocker" but also serve to expose if there was a failure to close the connection, etc.

Comment 2

[Marco Bonardo [:mak]]

We need a fallback plan though, I'm not sure Bug 1760855 will happen soon, this may happen earlier.
That said, connections not created on the MT are just a few (dom storage, indexeddb, and cookies iirc), so it would be easy to temporarily check them manually, we'd just need to catch new additions, for which we could just ask to pass a YES_I_USE_A_SHUTDOWN_BLOCKER const.

Comment 3

[Marco Bonardo [:mak]]

it would be nice to use mTelemetryName when asserting a connection remained open, thus looking into bug 1638256 first.