Refactor FXIOS-14466 FXIOS-14463 [Swift 6 Migration] Migrate ShareTo and BrowserKit Package to Swift 6 #36
Conversation
…es are enabled in Swift 6.2 by default.
…t does not seem to require @mainactor initialization anymore (and calling `startMockImageServer` on the main thread will causes the addHandler callback to crash for not being called on the main thread).
…currency warnings.
WalkthroughThis pull request upgrades Swift tooling from 5.10 to 6.2 in BrowserKit and adds explicit Swift 6.0 version settings across firefox-ios build configurations. Experimental concurrency flags are removed, while code is updated with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift (1)
97-113: Preservenilaction semantics when wrapping.
wrappedActionis always non-nil, even whenmoreButtonActionis nil. IfActionButtonViewModelusesnilto indicate “no action,” this changes behavior.🔧 Suggested adjustment
- let wrappedAction: (`@MainActor` (UIButton) -> Void)? = { button in - moreButtonAction?(button) - } + let wrappedAction: (`@MainActor` (UIButton) -> Void)? = moreButtonAction.map { action in + { button in action(button) } + }BrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swift (1)
12-36: Re-add expectation/wait for the async download.The first test is missing the async synchronization pattern used consistently in all other tests in this file (lines 40, 73, 104, 131). Without waiting for the callback, assertions inside the completion handler may never execute or race with test completion, causing the test to pass incorrectly.
Suggested fix
guard let mockedURL = try? startMockImageServer(imageData: imageData, forAssetType: assetType) else { XCTFail("Check bundle setup for mock server response data") return } + let exp = expectation(description: "Image download and parse") + let siteDownloader = DefaultSiteImageDownloader() siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in switch result { case .success(let result): XCTAssertEqual(result.originalData, imageData) XCTAssertEqual(result.url, mockedURL) XCTAssertEqual(result.image.size, expectedRasterSize) + exp.fulfill() case .failure(let error): XCTFail("Should not have an error: \(error) \(error.errorDescription ?? "")") + exp.fulfill() } } + + wait(for: [exp], timeout: 2.0)
🤖 Fix all issues with AI agents
In `@BrowserKit/Package.swift`:
- Line 1: Update the Swift toolchain version in the iOS build config and CI to
match BrowserKit's new requirement: change the SWIFT_VERSION setting in
Common.xcconfig (the SWIFT_VERSION = 5.10 entry) to SWIFT_VERSION = 6.2, and
update the CI configuration (bitrise.yml) to pin the Xcode stack to 26.1 or a
compatible Xcode that supports Swift 6.2 so focus-ios and BrowserKit use the
same Swift version.
In
`@BrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swift`:
- Around line 5-6: The static nonisolated(unsafe) generator is removing
concurrency protection for the non-thread-safe ContentBlockerGenerator; remove
the static `generator` declaration and instead call
`ContentBlockerGenerator.factory()` locally inside `main()` where it's used, or
if you truly need a shared instance, mark the type or the static as `@MainActor`
to enforce single-threaded access (update uses of `generator` to the new local
variable or actor-isolated static accordingly).
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift`:
- Around line 38-41: The test closure types for the media-permission handlers
(e.g., the decisionHandler variable used as the requestMediaCapturePermission
callback) are declared as `@Sendable` but the API expects `@escaping` `@MainActor`
(WKPermissionDecision) -> Void; update these closures to be declared as
`@Sendable` `@MainActor` (WKPermissionDecision) -> Void so their concurrency
annotations match the parameter signature (apply the same change to the other
test handlers referenced in the file, e.g., the handlers around the other
media-permission tests).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
BrowserKit/Package.swiftBrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swiftBrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swiftBrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swiftfirefox-ios/Client.xcodeproj/project.pbxprojfirefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift
🧰 Additional context used
🧬 Code graph analysis (1)
BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift (1)
BrowserKit/Sources/WebEngine/WKWebview/WKUIHandler.swift (1)
requestMediaCapturePermission(175-183)
🔇 Additional comments (2)
BrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swift (1)
40-71: LGTM on keeping expectations after removing async/await.
These tests still wait for completion, so the synchronous refactor remains deterministic.Also applies to: 73-103, 104-129, 131-153
firefox-ios/Client.xcodeproj/project.pbxproj (1)
28011-28011: SWIFT_VERSION = 6.0 updated across 30 build configuration entries, not just the 6 ShareTo entries highlighted.The specified lines (28011, 28496, 29786, 30094, 30411, 30941) represent only a portion of the Swift 6.0 migration. The file contains 30 total SWIFT_VERSION = 6.0 entries across multiple targets and configurations. Ensure the review covers the full scope of changes, not just the ShareTo extension configurations. Verify that all intended targets have been updated consistently and that no configurations were accidentally left with Swift 5.0 or an earlier version.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -1,4 +1,4 @@ | |||
| // swift-tools-version: 5.10 | |||
| // swift-tools-version: 6.2 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for Swift/Xcode/toolchain settings in repo configs
rg -n "swift-tools-version|SWIFT_VERSION|Xcode|TOOLCHAIN|swift version" -g "*.{yml,yaml,xcconfig,plist,md}" --max-count=50Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4591
🏁 Script executed:
#!/bin/bash
# Find all Package.swift files
fd -n "Package.swift" --type f
# Look for BrowserKit-specific Swift version settings
rg -n "swift-tools-version|SWIFT_VERSION|swift version" BrowserKit/ --type swift --type yaml --type plist --type xcconfigRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 350
🏁 Script executed:
#!/bin/bash
# Find all Package.swift files
fd "Package.swift" --type f
# Look for BrowserKit-specific Swift version settings
rg -n "swift-tools-version|SWIFT_VERSION|swift version" BrowserKit/ --type swift --type yaml --type xcconfigRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 256
🏁 Script executed:
#!/bin/bash
# Check all Package.swift files for swift-tools-version
echo "=== Root Package.swift ==="
head -5 Package.swift
echo ""
echo "=== BrowserKit/Package.swift ==="
head -5 BrowserKit/Package.swift
echo ""
echo "=== MozillaRustComponents/Package.swift ==="
head -5 MozillaRustComponents/Package.swift
echo ""
echo "=== BlockzillaPackage/Package.swift ==="
head -5 focus-ios/BlockzillaPackage/Package.swift
# Check for BrowserKit-specific xcconfig or build settings
echo ""
echo "=== BrowserKit Config Files ==="
fd "xcconfig|config" BrowserKit/ --type f | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Check if root Package.swift depends on BrowserKit
echo "=== Root Package.swift dependencies ==="
cat Package.swift
echo ""
echo "=== Check git structure to understand BrowserKit role ==="
git ls-tree -r HEAD | grep -E "Package\.swift|BrowserKit" | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 3231
🏁 Script executed:
#!/bin/bash
# Check if firefox-ios project depends on BrowserKit
echo "=== Search for BrowserKit references in firefox-ios ==="
rg -l "BrowserKit" firefox-ios/ --type swift --type yaml --type xcodeproj | head -10
echo ""
echo "=== Check bitrise.yml for Xcode version specs ==="
rg -n "xcode.*version|Xcode.*=|XCODE_VERSION" bitrise.yml -A 3 -B 1Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 2765
🏁 Script executed:
#!/bin/bash
# Check if focus-ios or other apps depend on BrowserKit
echo "=== Search for BrowserKit in focus-ios ==="
rg "BrowserKit" focus-ios/ | head -5
echo ""
echo "=== Check bootstrap.sh and setup scripts ==="
cat bootstrap.sh 2>/dev/null | head -50
echo ""
echo "=== Check for .xcode-version or version files ==="
fd -e ".xcode-version" -e ".xcoderc" -e ".swift-version" --type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 2232
Update firefox-ios Common.xcconfig and CI to enforce Swift 6.2 alignment with BrowserKit.
BrowserKit now requires Swift 6.2, but firefox-ios/Client/Configuration/Common.xcconfig still sets SWIFT_VERSION = 5.10. Since focus-ios depends on BrowserKit, this version mismatch will cause build failures. Update the build config to SWIFT_VERSION = 6.2 and ensure bitrise.yml pins to Xcode 26.1 (or later compatible version) to align with the new requirement.
🤖 Prompt for AI Agents
In `@BrowserKit/Package.swift` at line 1, Update the Swift toolchain version in
the iOS build config and CI to match BrowserKit's new requirement: change the
SWIFT_VERSION setting in Common.xcconfig (the SWIFT_VERSION = 5.10 entry) to
SWIFT_VERSION = 6.2, and update the CI configuration (bitrise.yml) to pin the
Xcode stack to 26.1 or a compatible Xcode that supports Swift 6.2 so focus-ios
and BrowserKit use the same Swift version.
| // FXIOS-14548 ContentBlockerGenerator is not thread safe | ||
| nonisolated(unsafe) static let generator = ContentBlockerGenerator.factory() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Swift 6 nonisolated(unsafe) static properties concurrency
💡 Result:
In Swift 6 strict concurrency, a static var is treated like global shared mutable state. Because it can be read/written from any actor/thread, the compiler diagnoses it as not concurrency-safe unless you make its access provably safe (or you explicitly opt out). This is why you see errors like “static property … is not concurrency-safe because it is nonisolated global shared mutable state”. [2]
What nonisolated(unsafe) does (and when to use it)
nonisolated(unsafe) is an unsafe opt-out: it tells the compiler “don’t enforce actor-isolation / data-race checking for this storage; I’m taking responsibility.” It was introduced specifically to handle cases where the compiler can’t “see” your synchronization (locks/queues/C code, etc.), including global/static variables. [1]
Typical use case: the storage is effectively isolated by some external mechanism (a serial queue, a lock, etc.), but the compiler can’t prove it:
import Dispatch
struct Cache {
static let q = DispatchQueue(label: "cache.q")
nonisolated(unsafe) static var storage: [String: Int] = [:]
static func get(_ k: String) -> Int? { q.sync { storage[k] } }
static func set(_ k: String, _ v: Int) { q.sync { storage[k] = v } }
}Swift.org’s release notes use the same pattern: guard all accesses externally, then mark the static/global as nonisolated(unsafe) to silence the warning. [1]
Preferred fixes (before nonisolated(unsafe))
The compiler (and Swift team guidance) generally expects one of these first: [2]
- Make it immutable:
static let(and the value/type should beSendable). - Isolate it to an actor/global actor (commonly
@MainActor), so accesses are serialized and crossing the boundary requiresawait. - Use
nonisolated(unsafe)only if you’ve already made it safe via external synchronization and just need to tell the compiler. [1][2]
Notes / gotchas
nonisolated(unsafe)removes compile-time protection; you can still have real races—TSAN/exclusivity can still catch bugs at runtime. [1]- If you notice
static varinside anactornot being flagged, that has been reported as a compiler bug; don’t assume it’s safe just because it compiles. [3]
Sources: Swift.org Swift 5.10 release notes (introducing nonisolated(unsafe) and showing static/global usage) [1], Swift Forums guidance on global/static mutable state diagnostics and recommended fixes [2], Stack Overflow discussion noting actor static-var checking issue/bug [3].
🏁 Script executed:
# Find and read the MainContentBlockerGenerator.swift file
find . -name "MainContentBlockerGenerator.swift" -type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 179
🏁 Script executed:
cat -n BrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 596
Avoid nonisolated(unsafe) on a known non-thread-safe singleton.
The ContentBlockerGenerator is explicitly non-thread-safe (per the comment), but nonisolated(unsafe) removes Swift 6 concurrency checks on this static instance, allowing potential unsafe cross-actor access without compile-time protection. Since the generator is only used in main(), move the instantiation locally instead:
Suggested fix
public static func main() {
- generator.generateLists()
+ let generator = ContentBlockerGenerator.factory()
+ generator.generateLists()
}Remove the static generator and its nonisolated(unsafe) annotation.
Alternatively, isolate the struct to @MainActor if the generator must be shared across functions.
🤖 Prompt for AI Agents
In
`@BrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swift`
around lines 5 - 6, The static nonisolated(unsafe) generator is removing
concurrency protection for the non-thread-safe ContentBlockerGenerator; remove
the static `generator` declaration and instead call
`ContentBlockerGenerator.factory()` locally inside `main()` where it's used, or
if you truly need a shared instance, mark the type or the static as `@MainActor`
to enforce single-threaded access (update uses of `generator` to the new local
variable or actor-isolated static accordingly).
| let decisionHandler: @Sendable (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in | ||
| XCTAssertEqual(decision, .prompt) | ||
| expectation.fulfill() | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "WKUIHandlerTests.swift" | head -5Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 146
🏁 Script executed:
# Read the file to understand the context
cat -n BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift | head -100Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4187
🏁 Script executed:
# Get line count to understand file size
wc -l BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 148
🏁 Script executed:
find . -type f -name "*.swift" | xargs rg -A 3 "func requestMediaCapturePermission" | head -30Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 12470
🏁 Script executed:
# Also search for the WKUIHandler or similar class that defines this method
rg -l "requestMediaCapturePermission" --type swift | head -10Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 884
🏁 Script executed:
cat -n BrowserKit/Sources/WebEngine/WKWebview/WKUIHandler.swift | sed -n '1,100p'Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4441
🏁 Script executed:
# Find the exact requestMediaCapturePermission method
rg -B 2 -A 10 "func requestMediaCapturePermission" BrowserKit/Sources/WebEngine/WKWebview/WKUIHandler.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 412
🌐 Web query:
Swift 6 @sendableclosure compatibility with@mainactor parameter annotation
💡 Result:
In Swift 6, @MainActor and @Sendable are independent parts of a function/closure type, so “@MainActor closure” is not automatically treated as “@Sendable closure” (at least in Swift versions discussed during the Swift 5.10 → Swift 6 transition). This is why you can hit warnings/errors when an API wants a @Sendable closure but you have (or store) a @MainActor-isolated one. (forums.swift.org)
What to do in Swift 6 code
1) If the closure must be main-actor isolated and cross concurrency domains, type it as both
Use a function type like:
typealias Work = `@Sendable` `@MainActor` () -> Void
// or: let work: `@Sendable` `@MainActor` () -> VoidThis commonly resolves:
- storing the closure inside a
Sendabletype, and/or - passing it to an API that requires
@Sendable. (forums.swift.org)
2) If you control the API, accept the combined type
Prefer:
func register(_ handler: `@Sendable` `@MainActor` () -> Void) { ... }rather than only @MainActor () -> Void, if callers will pass the handler across tasks/threads.
If you’re bridging older APIs (common with GCD / UIKit / etc.)
Apple and others have used a parameter attribute like @_unsafeSendable to “treat this closure parameter as @Sendable in Swift 6 (and adopted Swift 5 code) without ABI impact”, mainly for retrofitting existing APIs. (forums.swift.org)
Why this happens (the key compatibility point)
Function types are not automatically interchangeable here: adding/removing @Sendable or adding/removing @MainActor changes the type, so you often need to spell the function type that matches the API (@Sendable @mainactor ...) rather than assuming one implies the other. (forums.swift.org)
Citations:
- 1: https://forums.swift.org/t/implicitly-sendable-closures/69950?utm_source=openai
- 2: https://forums.swift.org/t/strict-concurrency-false-positives/70803?utm_source=openai
- 3: https://forums.swift.org/t/concurrency-in-swift-5-and-6/49337?utm_source=openai
- 4: https://forums.swift.org/t/implicitly-sendable-closures/69950?utm_source=openai
🌐 Web query:
Swift 6 passing @sendableclosure to@mainactor parameter
💡 Result:
In Swift 6, these are different function types, so you can’t pass one where the other is expected:
@Sendable () -> Void= may run concurrently on any executor, so captures must be sendable.@MainActor () -> Void= must run on the MainActor (global-actor isolated).
So this fails:
func takesMain(_ f: `@escaping` `@MainActor` () -> Void) {}
let f: `@Sendable` () -> Void = { /* ... */ }
takesMain(f) // cannot convertFix 1 (usually best): make the API accept what you actually want
If the closure is both main-actor isolated and safely transferable, declare it as:
func takesMain(_ f: `@escaping` `@Sendable` `@MainActor` () -> Void) {}This is a common Swift 6 migration pattern (and is what Swift Evolution work around global-actor usability aims to make smoother). (forums.swift.org)
Fix 2: adapt a @Sendable closure into a @MainActor closure (hop to main)
If you only have a @Sendable closure but need to call it from a main-actor callback site:
func takesMain(_ f: `@escaping` `@MainActor` () -> Void) {}
let f: `@Sendable` () -> Void = { /* thread-safe work */ }
takesMain {
Task { `@MainActor` in
f()
}
}Fix 3 (sync, when you know you’re already on MainActor): MainActor.assumeIsolated
When you’re bridging APIs and you know the call happens on main (but the type system doesn’t express it), you can wrap the call:
typealias Completion = `@Sendable` `@MainActor` () -> Void
// later, inside a callback you know is on main:
MainActor.assumeIsolated {
completion()
}This pattern is used specifically to handle “the docs guarantee main-thread, but the signature doesn’t” situations. (jessesquires.com)
Key takeaway
If a closure will be stored/passed across isolation domains, prefer @Sendable @mainactor () -> Void for “must run on main and is safe to send”. Swift Forums discussions confirm that today you often must spell both attributes explicitly. (forums.swift.org)
Citations:
- 1: https://forums.swift.org/t/warning-when-assigning-a-closure-to-an-immutable-property-on-a-mainactor-isolated-struct-from-a-non-isolated-initializer/70588?utm_source=openai
- 2: https://www.jessesquires.com/blog/2024/06/05/swift-concurrency-non-sendable-closures/?utm_source=openai
- 3: https://forums.swift.org/t/warning-when-assigning-a-closure-to-an-immutable-property-on-a-mainactor-isolated-struct-from-a-non-isolated-initializer/70588?utm_source=openai
Fix closure type to match parameter signature: use @Sendable @MainActor`` instead of @Sendable only.
The requestMediaCapturePermission parameter expects @escaping @mainactor (WKPermissionDecision) -> Void, but the test handlers are typed as @Sendable (WKPermissionDecision) -> Void. In Swift 6, @Sendable and @MainActor are independent annotations and not interchangeable—you cannot pass a @Sendable closure where @MainActor is required. These closures must be declared as @Sendable @MainActor`` to match the parameter type and satisfy strict concurrency requirements.
Also applies to: 50-54, 65-69
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift` around lines 38 - 41,
The test closure types for the media-permission handlers (e.g., the
decisionHandler variable used as the requestMediaCapturePermission callback) are
declared as `@Sendable` but the API expects `@escaping` `@MainActor`
(WKPermissionDecision) -> Void; update these closures to be declared as
`@Sendable` `@MainActor` (WKPermissionDecision) -> Void so their concurrency
annotations match the parameter signature (apply the same change to the other
test handlers referenced in the file, e.g., the handlers around the other
media-permission tests).
Benchmark PR from agentic-review-benchmarks#10
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.