Bug 211929 - ITP database should finalize all prepared statements before closing
Summary: ITP database should finalize all prepared statements before closing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-14 15:57 PDT by katherine_cheney
Modified: 2020-05-20 10:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.27 KB, patch)
2020-05-14 17:13 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (94.20 KB, patch)
2020-05-18 09:10 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (79.86 KB, patch)
2020-05-18 17:11 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (79.83 KB, patch)
2020-05-19 11:13 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (79.86 KB, patch)
2020-05-19 13:09 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (79.97 KB, patch)
2020-05-19 15:35 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2020-05-14 15:57:41 PDT
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).
Comment 1 katherine_cheney 2020-05-14 15:59:51 PDT
<rdar://problem/63246945>
Comment 2 katherine_cheney 2020-05-14 17:13:18 PDT
Created attachment 399430 [details]
Patch
Comment 3 Sihui Liu 2020-05-15 11:27:10 PDT
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.
Comment 4 katherine_cheney 2020-05-18 09:10:02 PDT
Created attachment 399649 [details]
Patch
Comment 5 katherine_cheney 2020-05-18 09:10:49 PDT
(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.
Comment 6 John Wilander 2020-05-18 15:40:08 PDT
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;
  }
}
Comment 7 katherine_cheney 2020-05-18 17:11:04 PDT
Created attachment 399685 [details]
Patch
Comment 8 katherine_cheney 2020-05-18 17:11:49 PDT
(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!
Comment 9 Sihui Liu 2020-05-19 09:18:47 PDT
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().
Comment 10 katherine_cheney 2020-05-19 09:28:09 PDT
(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.
Comment 11 katherine_cheney 2020-05-19 11:13:44 PDT
Created attachment 399751 [details]
Patch
Comment 12 katherine_cheney 2020-05-19 12:51:48 PDT
wincairo does not seem to like SQLiteStatementAutoResetScope
Comment 13 katherine_cheney 2020-05-19 13:09:29 PDT
Created attachment 399762 [details]
Patch
Comment 14 John Wilander 2020-05-19 14:58:36 PDT
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?
Comment 15 katherine_cheney 2020-05-19 15:18:21 PDT
(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.
Comment 16 katherine_cheney 2020-05-19 15:20:07 PDT
(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.
Comment 17 katherine_cheney 2020-05-19 15:35:36 PDT
Created attachment 399774 [details]
Patch for landing
Comment 18 EWS 2020-05-19 15:55:53 PDT
Committed r261892: <https://trac.webkit.org/changeset/261892>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399774 [details].
Comment 19 Truitt Savell 2020-05-20 10:12:18 PDT
It looks like the changes here

broke multiple API tests. tracking here: https://bugs.webkit.org/show_bug.cgi?id=212153
Comment 20 katherine_cheney 2020-05-20 10:17:58 PDT
(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.
Comment 21 Chris Dumez 2020-05-20 10:34:29 PDT
(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 :/
Comment 22 katherine_cheney 2020-05-20 10:35:16 PDT
(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 :/

:(