Bug 215813 – syntax error in "DELETE FROM StorageAccessUnderTopFrameDomains"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

This SQLite query error has been appearing in some logging.

[Radar WebKit Bug Importer]

<rdar://problem/67743521>

[Kate Cheney]

Created attachment 407202 [details]
Patch

[Brent Fulgham]

Comment on attachment 407202 [details]
Patch

Looks good! r=me.

Do we have a test for User Interaction expiration? (seems like maybe not?)

[Kate Cheney]

(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.

[EWS]

Found 1 new test failure: http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-database.html

[Kate Cheney]

Created attachment 407336 [details]
Patch

[Kate Cheney]

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.

[John Wilander]

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.

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 407410 [details]
Patch for landing

[Kate Cheney]

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.

[EWS]

Committed r266237: <https://trac.webkit.org/changeset/266237>

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