Bug 237130 – SQLiteDatabase::open should return early if journal mode cannot be set

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

SQLiteDatabase should bail out if opening fails

[Ben Nham]

Created attachment 453076 [details]
Patch

[Ben Nham]

<rdar://83130954>

[Sihui Liu]

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?

[Sihui Liu]

Created attachment 453671 [details]
Patch

[Sihui Liu]

Created attachment 453774 [details]
Patch

[Darin Adler]

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.

[Sihui Liu]

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.

[Ben Nham]

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).

[Sihui Liu]

Created attachment 453817 [details]
Patch for landing

[EWS]

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].