Use SQLITE_ENABLE_API_ARMOR in debug mode

RESOLVED FIXED in Firefox 52

Status

()

P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (obsolete)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

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

argh, I used the wrong define name!
Attachment #8808237 - Attachment is obsolete: true
Attachment #8808237 - Flags: review?(bugmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
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

2 years ago
mozreview-review
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.)
Attachment #8808237 - Flags: review?(bugmail) → review+
(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!
(Assignee)

Comment 10

2 years ago
(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

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/6f1c0c2af6b0
Use SQLITE_ENABLE_API_ARMOR in debug mode. r=asuth

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f1c0c2af6b0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.