Bug 215813 - syntax error in "DELETE FROM StorageAccessUnderTopFrameDomains"
Summary: syntax error in "DELETE FROM StorageAccessUnderTopFrameDomains"
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: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-25 09:47 PDT by Kate Cheney
Modified: 2020-08-27 09:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2020-08-25 10:00 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2020-08-26 14:00 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (8.45 KB, patch)
2020-08-27 09:19 PDT, Kate 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 Kate Cheney 2020-08-25 09:47:58 PDT
This SQLite query error has been appearing in some logging.
Comment 1 Radar WebKit Bug Importer 2020-08-25 09:58:23 PDT
<rdar://problem/67743521>
Comment 2 Kate Cheney 2020-08-25 10:00:32 PDT
Created attachment 407202 [details]
Patch
Comment 3 Brent Fulgham 2020-08-25 10:14:50 PDT
Comment on attachment 407202 [details]
Patch

Looks good! r=me.

Do we have a test for User Interaction expiration? (seems like maybe not?)
Comment 4 Kate Cheney 2020-08-25 10:18:39 PDT
(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.
Comment 5 EWS 2020-08-25 12:24:13 PDT
Found 1 new test failure: http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-database.html
Comment 6 Kate Cheney 2020-08-26 14:00:12 PDT
Created attachment 407336 [details]
Patch
Comment 7 Kate Cheney 2020-08-26 14:01:45 PDT
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 8 John Wilander 2020-08-26 17:31:50 PDT
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.
Comment 9 Kate Cheney 2020-08-26 17:48:10 PDT
(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.
Comment 10 Kate Cheney 2020-08-27 09:19:29 PDT
Created attachment 407410 [details]
Patch for landing
Comment 11 Kate Cheney 2020-08-27 09:19:55 PDT
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.
Comment 12 EWS 2020-08-27 09:47:03 PDT
Committed r266237: <https://trac.webkit.org/changeset/266237>

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