Bug 204779 - Resource Load Statistics (experimental): Delete non-cookie website data after 7 days of no user interaction
Summary: Resource Load Statistics (experimental): Delete non-cookie website data after...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-02 18:53 PST by John Wilander
Modified: 2019-12-04 13:46 PST (History)
5 users (show)

See Also:


Attachments
Patch (103.89 KB, patch)
2019-12-02 19:23 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-12-02 18:53:15 PST
Trackers are continuing to move cross-site tracking IDs into first-party storage. We should age out script-writable non-cookie website data in alignment with the 7-day cap on client-side cookies.
Comment 1 Radar WebKit Bug Importer 2019-12-02 18:53:43 PST
<rdar://problem/57578989>
Comment 2 John Wilander 2019-12-02 19:23:50 PST
Created attachment 384681 [details]
Patch
Comment 3 John Wilander 2019-12-03 05:59:55 PST
The Win test failures are all over the place and seem unrelated.
Comment 4 Alex Christensen 2019-12-03 15:49:26 PST
Comment on attachment 384681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384681&action=review

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:488
> +    postTask([this, mode, completionHandler = WTFMove(completionHandler)]() mutable {

Not for this patch, but I don't like it how in this one file it's ok to use this without protecting it or checking a weak pointer, but everywhere else we need to be careful.  Let's make it so postTask doesn't implicitly protect this and require all callers to protect this.
Comment 5 Chris Dumez 2019-12-03 15:51:07 PST
(In reply to Alex Christensen from comment #4)
> Comment on attachment 384681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384681&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:488
> > +    postTask([this, mode, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Not for this patch, but I don't like it how in this one file it's ok to use
> this without protecting it or checking a weak pointer, but everywhere else
> we need to be careful.  Let's make it so postTask doesn't implicitly protect
> this and require all callers to protect this.

Previously it was like that and people kept forgetting to protect this and we had tons of similar crashes. As a result, I made postTask() protect this. Worse case scenario, the caller protects again, which is a bit inefficient but at least does not cause crashes.
Comment 6 John Wilander 2019-12-03 17:57:50 PST
Thanks for the review!
Comment 7 WebKit Commit Bot 2019-12-03 18:41:58 PST
Comment on attachment 384681 [details]
Patch

Clearing flags on attachment: 384681

Committed r253082: <https://trac.webkit.org/changeset/253082>
Comment 8 WebKit Commit Bot 2019-12-03 18:41:59 PST
All reviewed patches have been landed.  Closing bug.