1091851 - Sqlite wrapped connections should close before the underlying connection

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we need a way to observe shutdown of the wrapped connection, so that we can finalize the cached statements before the connection goes away.

This likely requires an additional Storage API.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please estimate?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this requires changes in Storage, likely a new API (brainstorming, coordination...)

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, here we need a new API that allows to observe a connection, something like mozIStorageAsyncConnection.addObserver
At the beginning the only observed event will be when the connection is shutting down, in future more events could be observed.
We need to figure if we want a specific API for this, or reuse nsIObserver.

once done we can just use that in Sqlite.jsm to shutdown the wrapper.

Comment 4

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

For the purposes of discussing bug 1125157, I think I'm missing the motivation for this.

Comment 0 implies that a mozStorage connection being shutdown could be a surprise.  I don't think this is the case.  The only reason a Connection would automatically close itself would be if its destructor is triggered.  Since mozStorage is not cycle-collector aware and all statements hold a strong reference to their connection, the destructor being triggered implies that there must not be any statements that are alive.

Which means that any close is going to be explicit.  And since it sounds like Sqlite.jsm is involved, it seems like it's best for mozStorage itself to stay out of that and leave it to Sqlite.jsm.  Unless the issue is that Places is using mozStorage for native stuff and Sqlite.jsm-wrapped stuff and wants to coordinate those shutdowns using mozStorage as a communication mechanism?

NB: I did look at SQLite.jsm to see if its magic memory management stuff could be involved.  But AFAICT the finalizer witness it uses is just a last-ditch cleanup effort, and it never automatically finalizes cached statements (though it does have a public API to do it.)

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> Comment 0 implies that a mozStorage connection being shutdown could be a
> surprise.  I don't think this is the case.  The only reason a Connection
> would automatically close itself would be if its destructor is triggered. 
> Since mozStorage is not cycle-collector aware and all statements hold a
> strong reference to their connection, the destructor being triggered implies
> that there must not be any statements that are alive.

So the fact is that Sqlite.jsm has an API that allows it to wrap (as a javascript-nice API) an existing mozStorage connection.

A component like Places (more might come, and add-ons could) uses these wrappers to have a nice promise based API to use in js code. The connection is still managed by Places cpp code, included opening and closing it. Sqlite.jsm only wraps execution APIs and keeps a statements cache, it can't close the connection (or it would close it under the original owner).

So the problem is that from Sqlite.jsm we need to know when the connection owner is closing the connection cause we need to finalize the statements we have in cache.
While for the single places-sqlite.jsm case I could make Places itself close the wrapper, it would be very specific and any other wrappers would still warn like crazy on connection close (cause statements are not finalized).

The other use-case I was presenting was for corrupt, it happened that random API calls return SQLITE_CORRUPT but most of the consumers only check that on opening a connection. So it might be useful to have an API where Storage tells you "i encountered a corruption, you should do something about that".

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Note that this issue exists only because we warn in case of unfinalized statements on connection close. We could even decide that the automatic finalization we do in Storage is good enough, and stop caring. Originally we were warning cause the connection could not be closed, but today we auto-finalize statements through sqlite3_next_stmt().

Comment 7

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

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> Note that this issue exists only because we warn in case of unfinalized
> statements on connection close. We could even decide that the automatic
> finalization we do in Storage is good enough, and stop caring. Originally we
> were warning cause the connection could not be closed, but today we
> auto-finalize statements through sqlite3_next_stmt().

Safe auto-finalizing on close does seem like a better failure mode for mozStorage than what we have now.  Specifically, it would seem desirable for close() to always work and for the misuse to just result in subsequent attempts to use the statements to fail-fast.

I think the major risk right now with sqlite3_next_stmt is that it frees the memory, leaving all the Statements hanging onto the freed mDBStatement which can subsequently result in a crash.  I remember a semi-recent discussion where this came up and I think the options that jumped out were to a) keep a list of the live statements so we could null out their mDBStatements / otherwise notify them, or b) make sure that all statements properly check their mDBConnection's isClosing/isClosed so they would never use their dead pointers.  I rather doubt we're at "b" already, although again I'd want to re-read my previous analysis.  Both of these seem within reach.


In terms of this specific Places scenario, I think the right thing is to have Places coordinate shutdowns of any wrappers.  The only safe asynchronous notification scenarios we have are a) "hey, I already closed the connection, all of the stuff you are using is now dead and useless", or b) "hey, someone has indicated they want to AsyncClose() this connection, but I'm not going to actually start until you call this callback/resolve this promise, that way I'll know you've finalized all your statements."  Option A isn't particularly hard to implement, but seems like it could lead to your JS code potentially end up throwing an arbitrary number of errors in between the time when the connection has actually been closed down and when the code actually hears about it.  Option B is probably close to what Places might need to do on its own, but I would argue/hope that Places is the only one that ever has its structure and so it's better to avoid mozStorage taking on the additional control flow complexity.

Places' hybrid use of SQLite connections by C++ code and JS code seems somewhat unique and unlikely to recur.  We are aggressively opposed to native add-ons, which rules that out.  And I would expect new code to do one of the following instead:
- All JS code, on a worker (or main thread) using IndexedDB
- All JS code on a worker using js-ctypes against SQLite
- Hybrid, but with the C++ bit exposed to JS through WebIDL (rather than XPConnect) so the JS code can also run on a worker.  For example, this is how the Service Worker cache stuff works.
- The hybrid webidl thing but with Rust instead of C++.


Having said that, Places is the primary consumer of mozStorage these days.  I don't know how it works and whether I'm asking you to do horrible contortions in places instead.  If it still seems like close listener is appealing, if you could sorta sketch out the sequence of events that would occur when this mechanism is invoked, I think that would be super helpful, especially in ensuring there won't be potential races.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Marco told me he won't get to it this iteration. We'll drop this for now and it will probably be picked up again soon.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We'll fix this on the Places side, and leave Storage alone for now.

The biggest problem is making a fix that is "global" for all of Places, I'm unifying the various wrapped connections around into a single PlacesUtils.promiseWrappedConnection() in bug 1125115, but the history connection is already a little bit "exotic" since it's blocking shutdown.
We need to merge the history connection behavior into the global wrapped connection, so make it block shutdown, but at a point where we can still .close() before the underlying connection goes away.

I will likely discuss this with Yoric, or even ask him if he has the time to look into that. Should not be extremely hard, but requires some good knowledge of async and places shutdown, that he has.
Regardless, we first need bug 1125115 to land, that should happen soon-ish.

Comment 10

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

Note that https://reviewboard.mozilla.org/r/4975/ should cover some (most?) of this.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah, we need to setup a timeline for landing these things, or we are just going to bitrot each other. Or I could just handoff shutdown handling to you so you can properly enqueue these tasks.
Regardless, it's clear we need your shutdown work sooner than expected, so I'll have to reprioritize that review to happen sooner.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 1152333 fixed this for a while, but looks like bug 1043863 somehow broke that fix.

Comment 13

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

Attachment: MozReview Request: Bug 1091851 - Make sure that Places connections are closed timely;r=mak

Bug 1091851 - Make sure that Places connections are closed timely;r=mak

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://reviewboard.mozilla.org/r/10109/#review8933

::: toolkit/components/places/PlacesUtils.jsm:2076
(Diff revision 1)
> +          conn.close.bind(conn));

Could we do the same in gAsyncDBWrapperPromised and remove the places-will-close-connection "hack"?
I think makes sense to keep the code for the 2 connections similar.

Comment 15

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

https://reviewboard.mozilla.org/r/10109/#review8935

> Could we do the same in gAsyncDBWrapperPromised and remove the places-will-close-connection "hack"?
> I think makes sense to keep the code for the 2 connections similar.

Definitely.

Comment 16

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

Comment on attachment 8614916 [details]
MozReview Request: Bug 1091851 - Make sure that Places connections are closed timely;r=mak

Bug 1091851 - Make sure that Places connections are closed timely;r=mak

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8614916 [details]
MozReview Request: Bug 1091851 - Make sure that Places connections are closed timely;r=mak

https://reviewboard.mozilla.org/r/10111/#review8943

Comment 18

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

Attachment: MozReview Request: Bug 1091851 - Fixing a race condition in Sqlite.jsm shutdown;r=mak

Bug 1091851 - Fixing a race condition in Sqlite.jsm shutdown;r=mak

Comment 19

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

https://reviewboard.mozilla.org/r/10233/#review8945

Looks like we didn't shutdown properly in Sqlite.jsm either.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8615288 [details]
MozReview Request: Bug 1091851 - Fixing a race condition in Sqlite.jsm shutdown;r=mak

https://reviewboard.mozilla.org/r/10233/#review8947

Ship It!

Comment 21

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbe237319322
Apparently, the test failures are bug 1122520.

Comment 22

[Pulsebot]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1fba04d83168
https://hg.mozilla.org/mozilla-central/rev/ca501a867019

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Would this be reasonably safe to uplift to Beta40 to help alleviate bug 1146705? If so, please nominate it :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Got a no to that question on IRC.