Bug 237130

Created attachment 453076 [details] Patch

<rdar://83130954>

Comment on attachment 453076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453076&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:145 > + if (!isOpen()) { Why not return earlier? E.g. after sqlite3_open_v2() call above?

Created attachment 453671 [details] Patch

Created attachment 453774 [details] Patch

Comment on attachment 453774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453774&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:132 > + int result = SQLITE_OK; Just because there are braces below doesn’t mean we need to initialize this here. I suppose it’s OK. > Source/WebCore/platform/sql/SQLiteDatabase.h:102 > + int checkpoint(CheckpointMode); Why add this return value? No caller is checking it. Is it for future use? > Source/WebCore/platform/sql/SQLiteDatabase.h:172 > + int useWALJournalMode(); Seems like returning an error code as an int with no comment or type to disambiguate and say what the return value means is not a great pattern. In this particular case, the caller just needs to know if this succeeded or failed, so a bool for success would work and I think might be better than returning the error code. I suppose it’s not a *disaster* to return an SQLite error code as an int, but how would I know that’s what this function returns? Elsewhere we use named types whenever possible for error codes, or enums or special-purpose objects. I think booleans for success work too.

Comment on attachment 453774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453774&action=review >> Source/WebCore/platform/sql/SQLiteDatabase.h:102 >> + int checkpoint(CheckpointMode); > > Why add this return value? No caller is checking it. Is it for future use? I was planned to use the return value to decide if error is lethal and we should close database immediately. Then I decided to just keep existing behavior that ignores the checkpoint error. So this is unused; I will revert this change. >> Source/WebCore/platform/sql/SQLiteDatabase.h:172 >> + int useWALJournalMode(); > > Seems like returning an error code as an int with no comment or type to disambiguate and say what the return value means is not a great pattern. > > In this particular case, the caller just needs to know if this succeeded or failed, so a bool for success would work and I think might be better than returning the error code. > > I suppose it’s not a *disaster* to return an SQLite error code as an int, but how would I know that’s what this function returns? Elsewhere we use named types whenever possible for error codes, or enums or special-purpose objects. I think booleans for success work too. Sure, will use bool instead. We can get the error with sqlite3_errcode.

Patch looks reasonable to me. Just for the future, here is what we found: 1. sqlite3_open_v2 can succeed even if you can't open or create the -shm file for whatever reason (e.g. out of disk space or file handles). 2. Executing `PRAGMA journal_mode=wal` seems to fail if you can't open or create the -shm file. But we were ignoring the result of this operation. 3. Then we would try to call sqlite3_wal_checkpoint_v2 on the connection. This actually crashes if you can't open the -shm file. So the workaround in this patch was to add a bailout at (2) rather than crashing at (3).

Created attachment 453817 [details] Patch for landing

Committed r290822 (248058@main): <https://commits.webkit.org/248058@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453817 [details].