Bug 229886 - Regression(r277571) Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails
Summary: Regression(r277571) Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() fro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-03 12:40 PDT by Chris Dumez
Modified: 2021-09-03 15:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.33 KB, patch)
2021-09-03 12:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.13 KB, patch)
2021-09-03 13:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-09-03 12:40:16 PDT
Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails since r277571 and logs:
0x104de9900 - ResourceLoadStatisticsDatabaseStore::turnOnIncrementalAutoVacuum failed, error message: not an error
Comment 1 Chris Dumez 2021-09-03 12:40:33 PDT
<rdar://82581507>
Comment 2 Chris Dumez 2021-09-03 12:44:32 PDT
Created attachment 437299 [details]
Patch
Comment 3 Darin Adler 2021-09-03 13:42:43 PDT
Comment on attachment 437299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437299&action=review

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:630
>      int autoVacuumMode = 0;
> -    if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s))
> +    int error = 0;
> +    if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) {
>          autoVacuumMode = statement->columnInt(0);
> -    int error = lastError();
> +        error = lastError();
> +    }

Seems like we can straighten out the logic a little. If we can’t prepare the statement, we return false, but for some reason we implement that by setting error to "0" and then comparing it with SQLITE_ROW. I would write:

    auto statement = prepareStatement("PRAGMA auto_vacuum"_s);
    if (!statement)
        return false;

    // <Move here and/or improve long confusing comment about error codes that does not even mention SQLITE_ROW>?
    auto autoVacuumMode = statement->columnInt(0); 
    if (lastError() != SQLITE_ROW)
        return false;

I like that better than having a local variable named error. The code below also doesn’t need a local variable.

    return lastError() == SQLITE_OK;
Comment 4 Chris Dumez 2021-09-03 13:44:26 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 437299 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437299&action=review
> 
> > Source/WebCore/platform/sql/SQLiteDatabase.cpp:630
> >      int autoVacuumMode = 0;
> > -    if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s))
> > +    int error = 0;
> > +    if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) {
> >          autoVacuumMode = statement->columnInt(0);
> > -    int error = lastError();
> > +        error = lastError();
> > +    }
> 
> Seems like we can straighten out the logic a little. If we can’t prepare the
> statement, we return false, but for some reason we implement that by setting
> error to "0" and then comparing it with SQLITE_ROW. I would write:
> 
>     auto statement = prepareStatement("PRAGMA auto_vacuum"_s);
>     if (!statement)
>         return false;
> 
>     // <Move here and/or improve long confusing comment about error codes
> that does not even mention SQLITE_ROW>?
>     auto autoVacuumMode = statement->columnInt(0); 
>     if (lastError() != SQLITE_ROW)
>         return false;
> 
> I like that better than having a local variable named error. The code below
> also doesn’t need a local variable.
> 
>     return lastError() == SQLITE_OK;

Your proposal is extending the lifetime of the statement. I was trying to keep the same lifetime as before r277571 (which is why I had added the scope in the first place). Maybe it doesn't matter though.
Comment 5 Chris Dumez 2021-09-03 13:52:39 PDT
Created attachment 437308 [details]
Patch
Comment 6 Alex Christensen 2021-09-03 14:43:39 PDT
Comment on attachment 437308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:625
> +    auto statement = prepareStatement("PRAGMA auto_vacuum"_s);

I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to.
Comment 7 Chris Dumez 2021-09-03 14:46:26 PDT
Comment on attachment 437308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review

>> Source/WebCore/platform/sql/SQLiteDatabase.cpp:625
>> +    auto statement = prepareStatement("PRAGMA auto_vacuum"_s);
> 
> I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to.

I thought about it and I personally don't see how it changes behavior. Yes, 2 statements will live at the same time but I don't see any issue with that (except that we're using a very tiny amount of extra memory for a little bit longer).
Comment 8 EWS 2021-09-03 15:16:08 PDT
Committed r282030 (241334@main): <https://commits.webkit.org/241334@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437308 [details].