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/

chrome/content/zotero/xpcom/db.js

@@ -559,6 +559,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);
+		}
+		
 		// If the transaction was committed before the error, don't run rollback
 		// callbacks, since the data was saved
 		if (committed) {
@@ -727,7 +733,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)
@@ -736,6 +742,8 @@ Zotero.DBConnection.prototype.queryAsync = async function (sql, params, options
 						) + '] '
 					: '')
 				+ '[ERROR: ' + e.errors[0].message + ']');
+			newError.corruptionChecked = e.corruptionChecked;
+			throw newError;
 		}
 		else {
 			throw e;
@@ -1948,6 +1956,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;

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;

test/tests/dbTest.js

@@ -377,6 +377,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);
+		});
 	})