This SQLite query error has been appearing in some logging.
<rdar://problem/67743521>
Created attachment 407202 [details] Patch
Comment on attachment 407202 [details] Patch Looks good! r=me. Do we have a test for User Interaction expiration? (seems like maybe not?)
(In reply to Brent Fulgham from comment #3) > Comment on attachment 407202 [details] > Patch > > Looks good! r=me. > Thanks! > Do we have a test for User Interaction expiration? (seems like maybe not?) It seems like we don't have one which checks that storage access has been deleted. I filed a radar to add one: rdar://67744552.
Found 1 new test failure: http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-database.html
Created attachment 407336 [details] Patch
Looks like this error is occurring in code that should probably just be removed. Fixing the query resulted in incorrect behavior that was previously unnoticed because of the error causing an early return.
Comment on attachment 407336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407336&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:-2576 > - clearExpiredUserInteractions(); Since we will no longer clear expired user interactions here, won't the local variable oldestUserInteraction potentially get older than intended timestamps from mostRecentUserInteractionTime and change the if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) logic? Because oldestUserInteraction is updated before the call to shouldRemoveAllWebsiteDataFor() which in turn calls hasHadUnexpiredRecentUserInteraction(). Since the existing code was broken, I don't think we're making an actual change by removing the call to clearExpiredUserInteractions(), but we should acknowledge it in the change log at minimum. We could potentially move the oldestUserInteraction update to later in the loop so that user interaction timestamps have been clear before.
(In reply to John Wilander from comment #8) > Comment on attachment 407336 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407336&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:-2576 > > - clearExpiredUserInteractions(); > > Since we will no longer clear expired user interactions here, won't the > local variable oldestUserInteraction potentially get older than intended > timestamps from mostRecentUserInteractionTime and change the if > (!parameters().isRunningTest && now - oldestUserInteraction < > parameters().minimumTimeBetweenDataRecordsRemoval) logic? Because > oldestUserInteraction is updated before the call to > shouldRemoveAllWebsiteDataFor() which in turn calls > hasHadUnexpiredRecentUserInteraction(). Since the existing code was broken, > I don't think we're making an actual change by removing the call to > clearExpiredUserInteractions(), but we should acknowledge it in the change > log at minimum. We could potentially move the oldestUserInteraction update > to later in the loop so that user interaction timestamps have been clear > before. Isn't the point of that local variable to take into account that expired statistics might need a little more time for users to interact with a website before removing non-cookie website data? So shouldn't we include the expired statistics in that calculation? Potentially I am misunderstanding.
Created attachment 407410 [details] Patch for landing
Landing this to fix the SQLite error and because we know the code isn’t doing anything right now. I added a comment in the ChangeLog explaining why we should not clear expired statistics before setting oldestUserInteraction, it would mean that now - oldestUserInteraction would almost always be equal to now, because clearing expired statistics sets the mostRecentUserInteraction time to 0, so we would always clear non-cookie website data without giving the extra time for user interaction.
Committed r266237: <https://trac.webkit.org/changeset/266237> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407410 [details].