From 9244cbe50a0da2ea00c7463cdafbfab91e98aa2b Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 7 Sep 2026 11:03:57 -0400 Subject: [PATCH] Detect database corruption reported by a transaction's commit We only ever checked for corruption errors from transactions in queryAsync(), so a corruption error raised by the COMMIT that mozStorage runs itself bypassed the check. A 10.0 schema upgrade -- which heavily exercises the database -- that failed due to corruption showed "Database upgrade error" and a single Sqlite.sys.mjs frame instead of the prompt offering to restore from a backup. Two users reported this, but it's not clear what triggered it -- corruption usually occurs during a statement, which we did catch. There may have been some statement transaction whose corruption error was caught and ignored rather than being left to abort the transaction, causing SQLite to then block the commit. That's what the test does, and it fails without the fix. https://forums.zotero.org/discussion/133611/ (cherry picked from commit bdaecbb5cc07b989eef84e1e4b8badf9f361c055) --- chrome/content/zotero/xpcom/db.js | 15 +++- chrome/content/zotero/xpcom/zotero.js | 16 ++-- test/tests/dbTest.js | 102 ++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 6 deletions(-) diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js index d93c146990..d4bc0a880f 100644 --- a/chrome/content/zotero/xpcom/db.js +++ b/chrome/content/zotero/xpcom/db.js @@ -545,6 +545,12 @@ Zotero.DBConnection.prototype.executeTransaction = async function (func, options this._transactionID = null; } + // A corruption error from the transaction's own BEGIN or COMMIT is thrown by mozStorage + // instead of by one of our query methods, so it hasn't been checked yet + if (!e?.corruptionChecked) { + await this._checkException(e); + } + // Function to run once transaction has been committed but before any // permanent callbacks if (options.onRollback) { @@ -701,7 +707,7 @@ Zotero.DBConnection.prototype.queryAsync = async function (sql, params, options if (e.errors && e.errors[0]) { var eStr = e + ""; eStr = eStr.indexOf("Error: ") == 0 ? eStr.substr(7): e; - throw new Error(eStr + ' [QUERY: ' + sql + '] ' + let newError = new Error(eStr + ' [QUERY: ' + sql + '] ' + (params ? '[PARAMS: ' + (Array.isArray(params) @@ -710,6 +716,8 @@ Zotero.DBConnection.prototype.queryAsync = async function (sql, params, options ) + '] ' : '') + '[ERROR: ' + e.errors[0].message + ']'); + newError.corruptionChecked = e.corruptionChecked; + throw newError; } else { throw e; @@ -1922,6 +1930,11 @@ Zotero.DBConnection.prototype._revertWALHeader = async function (file) { * check, which detects index inconsistencies that quick_check misses) */ Zotero.DBConnection.prototype._checkException = async function (e, { mainConfirmedCorrupt } = {}) { + // Flag the error so that it isn't checked again as it propagates + if (e && typeof e == 'object') { + e.corruptionChecked = true; + } + if (this._externalDB || !this.isCorruptionError(e) || this._checkingCorruption || this._handlingCorruption) { return true; diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js index d096ce1e51..87f054e06c 100644 --- a/chrome/content/zotero/xpcom/zotero.js +++ b/chrome/content/zotero/xpcom/zotero.js @@ -669,10 +669,14 @@ const { CommandLineOptions } = ChromeUtils.importESModule("chrome://zotero/conte throw e; } - let stack = e.stack ? Zotero.Utilities.Internal.filterStack(e.stack) : null; - Zotero.startupError = Zotero.getString('startupError.databaseUpgradeError') - + "\n\n" - + (stack || e); + // Report the error unless corruption recovery has already started a quit + // or restart + if (!Zotero.skipLoading) { + let stack = e.stack ? Zotero.Utilities.Internal.filterStack(e.stack) : null; + Zotero.startupError = Zotero.getString('startupError.databaseUpgradeError') + + "\n\n" + + (stack || e); + } throw e; } finally { @@ -847,7 +851,9 @@ const { CommandLineOptions } = ChromeUtils.importESModule("chrome://zotero/conte } catch (e) { Zotero.logError(e); - if (!Zotero.startupError) { + // Report a generic error unless a more specific one was set above or corruption + // recovery has already started a quit or restart + if (!Zotero.startupError && !Zotero.skipLoading) { Zotero.startupError = Zotero.getString('startupError', Zotero.appName) + "\n\n" + Zotero.getString('db.integrityCheck.reportInForums') + "\n\n" + e.message ? (e.message + "\n\n" + e.stack) : e; diff --git a/test/tests/dbTest.js b/test/tests/dbTest.js index 30477cdb2d..a8662c040b 100644 --- a/test/tests/dbTest.js +++ b/test/tests/dbTest.js @@ -354,6 +354,108 @@ describe("Zotero.DB", function () { assert.ok(callback1Ran); assert.ok(callback2Ran); }); + + // Corrupt a table's pages, leaving the header, the schema and both table roots + // readable so that the database still opens and bar can be written to + async function createDatabaseWithCorruptTable() { + let dir = await getTempDirectory(); + let dbPath = PathUtils.join(dir, 'test.sqlite'); + let db = new Zotero.DBConnection(dbPath); + await db.queryAsync("CREATE TABLE bar (a INTEGER PRIMARY KEY, b TEXT)"); + await db.queryAsync("CREATE TABLE foo (a INTEGER PRIMARY KEY, b TEXT)"); + await db.executeTransaction(async function () { + for (let i = 0; i < 500; i++) { + await db.queryAsync("INSERT INTO foo VALUES (?, ?)", [i, 'x'.repeat(300)]); + } + }); + let pageSize = await db.valueQueryAsync("PRAGMA page_size"); + await db.closeDatabase(); + for (let suffix of ['-wal', '-shm', '-journal']) { + await IOUtils.remove(dbPath + suffix, { ignoreAbsent: true }); + } + + let bytes = await IOUtils.read(dbPath); + for (let i = 4 * pageSize; i < bytes.length; i++) { + bytes[i] = 0xde; + } + await IOUtils.write(dbPath, bytes); + + db = new Zotero.DBConnection(dbPath); + // Corruption handling is skipped for external databases + db._externalDB = false; + return db; + } + + it("should detect corruption reported by the commit of a transaction", async function () { + let db = await createDatabaseWithCorruptTable(); + let quitStub = sinon.stub(Zotero.Utilities.Internal, 'quit'); + let promptService = Services.prompt; + // Decline the offer to recover + let promptStub = sinon.stub().returns(1); + Services.prompt = { confirmEx: promptStub }; + var e; + try { + e = await getPromiseError(db.executeTransaction(async function () { + await db.queryAsync("INSERT INTO bar VALUES (1, 'written')"); + // SQLite prohibits the commit once a statement in the transaction has + // hit the corrupt pages, so swallow that error the way a caller doing + // optional work would + try { + await db.valueQueryAsync("SELECT COUNT(*) FROM foo"); + } + catch {} + })); + } + finally { + quitStub.restore(); + Services.prompt = promptService; + Zotero.skipLoading = false; + try { + await db.closeDatabase(); + } + catch {} + Zotero.hideZoteroPaneOverlays(); + } + + // The commit is what failed, so the error arrives without the query details + // that queryAsync() adds to a statement error + assert.include(e.message, "database disk image is malformed"); + assert.notInclude(e.message, "[QUERY:"); + assert.equal(promptStub.callCount, 1); + assert.include( + promptStub.args[0][2], + Zotero.getString('db.dbCorrupted', [Zotero.appName, 'test.sqlite']) + ); + }); + + it("shouldn't prompt twice for one corruption error in a transaction", async function () { + let db = await createDatabaseWithCorruptTable(); + let quitStub = sinon.stub(Zotero.Utilities.Internal, 'quit'); + let promptService = Services.prompt; + let promptStub = sinon.stub().returns(1); + Services.prompt = { confirmEx: promptStub }; + var e; + try { + // queryAsync() checks the statement error itself, and the same error then + // propagates out of the transaction + e = await getPromiseError(db.executeTransaction(async function () { + await db.queryAsync("SELECT COUNT(*) FROM foo"); + })); + } + finally { + quitStub.restore(); + Services.prompt = promptService; + Zotero.skipLoading = false; + try { + await db.closeDatabase(); + } + catch {} + Zotero.hideZoteroPaneOverlays(); + } + + assert.include(e.message, "database disk image is malformed"); + assert.equal(promptStub.callCount, 1); + }); })