704030 - Make the "all connections closed" check available on non debug builds

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Nicholas Nethercote [inactive]]

If an async DB connection is destroyed without calling asyncClose() first, we'll end up leaking the SQLite connection.  The interesting thing is that in Connection::Close() we can detect if this is happened, and could recover from it by scheduling some code to destroy the SQLite connection once all its async statements have finished executing.

So there's an interesting API decision here -- do we want the async connection API to be this forgiving?  Some of these leaks may occur at shut-down (I'm pretty sure we have one like this in the current code) and scheduling the extra code might just slow things down.

I'm not sure one way or the other, but sdwilsh wanted me to file this bug :)

Comment 1

[Marco Bonardo [:mak]]

I think there is a relation at least with bug 687726, since espindola's changes will make so that we can detect internal code leaks.
But add-ons can add any sort of problem still.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Can we change Close to use asyncClose and spin?

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This is also related to 758343. If we are going to spin on release builds to make sure connections where asyncClose was called are indeed closed, it might make sense to call asyncClose from a central location too, no?

If all statements that can be canceled are marked as so, this should be almost a nop.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Marco Bonardo [:mak] from comment #1)
> I think there is a relation at least with bug 687726, since espindola's
> changes will make so that we can detect internal code leaks.
> But add-ons can add any sort of problem still.

For example, I hit some assertion failures during xpcom-shutdown-threads that indicate unclosed connection from the Xmarks 4.1.3 and FVD Speed Dial with Full Online Sync 4.1.5 add-ons. These add-ons call Services.storage.openDatabase(), but never call close().

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> For example, I hit some assertion failures during xpcom-shutdown-threads
> that indicate unclosed connection from the Xmarks 4.1.3 and FVD Speed Dial
> with Full Online Sync 4.1.5 add-ons. These add-ons call
> Services.storage.openDatabase(), but never call close().

These are sync databases, right? In theory we could relax the assert, since with a sync db all disk operations are executed immediately and we shouldn't get any data loss from it not being closed.

When we first added the assert the reasoning for catching sync connections too was that we want users to switch to async ones, but it is fairly surprising if just changing from sync to async now causes data loss on release builds (and asserts on debug bulids).

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Chris and I talked a bit about this. We should try to make this visible to add-on developers without requiring them to use a debug build.

For exit(0) we discussed the idea of using an environment variable to select 3 shutdown behaviours
* Crash on a late write
* Report it via telemetry
* Exit early

We could use the same variable in here, except that it would be open connections instead late writes.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Having this in place would help with debugging bug 834945. I will try to write a patch for selecting the behaviour with an environment variable.

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=c2bcc2ba81cf

Comment 9

[Vladan Djeric (:vladan)]

Comment on attachment 711384 [details] [diff] [review]
patch

Patch looks good, but I notice it doesn't do anything in the SCM_RECORD case. Are you planning to report the names of DBs that were still open during the shutdown sequence to Telemetry?

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Vladan Djeric (:vladan) from comment #9)
> Comment on attachment 711384 [details] [diff] [review]
> patch
> 
> Patch looks good, but I notice it doesn't do anything in the SCM_RECORD
> case. Are you planning to report the names of DBs that were still open
> during the shutdown sequence to Telemetry?

Oh, sorry. I forgot to mention. I intend to use this variable for other shutdown checks. In particular, it should be used for late writes and early exit too. In that case the behaviour will be

*) crash: what we currently have a MOZ_ASSERT on late writes for.
*) record: normal path right now: record to disk and let telemetry report it.
*) nothing: don't even poison writes. Exit(0) early at some point.

I can drop that enum item for now if you want, but I should be implementing the above early next week.

Comment 11

[Vladan Djeric (:vladan)]

It's fine, leave it in the enum then

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 711384 [details] [diff] [review]
patch

Adding r? per comment 11.

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50df17d70386

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50df17d70386