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).
<rdar://problem/63246945>
Created attachment 399430 [details] Patch
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.
Created attachment 399649 [details] Patch
(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 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; } }
Created attachment 399685 [details] Patch
(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 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().
(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.
Created attachment 399751 [details] Patch
wincairo does not seem to like SQLiteStatementAutoResetScope
Created attachment 399762 [details] Patch
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?
(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.
(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.
Created attachment 399774 [details] Patch for landing
Committed r261892: <https://trac.webkit.org/changeset/261892> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399774 [details].
It looks like the changes here broke multiple API tests. tracking here: https://bugs.webkit.org/show_bug.cgi?id=212153
(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.
(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 :/
(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 :/ :(