Bug 211929 – ITP database should finalize all prepared statements before closing

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

If the database connection is associated with unfinalized prepared statements or unfinished sqlite3_backup objects then sqlite3_close() will leave the database connection open and return SQLITE_BUSY (https://www.sqlite.org/c3ref/close.html).

[Kate Cheney]

<rdar://problem/63246945>

[Kate Cheney]

Created attachment 399430 [details]
Patch

[Sihui Liu]

Comment on attachment 399430 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:345
> +        ASSERT_NOT_REACHED();

According to https://www.sqlite.org/c3ref/finalize.html, sqlite3_finalize() can return error when the last execution of the statement fails, so maybe it's fine to ignore the error of finalize.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:656
> +bool ResourceLoadStatisticsDatabaseStore::finalizeStatements()

I am not familiar with this code; why not use unique pointer for the statements and initialize them only when a statement is used? 
In that case, you can null out those statements before closing database. ~SQLiteStatement() will do finalize(). It should be easier to manage the lifetime of the objects.

[Kate Cheney]

Created attachment 399649 [details]
Patch

[Kate Cheney]

(In reply to Sihui Liu from comment #3)
> Comment on attachment 399430 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399430&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:345
> > +        ASSERT_NOT_REACHED();
> 
> According to https://www.sqlite.org/c3ref/finalize.html, sqlite3_finalize()
> can return error when the last execution of the statement fails, so maybe
> it's fine to ignore the error of finalize.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:656
> > +bool ResourceLoadStatisticsDatabaseStore::finalizeStatements()
> 
> I am not familiar with this code; why not use unique pointer for the
> statements and initialize them only when a statement is used? 
> In that case, you can null out those statements before closing database.
> ~SQLiteStatement() will do finalize(). It should be easier to manage the
> lifetime of the objects.

Good idea! The most recent patch makes this change.

[John Wilander]

Comment on attachment 399649 [details]
Patch

Could the init code be in a function on this form:

if (!statementParam) {
  statementParam = makeUnique<WebCore::SQLiteStatement>(m_database, queryParam);
  if (statementParam->prepare() != SQLITE_OK) {
    RELEASE_LOG_ERROR(Network, "%p - %s failed to prepare statement, error message: %{private}s", this, logStringParam, m_database.lastErrorMsg());
    ASSERT_NOT_REACHED();
    return;
  }
}

[Kate Cheney]

Created attachment 399685 [details]
Patch

[Kate Cheney]

(In reply to John Wilander from comment #6)
> Comment on attachment 399649 [details]
> Patch
> 
> Could the init code be in a function on this form:
> 
> if (!statementParam) {
>   statementParam = makeUnique<WebCore::SQLiteStatement>(m_database,
> queryParam);
>   if (statementParam->prepare() != SQLITE_OK) {
>     RELEASE_LOG_ERROR(Network, "%p - %s failed to prepare statement, error
> message: %{private}s", this, logStringParam, m_database.lastErrorMsg());
>     ASSERT_NOT_REACHED();
>     return;
>   }
> }

Yes, this makes this patch much cleaner. Thanks!

[Sihui Liu]

Comment on attachment 399685 [details]
Patch

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

Generally looks good.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:510
> +        statement = makeUnique<WebCore::SQLiteStatement>(m_database, query);

ASSERT(m_database->isOpen());

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:529
>      ASSERT_UNUSED(resetResult, resetResult == SQLITE_OK);

I see there are a lot of reset()s after each execution of statement. You can try use SQLiteStatementAutoResetScope. Make initializeStatement(rename it something like getScopedStatement) to return Optional<SQLiteStatementAutoResetScope>.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:257
> +        if (m_statisticsStore && !isEphemeral() && is<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore))
> +            downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore).close();
> +

Seems like you can do the close() in ~ResourceLoadStatisticsDatabaseStore. Then you don't need the explicit close().

[Kate Cheney]

(In reply to Sihui Liu from comment #9)
> Comment on attachment 399685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399685&action=review
> 
> Generally looks good.
> 

Thanks for the feedback, this is super helpful.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:510
> > +        statement = makeUnique<WebCore::SQLiteStatement>(m_database, query);
> 
> ASSERT(m_database->isOpen());
> 

Good catch.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:529
> >      ASSERT_UNUSED(resetResult, resetResult == SQLITE_OK);
> 
> I see there are a lot of reset()s after each execution of statement. You can
> try use SQLiteStatementAutoResetScope. Make initializeStatement(rename it
> something like getScopedStatement) to return
> Optional<SQLiteStatementAutoResetScope>.
> 

I didn't know that existed, great! I assume this means no need to call statement->reset()?

> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:257
> > +        if (m_statisticsStore && !isEphemeral() && is<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore))
> > +            downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore).close();
> > +
> 
> Seems like you can do the close() in ~ResourceLoadStatisticsDatabaseStore.
> Then you don't need the explicit close().

Yes, good point. I will change this.

[Kate Cheney]

Created attachment 399751 [details]
Patch

[Kate Cheney]

wincairo does not seem to like SQLiteStatementAutoResetScope

[Kate Cheney]

Created attachment 399762 [details]
Patch

[John Wilander]

Comment on attachment 399762 [details]
Patch

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

Given that Sihui has looked this over, I'm OK with it too. See comments though.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:504
> +SQLiteStatementAutoResetScope ResourceLoadStatisticsDatabaseStore::getScopedStatement(std::unique_ptr<WebCore::SQLiteStatement>& statement, const String query, const String& logString) const

Our style guide says no get prefix. If you get a name conflict with a local variable, you can call it with this->scopedStatement(). Can the query parameters be ref?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:510
> +            RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::%s failed to prepare statement, error message: %{private}s", this, logString.ascii().data(), m_database.lastErrorMsg());

I don't remember the reason for keeping these error messages private. Can they leak something about the user's browsing?

[Kate Cheney]

(In reply to John Wilander from comment #14)
> Comment on attachment 399762 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399762&action=review
> 
> Given that Sihui has looked this over, I'm OK with it too. See comments
> though.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:504
> > +SQLiteStatementAutoResetScope ResourceLoadStatisticsDatabaseStore::getScopedStatement(std::unique_ptr<WebCore::SQLiteStatement>& statement, const String query, const String& logString) const
> 
> Our style guide says no get prefix. If you get a name conflict with a local
> variable, you can call it with this->scopedStatement(). Can the query
> parameters be ref?
> 
Yes to ref, I will change. And I'll update the function name.

> > 
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:510
> > +            RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::%s failed to prepare statement, error message: %{private}s", this, logString.ascii().data(), m_database.lastErrorMsg());
> 
> I don't remember the reason for keeping these error messages private. Can
> they leak something about the user's browsing?

Good point, this can probably be public.

[Kate Cheney]

(In reply to katherine_cheney from comment #15)
> (In reply to John Wilander from comment #14)
> > Comment on attachment 399762 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=399762&action=review
> > 
> > Given that Sihui has looked this over, I'm OK with it too. See comments
> > though.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:504
> > > +SQLiteStatementAutoResetScope ResourceLoadStatisticsDatabaseStore::getScopedStatement(std::unique_ptr<WebCore::SQLiteStatement>& statement, const String query, const String& logString) const
> > 
> > Our style guide says no get prefix. If you get a name conflict with a local
> > variable, you can call it with this->scopedStatement(). Can the query
> > parameters be ref?
> > 
> Yes to ref, I will change. And I'll update the function name.
> 
> > > 
> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.
> cpp:510
> > > +            RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::%s failed to prepare statement, error message: %{private}s", this, logString.ascii().data(), m_database.lastErrorMsg());
> > 
> > I don't remember the reason for keeping these error messages private. Can
> > they leak something about the user's browsing?
> 
> Good point, this can probably be public.

Some need to be private because they could potentially leak information about bound values, like domains. But just preparing statements is not leaking anything.

[Kate Cheney]

Created attachment 399774 [details]
Patch for landing

[EWS]

Committed r261892: <https://trac.webkit.org/changeset/261892>

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

[Truitt Savell]

It looks like the changes here

broke multiple API tests. tracking here: https://bugs.webkit.org/show_bug.cgi?id=212153

[Kate Cheney]

(In reply to Truitt Savell from comment #19)
> It looks like the changes here
> 
> broke multiple API tests. tracking here:
> https://bugs.webkit.org/show_bug.cgi?id=212153

Weird EWS didn't catch this.

[Chris Dumez]

(In reply to katherine_cheney from comment #20)
> (In reply to Truitt Savell from comment #19)
> > It looks like the changes here
> > 
> > broke multiple API tests. tracking here:
> > https://bugs.webkit.org/show_bug.cgi?id=212153
> 
> Weird EWS didn't catch this.

WK2 EWS runs release, not debug :/

[Kate Cheney]

(In reply to Chris Dumez from comment #21)
> (In reply to katherine_cheney from comment #20)
> > (In reply to Truitt Savell from comment #19)
> > > It looks like the changes here
> > > 
> > > broke multiple API tests. tracking here:
> > > https://bugs.webkit.org/show_bug.cgi?id=212153
> > 
> > Weird EWS didn't catch this.
> 
> WK2 EWS runs release, not debug :/

:(