Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 0 additions & 25 deletions BrowserKit/Tests/WebEngineTests/Mock/MockWKFrameInfo.swift

This file was deleted.

31 changes: 0 additions & 31 deletions BrowserKit/Tests/WebEngineTests/Mock/MockWKSecurityOrigin.swift

This file was deleted.

50 changes: 50 additions & 0 deletions BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import WebKit
import XCTest

/// Provides real `WKFrameInfo` and `WKSecurityOrigin` objects for tests.
/// These types can no longer be created or mocked safely, as WebKit now requires
/// fully initialized instances. This helper loads a lightweight `WKWebView`
/// navigation and captures the frame and origin values that WebKit supplies.
final class WebKitTestHelpers {
class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate {
let expect: XCTestExpectation
var capturedFrame: WKFrameInfo?
var capturedOrigin: WKSecurityOrigin?

init(expect: XCTestExpectation) { self.expect = expect }

func webView(_ webView: WKWebView,
decidePolicyFor navigationAction: WKNavigationAction,
decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) {
let frame = navigationAction.targetFrame
capturedFrame = frame
capturedOrigin = frame?.securityOrigin
decisionHandler(.allow)
expect.fulfill()
return
Comment on lines +13 to +28
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "WebKitTestHelpers.swift" -type f 2>/dev/null | head -20

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 157


🏁 Script executed:

git ls-files | grep -i webkittesthelpers

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 155


🏁 Script executed:

find . -name "*.swift" -path "*Tests*" -type f 2>/dev/null | grep -i webkit

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 230


🏁 Script executed:

cat ./BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 2187


🏁 Script executed:

rg "FakeWKNavigationDelegate" --type swift -A 5 -B 2

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 1813


🏁 Script executed:

rg "captureFrameAndOrigin" --type swift -B 3 -A 3

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 3079


🏁 Script executed:

web_search
XCTestExpectation fulfill multiple times behavior

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 196


🌐 Web query:

XCTestExpectation fulfill called multiple times fatal error

💡 Result:

Cause:

  • XCTest throws a fatal error when you call expectation.fulfill() more than the framework expects (over-fulfillment) or after the test has completed — typically because your async callback fires multiple times or after teardown. [1][2]

Fixes (choose one):

  • Ensure you only call fulfill once: guard with a Bool flag or hold the expectation weak and nil it after fulfilling. This prevents duplicate callbacks from over‑fulfilling. [1][3]
  • If you legitimately expect N calls, set expectedFulfillmentCount = N (and optionally assertForOverFulfill = true). [2][3]
  • If you want to ignore extra fulfills, set expectation.assertForOverFulfill = false (but prefer fixing the multiple-callback root cause). [1][3]
  • Prefer XCTest’s async/await helpers (await fulfillment(of:timeout:)) or confirmation APIs for more explicit multi-confirmation semantics when using Swift concurrency. [2][6]

References:

  • StackOverflow: API violation — multiple calls made to -[XCTestExpectation fulfill]. [1]
  • Apple Developer — Asynchronous tests and expectations / XCTestExpectation docs (expectedFulfillmentCount, assertForOverFulfill). [2]
  • “XCTestExpectation Gotchas” (practical patterns: weak expectation, niling, guards). [3]

🏁 Script executed:

web_search
example.com HTTP redirects test behavior

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 190


🏁 Script executed:

web_search
WKNavigationDelegate decidePolicyFor multiple calls redirects subframes

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 199


🌐 Web query:

WKNavigationDelegate decidePolicyFor called multiple times redirects subframes

💡 Result:

Short answer: decidePolicyForNavigationAction can be invoked multiple times because WKWebView reports navigations for the main frame, for subframes/iframes, and again for provisional redirects (and for new-window/targetFrame=nil cases). Check navigationAction.targetFrame?.isMainFrame (or navigationAction.request.mainDocumentURL / navigationAction.sourceFrame) to distinguish main-frame navigations and avoid double-handling, and ensure you call the decisionHandler exactly once per callback.

Notes / tips:

  • Use navigationAction.targetFrame?.isMainFrame (or navigationAction.isMainFrame / WKFrameInfo) to ignore subframe calls when you only care about top-level navigation. [1]
  • Redirects and provisional navigations produce additional delegate callbacks (so the same logical navigation may trigger several decidePolicyFor calls). Track request URLs or navigation identifiers if you need to deduplicate. [2]
  • Never call decisionHandler more than once; guard paths (e.g. return after calling decisionHandler) to avoid the "completion handler ... was called more than once" crash. [1][2]

Sources:

  • Stack Overflow discussion about multiple/duplicate decisionHandler calls and crashes. [1]
  • WKNavigationDelegate API / docs describing navigation delegate callbacks (main-frame vs subframe / provisional redirects). [2]

🌐 Web query:

example.com HTTP 301 redirect test

💡 Result:

Short answer: example.com redirects HTTP -> HTTPS. I confirmed by requesting http://example.com which returned the site served at https://example.com. [1]

To verify yourself, run:

Expected output shows a 301/3xx status and a Location: https://example.com/ header. Use -L to follow the redirect chain. [2]

Sources:
[1] example.com homepage (requested via HTTP/HTTPS).
[2] Guide: using curl to test HTTP->HTTPS redirects.


🏁 Script executed:

rg "https://example.com" BrowserKit/Tests/WebEngineTests/ --type swift

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 1541


Guard against multiple expectation fulfillments from repeated navigation callbacks.

decidePolicyFor fires multiple times during real WKWebView navigation (for provisional navigations, redirects, and subframes). Calling expect.fulfill() unconditionally will cause a fatal error in XCTest. Add a flag to ensure fulfillment happens only once.

Proposed fix
 class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate {
     let expect: XCTestExpectation
     var capturedFrame: WKFrameInfo?
     var capturedOrigin: WKSecurityOrigin?
+    private var didFulfill = false

     init(expect: XCTestExpectation) { self.expect = expect }

     func webView(_ webView: WKWebView,
                  decidePolicyFor navigationAction: WKNavigationAction,
                  decisionHandler: `@escaping` (WKNavigationActionPolicy) -> Void) {
+        if !didFulfill {
+            let frame = navigationAction.targetFrame
+            capturedFrame = frame
+            capturedOrigin = frame?.securityOrigin
+            didFulfill = true
+            expect.fulfill()
+        }
         decisionHandler(.allow)
-        let frame = navigationAction.targetFrame
-        capturedFrame = frame
-        capturedOrigin = frame?.securityOrigin
-        decisionHandler(.allow)
-        expect.fulfill()
-        return
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate {
let expect: XCTestExpectation
var capturedFrame: WKFrameInfo?
var capturedOrigin: WKSecurityOrigin?
init(expect: XCTestExpectation) { self.expect = expect }
func webView(_ webView: WKWebView,
decidePolicyFor navigationAction: WKNavigationAction,
decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) {
let frame = navigationAction.targetFrame
capturedFrame = frame
capturedOrigin = frame?.securityOrigin
decisionHandler(.allow)
expect.fulfill()
return
class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate {
let expect: XCTestExpectation
var capturedFrame: WKFrameInfo?
var capturedOrigin: WKSecurityOrigin?
private var didFulfill = false
init(expect: XCTestExpectation) { self.expect = expect }
func webView(_ webView: WKWebView,
decidePolicyFor navigationAction: WKNavigationAction,
decisionHandler: `@escaping` (WKNavigationActionPolicy) -> Void) {
if !didFulfill {
let frame = navigationAction.targetFrame
capturedFrame = frame
capturedOrigin = frame?.securityOrigin
didFulfill = true
expect.fulfill()
}
decisionHandler(.allow)
}
}
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift` around
lines 13 - 28, The FakeWKNavigationDelegate's
webView(_:decidePolicyFor:decisionHandler:) unconditionally calls
expect.fulfill(), which can be invoked multiple times and crash tests; add a
Bool property (e.g., didFulfill or fulfilled) to the FakeWKNavigationDelegate
class, initialize it false, and guard the call to expect.fulfill() so it only
executes once (set the flag to true immediately after fulfilling); keep
capturing capturedFrame and capturedOrigin as-is and still call
decisionHandler(.allow).

}
}

// Loads a URL in a real WKWebView and returns the first valid
// WKFrameInfo and WKSecurityOrigin from a navigation action.
static func captureFrameAndOrigin(for url: URL, timeout: TimeInterval = 3.0) -> (WKFrameInfo, WKSecurityOrigin)? {
let webView = WKWebView(frame: .zero)
let expect = XCTestExpectation(description: "capture frame & origin")

let delegate = FakeWKNavigationDelegate(expect: expect)
webView.navigationDelegate = delegate

// load a real https URL (use example.com to be safe)
webView.load(URLRequest(url: url))

let waiter = XCTWaiter.wait(for: [expect], timeout: timeout)
if let frame = delegate.capturedFrame, let origin = delegate.capturedOrigin {
return (frame, origin)
}
return nil
}
}
34 changes: 23 additions & 11 deletions BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,32 +35,40 @@ final class WKUIHandlerTests: XCTestCase {
func testRequestMediaCaptureSuccess() {
let subject = createSubject(isActive: true)

let expectation = expectation(description: "Wait for the decision handler to be called")
guard let (frame, origin) = WebKitTestHelpers.captureFrameAndOrigin(for: URL(string: "https://example.com")!) else {
XCTFail("Could not obtain WKFrameInfo")
return
}

let decisionHandler = { (decision: WKPermissionDecision) in
XCTAssertEqual(decision, .prompt)
expectation.fulfill()
}

let expectation = expectation(description: "Wait for the decision handler to be called")
subject.webView(MockWKWebView(),
requestMediaCapturePermissionFor: MockWKSecurityOrigin.new(nil),
initiatedByFrame: MockWKFrameInfo(),
requestMediaCapturePermissionFor: origin,
initiatedByFrame: frame,
type: .cameraAndMicrophone,
decisionHandler: decisionHandler
)
decisionHandler: decisionHandler)
wait(for: [expectation])
expectation.fulfill()
}
Comment on lines 35 to 55
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift | sed -n '35,60p'

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 1413


🏁 Script executed:

find . -name "WKUIHandlerTests.swift" -type f

Repository: qodo-benchmark/firefox-ios-combined-coderabbit

Length of output: 146


Fix expectation lifecycle to prevent test timeout.

The expectation.fulfill() on line 54 is called after wait(for:) returns, which means the wait will always timeout. The expectation must be fulfilled inside the decisionHandler closure so it signals completion when the handler is invoked.

Fix
-        let decisionHandler = { (decision: WKPermissionDecision) in
-            XCTAssertEqual(decision, .prompt)
-        }
-
-        let expectation = expectation(description: "Wait for the decision handler to be called")
+        let expectation = expectation(description: "Wait for the decision handler to be called")
+        let decisionHandler = { (decision: WKPermissionDecision) in
+            XCTAssertEqual(decision, .prompt)
+            expectation.fulfill()
+        }
         subject.webView(MockWKWebView(),
                         requestMediaCapturePermissionFor: origin,
                         initiatedByFrame: frame,
                         type: .cameraAndMicrophone,
                         decisionHandler: decisionHandler)
         wait(for: [expectation])
-        expectation.fulfill()
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift` around lines 35 - 55,
The testRequestMediaCaptureSuccess test creates an XCTestExpectation but
fulfills it after wait(for:) returns, causing a timeout; move the
expectation.fulfill() into the decisionHandler closure so the expectation is
satisfied when the permission callback runs. Concretely, keep the expectation
declaration as-is, update the decisionHandler closure (used in
subject.webView(... requestMediaCapturePermissionFor: ... type: ...
decisionHandler: decisionHandler)) to call XCTAssertEqual(decision, .prompt) and
then call expectation.fulfill() inside that closure, and remove the trailing
expectation.fulfill() after wait(for:).


func testRequestMediaCaptureIsActiveFalse() {
let subject = createSubject(isActive: false)
let expectation = expectation(description: "Wait for the decision handler to be called")
guard let (frame, origin) = WebKitTestHelpers.captureFrameAndOrigin(for: URL(string: "https://example.com")!) else {
XCTFail("Could not obtain WKFrameInfo")
return
}

let expectation = expectation(description: "Wait for the decision handler to be called")
let decisionHandler = { (decision: WKPermissionDecision) in
XCTAssertEqual(decision, .deny)
expectation.fulfill()
}
subject.webView(MockWKWebView(),
requestMediaCapturePermissionFor: MockWKSecurityOrigin.new(nil),
initiatedByFrame: MockWKFrameInfo(),
requestMediaCapturePermissionFor: origin,
initiatedByFrame: frame,
type: .cameraAndMicrophone,
decisionHandler: decisionHandler
)
Expand All @@ -70,15 +78,19 @@ final class WKUIHandlerTests: XCTestCase {
func testRequestMediaCaptureDelegateReturnsFalse() {
sessionDelegate.hasMediaCapturePermission = false
let subject = createSubject(isActive: true)
guard let (frame, origin) = WebKitTestHelpers.captureFrameAndOrigin(for: URL(string: "https://example.com")!) else {
XCTFail("Could not obtain WKFrameInfo")
return
}
let expectation = expectation(description: "Wait for the decision handler to be called")

let decisionHandler = { (decision: WKPermissionDecision) in
XCTAssertEqual(decision, .deny)
expectation.fulfill()
}
subject.webView(MockWKWebView(),
requestMediaCapturePermissionFor: MockWKSecurityOrigin.new(nil),
initiatedByFrame: MockWKFrameInfo(),
requestMediaCapturePermissionFor: origin,
initiatedByFrame: frame,
type: .cameraAndMicrophone,
decisionHandler: decisionHandler
)
Expand Down