Merge remote-tracking branch 'origin/main'
This commit is contained in:
Taiga
@@ -0,0 +1,70 @@
|
||||
# EP2 TD Review - Candidate 848
|
||||
|
||||
## Scores
|
||||
|
||||
1. **Does the solution proposed effectively solve the goal of the project?**
|
||||
**3**
|
||||
|
||||
Yes. The proposal covers the actual MVP: fetch and store TDS with ETag validation, generate third-party WebKit content rules through TrackerRadarKit, support a current-site protection toggle, reload after protection changes, and provide an allowlist screen. It also starts from `BareBonesMobileBrowser`, which matters. This is not written as if the candidate can simply drop into the full DuckDuckGo browser architecture unchanged.
|
||||
|
||||
2. **Can they break down the project and extract what's important and de-risk the project?**
|
||||
**3**
|
||||
|
||||
This is one of the stronger parts of the submission. The candidate identifies the places where this feature can fail in ways users would actually notice: first navigation before rules are installed, offline first launch, failed TDS download, failed rule compilation, allowlist state drifting away from compiled state, cache misses, and WebKit's rule-count ceiling. The embedded TDS baseline is the right call. It means first launch with no network still has a protected path, which was the big calibrated gap in Candidate 7a2.
|
||||
|
||||
The phased plan is also sensible: prove blocking behavior first, then build the data lifecycle, then the toggle and allowlist UI.
|
||||
|
||||
3. **Are different approaches being considered with pros/cons and are the choices justified?**
|
||||
**3**
|
||||
|
||||
The tradeoff analysis is specific and useful. They explain why `WKContentRuleList` fits better than Safari extensions, `WKNavigationDelegate` interception, `NSURLProtocol`, or DNS/VPN filtering. They also compare ETag refresh against always downloading, compiled-list lookup against recompiling, exact-host allowlisting against eTLD+1, embedded TDS against download-only startup, and fail-closed behavior against best-effort behavior.
|
||||
|
||||
The choices are tied to real failure modes, not just generic pros/cons. That is exactly what I want from this kind of technical design.
|
||||
|
||||
4. **Are they able to technically apply good practices to solve the problem and demonstrate they understand the language/platform?**
|
||||
**2**
|
||||
|
||||
The platform understanding is clearly strong. Good signals include `WKContentRuleListStore` lookup before compile, deterministic identifiers that include the inputs affecting rule output, off-main TDS decoding / JSON encoding / rule compilation, commit-after-compile allowlist mutations, atomic TDS+ETag persistence, and a behavioral test strategy based on observing whether a controlled tracker endpoint receives a request.
|
||||
|
||||
The concurrency point is worth being precise about: the candidate does **not** propose compiling rules on the main thread. The submission explicitly says JSON encoding and compilation run off the main thread, with only `userContentController` application and UI work on the main actor. Given the TDS size and compile cost, that is the right direction.
|
||||
|
||||
I am still scoring this as a `2` rather than a `3` because the implementation handoff is not quite concrete enough for the starter app. The original prompt asks for enough detail that another engineer with zero context could implement tracker blocking. The design says the first navigation is gated behind `.ready`, but it does not spell out how `BareBonesMobileBrowser` actually enforces that: where a pending URL is stored, how address-bar submissions are blocked or queued, what happens to back/forward/reload while `.loading`, and whether this sits in a view model, coordinator, or navigation delegate. For a browser, that wiring is not a small detail. It is the part that prevents the user from accidentally loading before rules are applied.
|
||||
|
||||
Two other details should be treated as decisions needing approval rather than implementation defaults: raising the scaffold's deployment target, and offering a catastrophic "Continue without protection" path.
|
||||
|
||||
5. **Is the document well-structured and easy to understand/read?**
|
||||
**3**
|
||||
|
||||
The document is dense, but it is dense in a useful way. It moves cleanly from problem statement to risks, decisions, scope, architecture, flows, storage, concurrency, UI states, and testing. The tables and scenario matrix make the failure behavior easy to audit.
|
||||
|
||||
## Total
|
||||
|
||||
**14 / 15**
|
||||
|
||||
## Overall Recommendation
|
||||
|
||||
**3 - Yes**
|
||||
|
||||
## Feedback to Share With Candidate
|
||||
|
||||
Strong submission overall. The embedded TDS baseline, deterministic `WKContentRuleListStore` identifiers, lookup-before-compile path, off-main JSON/rule-list work, and commit-after-compile allowlist transaction are all solid choices for this feature. They show you understood that this is not just about generating rules. It is about keeping the applied rule list, stored inputs, and UI state consistent even when downloads or compiles fail.
|
||||
|
||||
The test plan is also a good sign. Verifying tracker blocking by checking whether a controlled endpoint receives a request is the right kind of proof here, since WebKit does not give us a neat callback for blocked subresources.
|
||||
|
||||
The main thing I would tighten before implementation is the BareBones integration detail. The design says navigation is gated behind `.ready`, but I would like to see the exact mechanics: where the pending navigation lives, how address-bar submits/back-forward/reload are handled while rules are compiling, and which object owns that state. That is the bit that makes the privacy guarantee real in the starter browser.
|
||||
|
||||
A couple of policy-ish items also need clearer framing. Moving off the scaffold's iOS 13 target may be reasonable, but it should not be assumed as part of the technical path. Same for the catastrophic "Continue without protection" option: that needs product/security agreement, not just an implementation decision.
|
||||
|
||||
## Calibration Notes
|
||||
|
||||
Compared with Candidate 7a2, this submission closes the main calibrated gaps:
|
||||
|
||||
- It handles first launch with no network by bundling an embedded TDS baseline.
|
||||
- It discusses `WKContentRuleListStore` reuse explicitly and designs identifiers around the inputs that affect compiled output.
|
||||
- It treats allowlist changes as a state consistency problem, not just a UI toggle.
|
||||
- It includes recovery paths for bad downloads, bad compiles, stale cache, and catastrophic store failure.
|
||||
- It considers exact-host allowlisting and validates that choice against DuckDuckGo behavior.
|
||||
|
||||
The score comes down on technical practices because the design stops one step short of showing how the loading gate is enforced in the actual starter browser UI. The idea is right. The integration detail is the missing piece.
|
||||
|
||||
Against the original prompt, the submission covers the requirements well: TDS download/storage, third-party-only blocking, allowlist toggle and reload, allowlist screen, scope, tradeoffs, error handling, performance, architecture, and testing. The main prompt gap is the final instruction: the document should be detailed enough for an engineer with zero context to implement tracker blocking. The core design is there, but the exact starter-app integration path is still too implicit.
|
||||
@@ -0,0 +1,25 @@
|
||||
# EP2 Prompt
|
||||
|
||||
## Background & Requirements
|
||||
|
||||
We are building a privacy focused browser. We already have the basic elements of a browser (a web view, address bar, back and forward buttons) and now need to extend that to add a tracker blocker MVP.
|
||||
|
||||
This should:
|
||||
|
||||
- Handle download and storage of the Tracker Radar data.
|
||||
- Implement basic tracker blocking (third party only) using `WKContentRuleList` & `TrackerRadarKit`. Outline any challenges that come with this approach, and how you would solve them.
|
||||
- From Main browser view, user should be able to toggle protection for a given website with a button (which adds/removes that website to/from the allowlist). After that, website should automatically reload with, or without protection.
|
||||
- Allow the user to see which websites have tracker blocking disabled, by navigating to a screen that has a simple list of domains coming from the allowlist.
|
||||
|
||||
There is more information about tracker blocking in the attached Tracker Blocking Background Information document. Please use the Tracker Radar data: [https://github.com/duckduckgo/tracker-radar](https://github.com/duckduckgo/tracker-radar) which is available via [https://staticcdn.duckduckgo.com/trackerblocking/v2.1/tds.json](http://staticcdn.duckduckgo.com/trackerblocking/v2.1/tds.json) (versioning is done with ETags), on iOS you can use the existing TrackerRadarKit: [https://github.com/duckduckgo/TrackerRadarKit](https://github.com/duckduckgo/TrackerRadarKit)
|
||||
|
||||
Think of this as a MVP to de-risk the project. Ideally what you create is something that can be expanded and collaborated on, so it's worth putting some thought into the architecture of the application, how to handle errors and possible performance impact.
|
||||
|
||||
The template below is a guide to what we are looking to understand with a Technical Design document. At DuckDuckGo we work asynchronously, so the goal of our Technical Design is to:
|
||||
|
||||
- Create and de-risk a plan of attack
|
||||
- Build buy-in into the plan, backed by reasoning and research
|
||||
|
||||
Following the template below, we would like to understand how you plan to break the problem down (including clarifying what is in and out of scope), what the pros, cons and tradeoffs of each approach are, what you'd ultimately recommend and how you plan to test it.
|
||||
|
||||
Please note that the Technical Design document should be at a level of detail that allows another engineer with zero context to implement tracker blocking.
|
||||
@@ -0,0 +1,233 @@
|
||||
---
|
||||
title: 'Tech Design: Opaque URL Fragment Detection for Same-Document Navigation'
|
||||
created: '2026-06-24'
|
||||
tags:
|
||||
- tech-design
|
||||
- apple
|
||||
- navigation
|
||||
- webkit
|
||||
- url
|
||||
- crash
|
||||
status: draft
|
||||
asana: >-
|
||||
https://app.asana.com/1/137249556945/project/1202406491309510/task/1215403820548743
|
||||
---
|
||||
**Author:** Apple team
|
||||
**Project:** fix-fragment-crash (macOS / iOS browser)
|
||||
**Sentry:** [APPLE-IOS-DY5A](https://errors.duckduckgo.com/organizations/ddg/issues/APPLE-IOS-DY5A/?project=8) · [DZ58](https://errors.duckduckgo.com/organizations/ddg/issues/APPLE-IOS-DZ58/?project=8) · [DZ5C](https://errors.duckduckgo.com/organizations/ddg/issues/APPLE-IOS-DZ5C/?project=8)
|
||||
|
||||
---
|
||||
|
||||
## Background & Requirements
|
||||
|
||||
The DuckDuckGo browser uses `WKWebView` for all page rendering. Every navigation goes through the `decidePolicyFor` delegate chain managed by `DistributedNavigationDelegate` (BrowserServicesKit). A central piece of that chain is classifying each `WKNavigationAction` as a **same-document navigation** (fragment-only change within the current page, e.g. `#section`) via:
|
||||
|
||||
```swift
|
||||
// WKNavigationActionExtension.swift:184
|
||||
public var isSameDocumentNavigation: Bool {
|
||||
switch navigationType {
|
||||
case .linkActivated, .other:
|
||||
return !isRedirect && newURL.hasFragment && currentURL.equals(newURL, by: .sameDocument)
|
||||
case .backForward:
|
||||
return (newURL.hasFragment || currentURL.hasFragment) && currentURL.equals(newURL, by: .sameDocument)
|
||||
...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`isSameDocumentNavigation` drives the `DistributedNavigationDelegate` state machine: same-document navigations reuse the existing `Navigation` object and trigger client-redirect handling rather than starting a fresh navigation, while non-same-document navigations allocate a new `Navigation`. Misclassification corrupts navigation state.
|
||||
|
||||
`hasFragment` and the `.sameDocument` equality both depend on extracting the URL fragment—which is where the bug lives.
|
||||
|
||||
### Two URL parsers coexist in WebKit on Apple platforms
|
||||
|
||||
| Parser | Used where | Standard | Fragment in opaque URLs (`data:`, `about:`, `blob:`) |
|
||||
|--------|-----------|----------|------------------------------------------------------|
|
||||
| **WTF::URLParser** | WebKit rendering, WKWebView navigation decisions | WHATWG URL | ✅ Correctly split at `#` |
|
||||
| **NSURL / CFURL** | Foundation APIs, Swift `URL` | Old RFC 1808/2396 | ❌ Encodes `#` as `%23`, returns `nil` for `.fragment` |
|
||||
|
||||
When WebKit bridges its internal `WTF::URL` to Objective-C it calls:
|
||||
|
||||
```c
|
||||
CFURLCreateAbsoluteURLWithBytes(alloc, rawBytes, len, kCFStringEncodingUTF8, nil, true)
|
||||
```
|
||||
|
||||
The raw bytes at this point still contain the literal `#`. CFURL re-parses them with its old-RFC parser, which does **not** recognise `#` as a fragment delimiter in opaque-scheme URLs. It encodes `#` → `%23` and stores the whole thing as the opaque path.
|
||||
|
||||
Result for `data:text/html,hello#anchor`:
|
||||
|
||||
| Property | WTF::URL | NSURL / Swift URL |
|
||||
|----------|----------|-------------------|
|
||||
| `.path` | `text/html,hello` | `""` (empty) |
|
||||
| `.query` | nil | nil |
|
||||
| `.fragment` | `"anchor"` | nil |
|
||||
| `.absoluteString` | `data:text/html,hello#anchor` | `data:text/html,hello%23anchor` |
|
||||
|
||||
---
|
||||
|
||||
## Problem Statement
|
||||
|
||||
A pathological URL—a multi-MB `data:` URL or attacker-crafted href—reaches `WKNavigationAction.isSameDocumentNavigation` on the main thread. The call chain:
|
||||
|
||||
1. `WebKit::WebPageProxy::decidePolicyForNavigationAction` dispatches to the main-thread delegate.
|
||||
2. `DistributedNavigationDelegate` reads `wkNavigationAction.isSameDocumentNavigation`.
|
||||
3. `isSameDocumentNavigation` calls `newURL.absoluteString.hashedSuffix` (and for `.backForward`, also `currentURL.absoluteString.hashedSuffix`).
|
||||
4. `hashedSuffix` → `hashedSuffixRange` → `String.firstIndex(of: "#")` (`StringExtension.swift:67`) iterates grapheme-by-grapheme over `absoluteString`.
|
||||
5. For a multi-MB `data:` URL this is O(n) UTF-8 grapheme decoding on the **main thread**, per navigation decision.
|
||||
6. Main thread blocked >10 s under thermal-state `serious` at 100% CPU → RunningBoard scene-update watchdog kills the app: **SIGKILL, terminationReason `0x8BADF00D`**.
|
||||
|
||||
The fix must:
|
||||
1. Make `hashedSuffixRange` / `isSameDocumentNavigation` fast for arbitrarily large URLs (no O(n) grapheme scan).
|
||||
2. Correctly detect fragments in opaque URLs where NSURL has already encoded `#` as `%23`.
|
||||
|
||||
---
|
||||
|
||||
## Why Standard APIs Can't Help: The Information-Loss Chain
|
||||
|
||||
Fragment information is **irreversibly lost** between WebKit's internal representation and what Swift code receives in `decidePolicyFor`:
|
||||
|
||||
```
|
||||
WTF::URL (WHATWG-compliant; m_string = "data:…#anchor", m_queryEnd before '#')
|
||||
↓ URL::createNSURL() → createCFURL()
|
||||
↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") ← '#' still visible here
|
||||
↓ CFURL re-parses with old-RFC parser; encodes '#' → '%23'
|
||||
NSURL (.absoluteString = "…%23anchor", .fragment = nil, .path = "")
|
||||
↓ [NSMutableURLRequest initWithURL: url().createNSURL()] (ResourceRequestCocoa.mm)
|
||||
NSURLRequest (same NSURL)
|
||||
↓
|
||||
WKNavigationAction.request.URL ← what Swift sees
|
||||
WKNavigationAction._originalURL ← same path, same loss (createNSURL() again)
|
||||
```
|
||||
|
||||
Every Foundation/Swift API is downstream of the encoding step:
|
||||
|
||||
- **`URL.fragment` / `.path` / `.query`** — return `nil` / `""` for all opaque URLs.
|
||||
- **`URLComponents(url:)` / `(string:)` and `NSURLComponents` equivalents** — either use NSURL's already-broken cached parse tree, or re-parse `absoluteString` where `#` is already `%23`. RFC 3986 treats `%23` as a literal character, not a delimiter. Fragment stays nil in all four variants.
|
||||
- **`WKNavigationActionPrivate.h`** (`_originalURL`, `_isRedirect`, `_hitTestResult`, …) — all URL-returning properties go through `WTF::URL::createNSURL()` → same `createCFURL()` path. No private API exposes the raw WTF string.
|
||||
|
||||
There is **no Foundation or WebKit API**, public or private, that recovers the original `#` once the NSURL has been constructed.
|
||||
|
||||
---
|
||||
|
||||
## Approaches Tried
|
||||
|
||||
### 1. Scan `absoluteString` for `%23`
|
||||
|
||||
Since NSURL encodes the original `#` as `%23` in `absoluteString`, scanning for `%23` as a fragment delimiter recovers the fragment in many cases. Implemented and partly working.
|
||||
|
||||
**Fundamental problem:** `%23` is also valid payload data in `data:` URLs:
|
||||
- SVG fill colors: `data:image/svg+xml,<rect fill="%23ff0000"/>` — `%23` is `#` in the SVG
|
||||
- HTML anchor hrefs: `data:text/html,<a href="%23section">`
|
||||
- CSS: `data:text/css,color:%23333`
|
||||
|
||||
Both cases produce identical `absoluteString`. They are **indistinguishable from the string alone**. A 4 KB size guard was added to avoid scanning large `data:` URLs, but that makes two different large data: URLs compare as equal — semantically wrong.
|
||||
|
||||
### 2. Private API / `WKNavigationAction._originalURL`
|
||||
|
||||
See information-loss chain above. No private API bypasses it; all paths go through `createCFURL()`.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Approach: `CFURLCreateAbsoluteURLWithBytes` Interpose
|
||||
|
||||
### Key Insight
|
||||
|
||||
WebKit calls `CFURLCreateAbsoluteURLWithBytes()` at the **exact** moment it converts a `WTF::URL` to a `CFURL`/`NSURL`. The raw byte buffer at that call site still contains the literal `#`, before CFURL encodes it. Intercepting this C function gives unambiguous access to the original bytes.
|
||||
|
||||
### Mechanism: `DYLD_INTERPOSE`
|
||||
|
||||
The `__DATA,__interpose` Mach-O section replaces any C function in the same dylib at load time, without method swizzling or private SPI:
|
||||
|
||||
```c
|
||||
#define DYLD_INTERPOSE(_replacement, _replacee) \
|
||||
__attribute__((used)) \
|
||||
static struct { const void *replacement; const void *replacee; } \
|
||||
_interpose_##_replacee \
|
||||
__attribute__((section("__DATA,__interpose"))) = \
|
||||
{ (const void *)&(_replacement), (const void *)&(_replacee) }
|
||||
```
|
||||
|
||||
### Implementation
|
||||
|
||||
Files:
|
||||
- `CFURLCreateAbsoluteURLWithBytesInterpose.m` — DEBUG-only (`#ifdef DEBUG`)
|
||||
- `NSURL+FragmentByteRange.h` / `.m` — category exposing the stored range
|
||||
|
||||
**Interpose logic:**
|
||||
|
||||
1. Call the real `CFURLCreateAbsoluteURLWithBytes` to get the `CFURLRef`.
|
||||
2. For opaque URLs (scheme present, no `//` authority), scan the raw byte buffer for `'#'` (byte `0x23`) with a simple C loop—O(n) but done once at NSURL construction, not per navigation decision.
|
||||
3. If found, store the byte range as an `NSValue` associated object on the bridged `NSURL` via `objc_setAssociatedObject`.
|
||||
4. Callers read `NSURL.ddg_fragmentByteRange` for the unambiguous `#` position.
|
||||
|
||||
```objc
|
||||
if (relativeURLBytes[i] == '#') {
|
||||
NSRange fragmentRange = NSMakeRange(i, length - i);
|
||||
NSURL *nsurl = (__bridge NSURL *)result;
|
||||
objc_setAssociatedObject(nsurl, &kFragmentByteRangeKey,
|
||||
[NSValue valueWithRange:fragmentRange],
|
||||
OBJC_ASSOCIATION_RETAIN_NONATOMIC);
|
||||
}
|
||||
```
|
||||
|
||||
### Why This Is Correct
|
||||
|
||||
- Bytes are scanned **before** CFURL encodes `#` → `%23` — no ambiguity with payload `%23`.
|
||||
- Associated object is set on the **exact NSURL instance** for that WTF::URL.
|
||||
- `ddg_fragmentByteRange.location` matches WTF::URL's internal `m_queryEnd + 1`.
|
||||
- Works for all opaque schemes: `data:`, `about:`, `blob:`, `javascript:`.
|
||||
- DEBUG-only: zero production overhead; interpose section absent from release builds.
|
||||
|
||||
### Complete Call Flow with Interpose
|
||||
|
||||
```
|
||||
WTF::URL (m_string = "data:…#anchor", m_queryEnd before '#')
|
||||
↓ URL::createCFURL()
|
||||
↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor")
|
||||
↑↑ INTERPOSED: scan rawBytes → store NSRange on result NSURL
|
||||
↓ NSURL (.absoluteString = "…%23anchor", .fragment = nil)
|
||||
↑ .ddg_fragmentByteRange → {location=N} — unambiguous '#' position
|
||||
Swift URLExtension.opaqueComponents:
|
||||
DEBUG → use ddg_fragmentByteRange (exact)
|
||||
release → NSString.range(of: "#") fallback (unambiguous for non-data: schemes)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
[1] The `%23` ambiguity only affects `data:` or `about:` URLs. For `blob:`, `javascript:` the opaque path never legitimately contains `%23` as encoded payload.
|
||||
|
||||
[2] The primary safety fix is the O(n) crash: replace `String.firstIndex(of: "#")` with a byte-level scan and short-circuit on `data:` scheme or length threshold. The interpose is the accuracy improvement layered on top for DEBUG validation.
|
||||
|
||||
[3] WebKit bug for NSURL not handling `#` in opaque URLs: [bugs.webkit.org/68089](https://bugs.webkit.org/show_bug.cgi?id=68089). Chromium fixed this in M72 (2019). WebKit's WTF::URLParser is correct; only the Foundation/CFURL bridge is broken.
|
||||
|
||||
---
|
||||
|
||||
## Testing
|
||||
|
||||
- **Unit tests** (`URLExtensionTests.swift`):
|
||||
- `equals(by: .sameDocument)` and `equals(by: .fuzzyIdentity)` on opaque URLs with fragment: `about:blank#section`, `data:text/html,…#anchor`
|
||||
- Opaque URLs with query AND fragment: `data:text/html,…?key=value#anchor`
|
||||
- Large (20 MB) `data:` URL performance test: comparison must complete in < 0.1 s
|
||||
- Diagnostic timing test (`tempOpaqueNSURLComponentTimings`) printing per-operation durations for all Foundation URL APIs on a 20 MB `data:` URL with query and fragment
|
||||
|
||||
- **Manual test** (DEBUG build): verify `ddg_fragmentByteRange` is set on `WKNavigationAction.request.URL` when navigating to a `data:` URL with a `#` fragment via `WKWebView`.
|
||||
|
||||
- **Regression**: navigation tests in `DistributedNavigationDelegateTests.swift` and `NavigationTestHelpers.swift` updated to use `.equals(by: .fuzzyIdentity)` in place of removed `.matches()` API.
|
||||
|
||||
---
|
||||
|
||||
## Additional Considerations
|
||||
|
||||
### Privacy
|
||||
- No user data collected or transmitted. All processing is local and in-process.
|
||||
|
||||
### Security
|
||||
- No extra data is stored or passed anywhere
|
||||
- The approach injects into the existing API and stores a helper pointer
|
||||
### Site Breakage
|
||||
- The O(n) fix prevents crashes on large URLs. Fragment-detection correctness improves navigation state machine accuracy but does not change whether a navigation is allowed or blocked.
|
||||
|
||||
### Operational
|
||||
- No new infrastructure, network calls, or persisted state. The crash was a SIGKILL watchdog kill visible in Sentry; the fix eliminates the `0x8BADF00D` terminations for this call chain.
|
||||
@@ -0,0 +1,160 @@
|
||||
---
|
||||
tags:
|
||||
- ddg-browser
|
||||
- bugs
|
||||
- performance
|
||||
- ios
|
||||
- macos
|
||||
created: '2026-06-23'
|
||||
status: in-progress
|
||||
---
|
||||
# absoluteString O(n) scan — main-thread stall bugs
|
||||
|
||||
## Root cause
|
||||
|
||||
`String.firstIndex(of: "#")` decodes grapheme clusters across every UTF-8 byte, making it O(n) in string length. `URL.absoluteString` materialises the full raw URL — including multi-megabyte `data:` URIs (e.g. content-scope privacy config JSON). Calling any character-scanning API on this on the **main thread** causes visible stalls and, for large enough payloads, a **SIGKILL from the iOS watchdog**.
|
||||
|
||||
**Measured baseline (Apple Silicon M-series, optimised utf8 byte scan, no guard):**
|
||||
|
||||
| Payload | Time |
|
||||
|---|---|
|
||||
| 1 MB, no `#` | ~15 ms |
|
||||
| 1 MB, `#` at end | ~15 ms |
|
||||
| 15 MB, no `#` | **~232 ms** |
|
||||
|
||||
On older iPhone A-series (3–5× slower): 15 MB → **~700 ms–1.1 s per call**. Stacked across multiple nav actions per load = watchdog territory.
|
||||
|
||||
`Foundation.URL.fragment`, by contrast, parses all URL components eagerly at `URL(string:)` init time — accessing `.fragment` is O(1) regardless of payload size (confirmed with 1 MB `data:` URI test, both with and without `#` at the end: < 1 ms).
|
||||
|
||||
---
|
||||
|
||||
## Known fixed
|
||||
|
||||
### `hashedSuffixRange` — `StringExtension.swift`
|
||||
|
||||
**File:** `SharedPackages/Infrastructure/SystemFrameworksExtensions/Sources/FoundationExtensions/StringExtension.swift`
|
||||
|
||||
**Fix:** `data:` guard at top of `hashedSuffixRange` short-circuits before any scan. Also replaced `String.firstIndex(of: "#")` with `utf8.firstIndex(of: 0x23)` (byte-level, avoids grapheme decoding).
|
||||
|
||||
**Note:** `about:` + `%23` special case intentionally preserved — Foundation percent-encodes `#` to `%23` in opaque `about:` URL absoluteStrings in some construction paths (e.g. `URL(trimmedAddressBarString:)`). Introduced in commit `e880d888b3` ("fix about: scheme fragments dropping", Mar 2023). `URL.fragment` returns nil for these, so the string scan branch is genuinely needed.
|
||||
|
||||
**Asana task:** https://app.asana.com/1/137249556945/project/1202406491309510/task/1215403820548743
|
||||
|
||||
---
|
||||
|
||||
## Known bugs — not yet fixed
|
||||
|
||||
### HIGH — navigation hot path
|
||||
|
||||
#### `isSameDocument` / `droppingHashedSuffix` — `URLExtension.swift:387`
|
||||
|
||||
```swift
|
||||
// URLExtension.swift:387
|
||||
public func isSameDocument(_ other: URL) -> Bool {
|
||||
self.absoluteString.droppingHashedSuffix() == other.absoluteString.droppingHashedSuffix()
|
||||
}
|
||||
```
|
||||
|
||||
Called from `WKNavigationActionExtension.isSameDocumentNavigation` (main thread, every nav action). Same O(n) scan via `droppingHashedSuffix` → `hashedSuffixRange`. Will be covered once the `data:` guard lands in `hashedSuffixRange`.
|
||||
|
||||
**Fix suggestion:** The `data:` guard in `hashedSuffixRange` propagates here automatically since `droppingHashedSuffix` delegates to it. No separate fix needed — covered by the `StringExtension` fix.
|
||||
|
||||
#### `matches(_ other: URL)` — `URLExtension.swift:32–36`
|
||||
|
||||
```swift
|
||||
public func matches(_ other: URL) -> Bool {
|
||||
let string1 = self.absoluteString
|
||||
let string2 = other.absoluteString
|
||||
return string1.droppingHashedSuffix().dropping(suffix: "/").appending(string1.hashedSuffix ?? "")
|
||||
== string2.droppingHashedSuffix().dropping(suffix: "/").appending(string2.hashedSuffix ?? "")
|
||||
}
|
||||
```
|
||||
|
||||
Used in `FrameInfo.Equatable`, `ContextMenuManager`, `PopupHandlingTabExtension` — all main thread. Also covered by `hashedSuffixRange` `data:` guard.
|
||||
|
||||
#### `DistributedNavigationDelegate.swift:673`
|
||||
|
||||
`hashedSuffix` + `droppingHashedSuffix` in `willPerformClientRedirect`. Same chain. Covered by guard.
|
||||
|
||||
---
|
||||
|
||||
### HIGH — JS injection with full absoluteString
|
||||
|
||||
#### `TabViewController.swift:4719`
|
||||
```swift
|
||||
"window.location.href='" + url.absoluteString + "'"
|
||||
```
|
||||
Entire URL embedded in a JS string for `evaluateJavaScript`. If `url` is a large `data:` URI, the JS string is megabytes long.
|
||||
|
||||
**Fix suggestion:** Check scheme before embedding. `data:` URIs should never be navigated to via `window.location.href` injection — bail early if `url.scheme == "data"`.
|
||||
|
||||
#### `Tab.swift` / `WKWebViewExtension.swift` — `escapedJavaScriptString()`
|
||||
|
||||
Same pattern — `absoluteString` → `escapedJavaScriptString()` → `evaluateJavaScript(...)`. Used in error-page recovery (`location.replace(...)`) and `window.open`.
|
||||
|
||||
**Fix suggestion:** Same — guard on scheme before serialising into JS.
|
||||
|
||||
---
|
||||
|
||||
### HIGH — blob: credential stripping
|
||||
|
||||
#### `macOS/DuckDuckGo/Common/Extensions/URLExtension.swift:843–849`
|
||||
|
||||
```swift
|
||||
// strippingUnsupportedCredentials() for blob: URLs
|
||||
firstIndex(of: "@") + replacingOccurrences(..., .regularExpression)
|
||||
```
|
||||
|
||||
`blob:` URLs can embed large payloads (base64-encoded media). Called from `TabViewModel.addressBarString` on `@MainActor`.
|
||||
|
||||
**Fix suggestion:** `blob:` URLs never have `@`-credentials in practice. Add a scheme guard:
|
||||
```swift
|
||||
guard scheme == "http" || scheme == "https" else { return self }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM — address bar rendering
|
||||
|
||||
#### `macOS/DuckDuckGo/Tab/ViewModel/TabViewModel.swift:403`
|
||||
```swift
|
||||
addressBarString = url.absoluteString // non-blob path
|
||||
```
|
||||
`TabViewModel.passiveAddressBarString` already guards `data:` (returns `"data:"`), but `addressBarString` does not.
|
||||
|
||||
**Fix suggestion:** Apply same guard — if `url.scheme == "data"`, set `addressBarString = "data:"`.
|
||||
|
||||
#### `iOS/DuckDuckGo/AddressDisplayHelper.swift:55–74`
|
||||
`NSMutableAttributedString(string: absoluteString)` + `NSRange` deemphasis. Omnibar rendering on main thread.
|
||||
|
||||
**Fix suggestion:** Same — cap or replace `data:` URLs before constructing attributed string.
|
||||
|
||||
#### `iOS/DuckDuckGo/BackForwardMenuHistoryItem.swift:41–58`
|
||||
`URL(trimmedAddressBarString: url.absoluteString)`, `dropping`, `.count`, `dropLast` on main thread for back/forward menu construction.
|
||||
|
||||
**Fix suggestion:** Guard on `url.scheme != "data"` before passing to `trimmedAddressBarString` initialiser.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM — navigation pipeline regex/prefix checks
|
||||
|
||||
#### `LinkCleaner.swift:36,61–65`
|
||||
`NSRegularExpression.matches(in: url.absoluteString)` — AMP format matching. Guarded to http/https by the call site but still scans full string length.
|
||||
|
||||
**Fix suggestion:** `AMPCanonicalExtractor` already has a `maxURLLength` guard — ensure `LinkCleaner` applies the same.
|
||||
|
||||
#### `HistoryViewDataProvider.swift:478`
|
||||
`absoluteString.localizedCaseInsensitiveContains(searchTerm)` — history search. Runs on main thread. History URLs are rarely `data:` but theoretically possible.
|
||||
|
||||
#### `AIChatMentionPickerFilter.swift:57`
|
||||
`tab.url.absoluteString.lowercased().contains(query)` — AI Chat tab picker filter, main thread.
|
||||
|
||||
**Fix suggestion:** Both — early-return `false` if `url.scheme == "data"` before scanning.
|
||||
|
||||
---
|
||||
|
||||
## Test coverage added (branch `fix-fragment-crash`)
|
||||
|
||||
- `HashedSuffixDataURIPerformanceTests` in `StringExtensionTests.swift` — asserts `hashedSuffix` completes in < 1 ms on 1 MB and 15 MB `data:` URIs (with and without `#` at end). Fails without the `data:` guard (15 MB → ~232 ms).
|
||||
- `URLFragmentDataURIPerformanceTests` in `WKNavigationActionExtensionTests.swift` — asserts `URL.fragment` completes in < 10 ms on 1 MB `data:` URI. Confirms Foundation parses eagerly (O(1) access).
|
||||
- `WKNavigationActionExtensionTests` — full unit test suite for `isSameDocumentNavigation` covering all nav types, `data:` URLs, `about:%23` edge case.
|
||||
Reference in New Issue
Block a user