Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add error handling to contrainer removal #3424

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Add error handling to contrainer removal #3424

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
836bcb2
Add error handling to contrainer removal
bwaresiak Oct 10, 2024
264865f
Update async code
bwaresiak Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions Core/WebCacheManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,28 @@ 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 {
DispatchQueue.main.asyncAfter(deadline: .now() + 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried about calling this on the main thread. This function doesn't need to. It will call back to the main thread, but the API suggests it can be called from any thread. Also, I think it'd be better if this was doing as a task rather than DispatchQueue, e.g.

        Task {
            try? await Task.sleep(interval: 3.0)
            for uuid in leftoverContainerIDs {
                try? await WKWebsiteDataStore.remove(forIdentifier: uuid)
            }
        }

That said, I'm going to approve because it does work so up to you if you want to tweak it.

for uuid in leftoverContainerIDs {
WKWebsiteDataStore.remove(forIdentifier: uuid, completionHandler: { _ in })
}
}
}
}

}
Expand All @@ -113,18 +127,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 +156,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
Loading