Skip to content

Conversation

@sysrqb
Copy link
Contributor

@sysrqb sysrqb commented May 30, 2025

772ed3b

[Experimental] Further relax general deletion of script-written website data to 400 days
https://bugs.webkit.org/show_bug.cgi?id=293793
rdar://152095900

Reviewed by NOBODY (OOPS!).

274398@main introduced a "long" DataRemovalFrequency that was set at 30
operational days. This patch extends that lifetime to 400 operational days.
This roughly aligns web storage with the proposed upper-limit on cookie age [0]
(although the cookie age is strictly based on calendar days). We may change
this behavior in the future, again.

This patch fixes some flakey tests by giving
ResourceLoadStatisticsStore::processStatisticsAndDataRecords a completion
handler that is called when the process completes. It seems like our thread
hopping performance has regressed a bit since these tests were written. This
patch also adds a new test that verifies the state of the website data and
database after 399 days.

The code changes make explicit how each time period is used:
  - deletion of prevalent domain data (30 operation days)
  - deletion of script-written website data when link decoration filtering is disabled (7 operational days)
  - deletion of script-written website data when link decoration filtering is enabled (400 operational days)

[0] https://httpwg.org/http-extensions/draft-ietf-httpbis-rfc6265bis.html#name-cookie-lifetime-limits

* LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-website-data-removed.html:
* LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-before-long-deletion-expected.txt: Copied from LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt.
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-before-long-deletion.html: Copied from LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion.html.
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration-expected.txt:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration.html:
* LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt:
* LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration-expected.txt:
* Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:
(WebKit::ResourceLoadStatisticsStore::processStatisticsAndDataRecords):
(WebKit::ResourceLoadStatisticsStore::scheduleStatisticsProcessingRequestIfNecessary):
(WebKit::ResourceLoadStatisticsStore::resetParametersToDefaultValues):
(WebKit::ResourceLoadStatisticsStore::grantStorageAccess):
(WebKit::ResourceLoadStatisticsStore::grantStorageAccessInternal):
(WebKit::ResourceLoadStatisticsStore::logUserInteraction):
(WebKit::ResourceLoadStatisticsStore::areAllUnpartitionedThirdPartyCookiesBlockedUnder):
(WebKit::ResourceLoadStatisticsStore::hasHadRecentWebPushInteraction const):
(WebKit::ResourceLoadStatisticsStore::hasHadUnexpiredRecentUserInteraction):
(WebKit::ResourceLoadStatisticsStore::shouldRemoveAllWebsiteDataFor):
(WebKit::ResourceLoadStatisticsStore::shouldRemoveAllButCookiesFor):
(WebKit::ResourceLoadStatisticsStore::updateOperatingDatesParameters):
(WebKit::ResourceLoadStatisticsStore::includeTodayAsOperatingDateIfNecessary):
(WebKit::ResourceLoadStatisticsStore::hasStatisticsExpired const):
* Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:
* Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::scheduleStatisticsAndDataRecordsProcessing):
(WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
(WebKit::WebResourceLoadStatisticsStore::hasHadUserInteraction):

772ed3b

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ❌ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@sysrqb sysrqb self-assigned this May 30, 2025
@sysrqb sysrqb added the Website Storage Bugs related to storage APIs, including IndexedDB and localStorage label May 30, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 30, 2025
@sysrqb sysrqb removed the merging-blocked Applied to prevent a change from being merged label May 30, 2025
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from e18123d to 3758ba8 Compare May 30, 2025 14:43
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from 3758ba8 to db7e027 Compare May 30, 2025 15:36
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from db7e027 to c4a496a Compare May 30, 2025 15:38
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from c4a496a to c194314 Compare May 30, 2025 16:08
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 30, 2025
@sysrqb sysrqb removed the merging-blocked Applied to prevent a change from being merged label Jun 1, 2025
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from c194314 to 49e3487 Compare June 1, 2025 04:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 1, 2025
@sysrqb sysrqb removed the merging-blocked Applied to prevent a change from being merged label Jun 1, 2025
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from 49e3487 to af0e0ea Compare June 1, 2025 17:04
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 1, 2025
@sysrqb sysrqb removed the merging-blocked Applied to prevent a change from being merged label Jun 2, 2025
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from af0e0ea to acb2c77 Compare June 2, 2025 16:50
@sysrqb sysrqb marked this pull request as ready for review June 2, 2025 16:50
@sysrqb sysrqb requested a review from cdumez as a code owner June 2, 2025 16:50
removeDataRecords([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler)] () mutable {
ASSERT(!RunLoop::isMain());
RefPtr protectedThis = weakThis.get();
if (!protectedThis)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not calling the completion handler here

if (RefPtr statisticsStore = m_statisticsStore) {
statisticsStore->processStatisticsAndDataRecords([weakThis = ThreadSafeWeakPtr { *this }, completionHandler = WTFMove(completionHandler)] () mutable {
if (RefPtr store = weakThis.get())
store->postTaskReply(WTFMove(completionHandler));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, if this is gone the completion handler will never be called

}

void ResourceLoadStatisticsStore::processStatisticsAndDataRecords()
void ResourceLoadStatisticsStore::processStatisticsAndDataRecords(CompletionHandler<void()>&& completionHandler)
Copy link
Member

@charliewolfe charliewolfe Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to add this completion handler in this change? It's not clear to me why this is related to extending the days on script-written data deletion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are flakey. I'll add this into the commit message. If we don't move the completion handler, then many of the resourceLoadStatistics tests will call processStatisticsAndDataRecords multiple times, and those calls will overlap. This invalidates some of the test results because the test expectations depend on the state of the database at specific times. The current code runs processStatisticsAndDataRecords asynchronously, so there's no way to verify the state of website data at specific time points.

@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from acb2c77 to 96ccdc3 Compare June 7, 2025 03:18
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from 96ccdc3 to 3fb2cf8 Compare June 13, 2025 13:26
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 13, 2025
…te data to 400 days

https://bugs.webkit.org/show_bug.cgi?id=293793
rdar://152095900

Reviewed by NOBODY (OOPS!).

274398@main introduced a "long" DataRemovalFrequency that was set at 30
operational days. This patch extends that lifetime to 400 operational days.
This roughly aligns web storage with the proposed upper-limit on cookie age [0]
(although the cookie age is strictly based on calendar days). We may change
this behavior in the future, again.

This patch fixes some flakey tests by giving
ResourceLoadStatisticsStore::processStatisticsAndDataRecords a completion
handler that is called when the process completes. It seems like our thread
hopping performance has regressed a bit since these tests were written. This
patch also adds a new test that verifies the state of the website data and
database after 399 days.

The code changes make explicit how each time period is used:
  - deletion of prevalent domain data (30 operation days)
  - deletion of script-written website data when link decoration filtering is disabled (7 operational days)
  - deletion of script-written website data when link decoration filtering is enabled (400 operational days)

[0] https://httpwg.org/http-extensions/draft-ietf-httpbis-rfc6265bis.html#name-cookie-lifetime-limits

* LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-website-data-removed.html:
* LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-before-long-deletion-expected.txt: Copied from LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt.
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-before-long-deletion.html: Copied from LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion.html.
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration-expected.txt:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration.html:
* LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-filtered-link-decoration-after-long-deletion-expected.txt:
* LayoutTests/platform/glib/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-without-link-decoration-expected.txt:
* Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:
(WebKit::ResourceLoadStatisticsStore::processStatisticsAndDataRecords):
(WebKit::ResourceLoadStatisticsStore::scheduleStatisticsProcessingRequestIfNecessary):
(WebKit::ResourceLoadStatisticsStore::resetParametersToDefaultValues):
(WebKit::ResourceLoadStatisticsStore::grantStorageAccess):
(WebKit::ResourceLoadStatisticsStore::grantStorageAccessInternal):
(WebKit::ResourceLoadStatisticsStore::logUserInteraction):
(WebKit::ResourceLoadStatisticsStore::areAllUnpartitionedThirdPartyCookiesBlockedUnder):
(WebKit::ResourceLoadStatisticsStore::hasHadRecentWebPushInteraction const):
(WebKit::ResourceLoadStatisticsStore::hasHadUnexpiredRecentUserInteraction):
(WebKit::ResourceLoadStatisticsStore::shouldRemoveAllWebsiteDataFor):
(WebKit::ResourceLoadStatisticsStore::shouldRemoveAllButCookiesFor):
(WebKit::ResourceLoadStatisticsStore::updateOperatingDatesParameters):
(WebKit::ResourceLoadStatisticsStore::includeTodayAsOperatingDateIfNecessary):
(WebKit::ResourceLoadStatisticsStore::hasStatisticsExpired const):
* Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:
* Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::scheduleStatisticsAndDataRecordsProcessing):
(WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
(WebKit::WebResourceLoadStatisticsStore::hasHadUserInteraction):
@sysrqb sysrqb removed the merging-blocked Applied to prevent a change from being merged label Jun 13, 2025
@sysrqb sysrqb force-pushed the eng/Experimental-Further-relax-general-deletion-of-script-written-website-data-to-400-days branch from 3fb2cf8 to 772ed3b Compare June 13, 2025 14:33
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 772ed3b. Live statuses available at the PR page, #46107

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merging-blocked Applied to prevent a change from being merged Website Storage Bugs related to storage APIs, including IndexedDB and localStorage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants