CrashTracer: com.apple.WebKit.Networking at WebKit: WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource

kcheney1 · kcheney1 · commit c40082b7dac4 · 2021-02-05T17:11:37.000Z
https://bugs.webkit.org/show_bug.cgi?id=221432 <rdar://problem/67069819> Reviewed by John Wilander. We are seeing crashes in ResourceLoadStatisticsDatabaseStore::setPrevalentResource as a result of trying to use a nullopt domainID value. In theory this should never be WTF::nullopt but is because of a failing SQLite query in ResourceLoadStatisticsDatabaseStore::domainID which reports the error "not an error". To fix this we should check the domain ID and return early with a debug assert in setPrevalentResource() if it is WTF::nullopt to avoid a crash. Additionally, we should add more information to the logging statement, specifically the SQLite statement string, to try and debug further. * NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp: (WebKit::ResourceLoadStatisticsDatabaseStore::domainID const): (WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource): Canonical link: https://commits.webkit.org/233742@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@272418 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,25 @@
+2021-02-05  Kate Cheney  <katherine_cheney@apple.com>
+
+        CrashTracer: com.apple.WebKit.Networking at WebKit: WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource
+        https://bugs.webkit.org/show_bug.cgi?id=221432
+        <rdar://problem/67069819>
+
+        Reviewed by John Wilander.
+
+        We are seeing crashes in ResourceLoadStatisticsDatabaseStore::setPrevalentResource
+        as a result of trying to use a nullopt domainID value. In theory this should
+        never be WTF::nullopt but is because of a failing SQLite query in
+        ResourceLoadStatisticsDatabaseStore::domainID which reports the error "not an error".
+
+        To fix this we should check the domain ID and return early with a
+        debug assert in setPrevalentResource() if it is WTF::nullopt to avoid
+        a crash. Additionally, we should add more information to the logging
+        statement, specifically the SQLite statement string, to try and debug further.
+
+        * NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:
+        (WebKit::ResourceLoadStatisticsDatabaseStore::domainID const):
+        (WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource):
+
 2021-02-05  Sihui Liu  <sihui_liu@appe.com>
 
         SpeechRecognitionPermissionManager should not handle requests that are already cancelled
diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
@@ -796,7 +796,7 @@ Optional<unsigned> ResourceLoadStatisticsDatabaseStore::domainID(const Registrab
     auto scopedStatement = this->scopedStatement(m_domainIDFromStringStatement, domainIDFromStringQuery, "domainID"_s);
     if (!scopedStatement
         || scopedStatement->bindText(1, domain.string()) != SQLITE_OK) {
-        RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p - ResourceLoadStatisticsDatabaseStore::domainIDFromString failed, error message: %{private}s", this, m_database.lastErrorMsg());
+        RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p - ResourceLoadStatisticsDatabaseStore::domainIDFromString failed. Statement: %s. Error message: %{private}s", this, scopedStatement->query().utf8().data(), m_database.lastErrorMsg());
         ASSERT_NOT_REACHED();
         return WTF::nullopt;
     }
@@ -1670,6 +1670,13 @@ void ResourceLoadStatisticsDatabaseStore::setPrevalentResource(const Registrable
     if (shouldSkip(domain))
         return;
 
+    auto registrableDomainID = domainID(domain);
+    if (!registrableDomainID) {
+        RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p - ResourceLoadStatisticsDatabaseStore::setPrevalentResource domainID should exist but was not found.", this);
+        ASSERT_NOT_REACHED();
+        return;
+    }
+
     auto scopedUpdatePrevalentResourceStatement = this->scopedStatement(m_updatePrevalentResourceStatement, updatePrevalentResourceQuery, "setPrevalentResource"_s);
     if (!scopedUpdatePrevalentResourceStatement
         || scopedUpdatePrevalentResourceStatement->bindInt(1, 1) != SQLITE_OK
@@ -1693,7 +1700,7 @@ void ResourceLoadStatisticsDatabaseStore::setPrevalentResource(const Registrable
     }
 
     StdSet<unsigned> nonPrevalentRedirectionSources;
-    recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain(domainID(domain).value(), nonPrevalentRedirectionSources, 0);
+    recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain(*registrableDomainID, nonPrevalentRedirectionSources, 0);
     setDomainsAsPrevalent(WTFMove(nonPrevalentRedirectionSources));
 }
 
