Skip to content

Commit

Permalink
Add error handling to contrainer removal (#3424)
Browse files Browse the repository at this point in the history
<!--
Note: This checklist is a reminder of our shared engineering
expectations. Feel free to change it, although assigning a GitHub
reviewer and the items in bold are required.

⚠️ If you're an external contributor, please file an issue first before
working on a PR, as we can't guarantee that we will accept your changes
if they haven't been discussed ahead of time. Thanks!
-->

Task/Issue URL:
https://app.asana.com/0/856498667320406/1207976962313181/f

**Description**:

Make sure the containers created recently or the ones that fail to be
removed are given another chance to be cleaned.

<!--
If at any point it isn't actively being worked on/ready for
review/otherwise moving forward strongly consider closing it (or not
opening it in the first place). If you decide not to close it, use Draft
PR while work is still in progress or use `DO NOT MERGE` label to
clarify the PRs state and comment with more information.
-->

**Steps to test this PR**:

Open the app, navigate to site that sets cookies, refresh, press fire
button.

Smoke test data clearing, auto clear - both on app restart and time
based one.
Test fireproofing on usage and after restart.

<!--
Before submitting a PR, please ensure you have tested the combinations
you expect the reviewer to test, then delete configurations you *know*
do not need explicit testing.

Using a simulator where a physical device is unavailable is acceptable.
-->

**Definition of Done (Internal Only)**:

* [ ] Does this PR satisfy our [Definition of
Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)?

**OS Testing**:

* [ ] iOS 15
* [ ] iOS 16
* [ ] iOS 17

**Theme Testing**:

* [ ] Light theme
* [ ] Dark theme

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
  • Loading branch information
bwaresiak authored Oct 14, 2024
1 parent 09e542c commit c9a486b
Showing 1 changed file with 34 additions and 6 deletions.
40 changes: 34 additions & 6 deletions Core/WebCacheManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,29 @@ public class WebCacheManager {
dataStoreIdManager: DataStoreIdManaging = DataStoreIdManager.shared) async {

var cookiesToUpdate = [HTTPCookie]()
var leftoverContainerIDs = [UUID]()
if #available(iOS 17, *) {
cookiesToUpdate += await containerBasedClearing(storeIdManager: dataStoreIdManager) ?? []
let result = await containerBasedClearing(storeIdManager: dataStoreIdManager)
cookiesToUpdate += result.cookies
leftoverContainerIDs = result.leftoverContainerIDs
}

// Perform legacy clearing to migrate to new container
cookiesToUpdate += await legacyDataClearing() ?? []

cookieStorage.updateCookies(cookiesToUpdate, keepingPreservedLogins: logins)

// Attempt to clean up leftover stores again after a delay
// This should not be a problem as these containers are not supposed to be used anymore.
// If this fails, we are going to still clean them next time as WebKit keeps track of all stores for us.
if #available(iOS 17, *), !leftoverContainerIDs.isEmpty {
Task {
try? await Task.sleep(for: .seconds(3))
for uuid in leftoverContainerIDs {
try? await WKWebsiteDataStore.remove(forIdentifier: uuid)
}
}
}
}

}
Expand All @@ -113,18 +128,27 @@ extension WebCacheManager {
}

@available(iOS 17, *)
private func containerBasedClearing(storeIdManager: DataStoreIdManaging) async -> [HTTPCookie]? {
private func containerBasedClearing(storeIdManager: DataStoreIdManaging) async -> (cookies: [HTTPCookie],
leftoverContainerIDs: [UUID]) {
guard let containerId = storeIdManager.currentId else {
storeIdManager.invalidateCurrentIdAndAllocateNew()
return []
return ([], [])
}
storeIdManager.invalidateCurrentIdAndAllocateNew()

var leftoverContainerIDs = [UUID]()

var dataStore: WKWebsiteDataStore? = WKWebsiteDataStore(forIdentifier: containerId)
let cookies = await dataStore?.httpCookieStore.allCookies()
let cookies = await dataStore?.httpCookieStore.allCookies() ?? []
dataStore = nil

var uuids = await WKWebsiteDataStore.allDataStoreIdentifiers

// There may be a timing issue related to fetching current container, so append previous UUID as a precaution
if uuids.firstIndex(of: containerId) == nil {
uuids.append(containerId)
}

if let newContainerID = storeIdManager.currentId,
let newIdIndex = uuids.firstIndex(of: newContainerID) {
assertionFailure("Attempted to cleanup current Data Store")
Expand All @@ -133,11 +157,15 @@ extension WebCacheManager {

let previousLeftOversCount = max(0, uuids.count - 1) // -1 because one store is expected to be cleared
for uuid in uuids {
try? await WKWebsiteDataStore.remove(forIdentifier: uuid)
do {
try await WKWebsiteDataStore.remove(forIdentifier: uuid)
} catch {
leftoverContainerIDs.append(uuid)
}
}
await checkForLeftBehindDataStores(previousLeftOversCount: previousLeftOversCount)

return cookies
return (cookies, leftoverContainerIDs)
}

private func legacyDataClearing() async -> [HTTPCookie]? {
Expand Down

0 comments on commit c9a486b

Please sign in to comment.