Bug 229886 – Regression(r277571) Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails since r277571 and logs:
0x104de9900 - ResourceLoadStatisticsDatabaseStore::turnOnIncrementalAutoVacuum failed, error message: not an error

[Chris Dumez]

<rdar://82581507>

[Chris Dumez]

Created attachment 437299 [details]
Patch

[Darin Adler]

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;

[Chris Dumez]

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

[Chris Dumez]

Created attachment 437308 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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

[EWS]

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