1123334 - Use SQLITE_ENABLE_API_ARMOR in debug mode

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P2)

Description

[Marco Bonardo [:mak]]

We can evaluate enabling this new option in debug mode, it might allow us to find bugs.
Though, first we need a Try run with it to ensure it won't break tests.

Comment 1

[Marco Bonardo [:mak]]

https://hg.mozilla.org/try/rev/4cffe1eedd1ba7aa7349f0f331c48e01177bd7e5

Comment 2

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cffe1eedd1ba7aa7349f0f331c48e01177bd7e5

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1123334 - Use SQLITE_ENABLE_API_ARMOR in debug mode.

Review commit: https://reviewboard.mozilla.org/r/91078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91078/

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8808237 [details]
Bug 1123334 - Use SQLITE_ENABLE_API_ARMOR in debug mode.

argh, I used the wrong define name!

Comment 5

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=849089a59b387ac63235740db2e1bd37cf9c0a30

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8808237 [details]
Bug 1123334 - Use SQLITE_ENABLE_API_ARMOR in debug mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91078/diff/1-2/

Comment 7

[Marco Bonardo [:mak]]

I wonder if this may allow us to drop SQLITE_DEBUG... I thought we were not using it, but we do in debug mode. SQLITE_DEBUG is really really slow, and I'm under the impression it is more useful for Sqlite internal development than for its consumers. We'd drop a bunch of internal asserts, but some tests would be three times faster.

Comment 8

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

Comment on attachment 8808237 [details]
Bug 1123334 - Use SQLITE_ENABLE_API_ARMOR in debug mode.

https://reviewboard.mozilla.org/r/91078/#review91014

Restating my understanding:
- By enabling this, we cause SQLite to perform additional checks which will cause early returns from SQLite3 API calls with a return value of SQLITE_MISUSE which we will map to NS_ERROR_UNEXPECTED.  This is the only observable change in behavior from our perspective at this time.
- SQLite will also invoke sqlite3_log when doing this, which would result in a call to the function registered via `sqlite3_config(SQLITE_CONFIG_LOG, ...)`, but we don't currently do this.
- So a fancy thing we could do in the future would be to implement a logging function that checks when the passed-in error code is SQLITE_MISUSE and does a MOZ_ASSERT/MOZ_CRASH.  We could also be less fancy and not crash and instead just direct the output to MOZ_LOG, but almost no one reads logs so it's not clear that buys us much.
- But non-crashing MOZ_LOG exposure of sqlite3_log could make sense for errors that aren't SQLITE_MISUSE.

Maybe file a spin-off about SQLITE_CONFIG_LOG?  Especially since :drh mentioned it in https://bugzilla.mozilla.org/show_bug.cgi?id=1244038#c16, I suspect it would be helpful to us to be helpful to team SQLite to be able to provide its output with any logs we provide.  (Since you're being super proactive about mozStorage, maybe his mention of CORRUPT could also merit its own spin-off to help provide connections with easier corrupt-database-handling logic.)

Comment 9

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

(In reply to Marco Bonardo [::mak] from comment #7)
> I wonder if this may allow us to drop SQLITE_DEBUG... I thought we were not
> using it, but we do in debug mode. SQLITE_DEBUG is really really slow, and
> I'm under the impression it is more useful for Sqlite internal development
> than for its consumers. We'd drop a bunch of internal asserts, but some
> tests would be three times faster.

Agreed that we should drop it.  From my perspective, the only real utility of SQLITE_DEBUG is that it gets us "PRAGMA vdbe_trace"/friends (https://www.sqlite.org/opcode.html#viewing_the_bytecode) and -DSQLITE_ENABLE_EXPLAIN_COMMENTS (https://www.sqlite.org/compile.html#enable_explain_comments) is automatically defined for us.

Both of these things are cool, but absolutely not justified for being turned on all in DEBUG builds.  In many cases a command-line sqlite3 binary with these features can be used for any debugging/performance investigations or a custom Firefox build spun.

r=asuth on removing the line if you want to fold it into your patch.

ps: All these cleanups and maintenance is awesome!  Woohoo!

Comment 10

[Marco Bonardo [:mak]]

(In reply to Andrew Sutherland [:asuth] from comment #8)
> Maybe file a spin-off about SQLITE_CONFIG_LOG?  Especially since :drh
> mentioned it in https://bugzilla.mozilla.org/show_bug.cgi?id=1244038#c16, I
> suspect it would be helpful to us to be helpful to team SQLite to be able to
> provide its output with any logs we provide.  (Since you're being super
> proactive about mozStorage, maybe his mention of CORRUPT could also merit
> its own spin-off to help provide connections with easier
> corrupt-database-handling logic.)

It's a very good idea, I filed bug 1315897.

(In reply to Andrew Sutherland [:asuth] from comment #9)
> Both of these things are cool, but absolutely not justified for being turned
> on all in DEBUG builds.  In many cases a command-line sqlite3 binary with
> these features can be used for any debugging/performance investigations or a
> custom Firefox build spun.

I filed bug 1315900. The real problem is that our SqliteMutex wrapper has assertCurrentThreadOwns and assertNotCurrentThreadOwns that use sqlite3_mutex_held and sqlite3_mutex_notheld, that are only provided if SQLITE_DEBUG is defined. We use them in various parts of the code.
I'll file an investigation bug, but it may not be as trivial as I hoped.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f1c0c2af6b0
Use SQLITE_ENABLE_API_ARMOR in debug mode. r=asuth

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/6f1c0c2af6b0