Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/625")Built from 685d3b9 |
f15a637 to
9944c98
Compare
dcalhoun
left a comment
There was a problem hiding this comment.
I feel this type of design decision is worth @crazytonyli's review.
The alignment with Android and proposed changes make sense. I tested these changes in the Demo app using #629.
I captured findings by Claude, which I believe are legitimate and worth considering.
| // the failure actually being hit — not an oversight. #630 drops the class | ||
| // requirement from the protocol so a host can conform with a value type. | ||
| // swiftlint:disable:next weak_delegate | ||
| public var mediaUploadDelegate: (any MediaUploadDelegate)? { |
There was a problem hiding this comment.
Finding from Claude:
Is editor ownership a goal here, or only the fix for the mid-request read?
If it's only the fix, reading the delegate once at the top of handleUpload and passing it down keeps the whole request on one delegate while the property stays weak. If it's a goal (assign-and-forget, value-type conformers in #630, Android parity), it brings the cycle from Known issue, and #630 and #631 already build on it.
If the editor keeps ownership
- A host that hits the cycle has no way out. The setter traps after load, and the server holds its own reference, so clearing the property wouldn't break the cycle anyway. Is the doc warning enough, or is an explicit teardown worth adding?
releaseConnectionHandlerdoesn't cover an in-flight upload. The task holds the delegate untilprocessFilereturns, so when the editor is its only owner (as the docs invite), the last release can still land off main. Worth noting on the property, and scopingreleaseConnectionHandler's doc to an idle server.
If we read once per request
- Revert the property and
UploadContextto weak. refactor!: rename MediaUploadDelegate to MediaProcessor, and drop its class bound #630 keeps: AnyObject, and feat(ios): add HTTPRequestHandler, and serve media uploads from one #631'sHandlerholds the processor weakly. processesForHostReleasedDelegatewould need to drop the host's reference insidehandlesFile, since dropping it before the request should pass through under a weak contract.- Restore
doesNotStronglyRetainDelegate.releaseConnectionHandlerbecomes cleanup rather than a fix.
There was a problem hiding this comment.
Ownership is the goal, not only the fix for the mid-request read. The handler is now supplied at init and held for the editor's lifetime, so both points under "if the editor keeps ownership" are addressed:
- Way out:
733a2a88addsstopMediaHandling()— it stops the server, drops the editor's references, and withdraws the endpoint from the page. The setter and its after-load trap are gone.stopMediaHandlingBreaksTheOwnershipCyclepins it. - In-flight release:
b6cd4f27documents that a request in flight holds its own reference untilprocessFilereturns, so the last release can land on the task's executor. That's on the property, onstopMediaHandling(), and onreleaseConnectionHandler, whose doc is now scoped to an idle server.
Read-once with weak was rejected: it makes the three reads agree, but leaves nil ambiguous between "never configured" and "host released it", deallocates an init-injected processor immediately, and can't hold the value-type conformers MediaProcessor now allows.
| /// lands there rather than wherever `stop()` was called. For GutenbergKit's | ||
| /// upload server that means a host's media handler could be deallocated off the | ||
| /// main thread on a path that started in `EditorViewController.deinit`. |
There was a problem hiding this comment.
Finding from Claude:
Nit: GutenbergKitHTTP doesn't otherwise know about its consumers, and the preceding sentence already covers the effect.
| /// lands there rather than wherever `stop()` was called. For GutenbergKit's | |
| /// upload server that means a host's media handler could be deallocated off the | |
| /// main thread on a path that started in `EditorViewController.deinit`. | |
| /// lands there rather than wherever `stop()` was called. |
There was a problem hiding this comment.
Applied in b6cd4f27 — the paragraph now ends at "wherever stop() was called", followed by the idle-server scoping.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `releaseMediaHandling()` drops the server and both handlers, and `viewDidDisappear` calls it once the editor is genuinely going away (`isBeingDismissed || isMovingFromParent`, both false when a view controller is merely presented over it — which is why stopping on a bare disappear previously left uploads broken on return). Both edges have to go: releasing only one leaves the cycle routed through the other. This mirrors Android, which tears the server down in `onDetachedFromWindow`. That — a lifecycle callback rather than a reachability event — is the actual asymmetry between the platforms, not the garbage collector. Both handlers move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. Android keeps its settable property and `check(...)` because a `View` is inflated, not constructed by the host. Constructor injection doesn't close the cycle — a host that owns the editor and is its own processor writes the same shape — so the release above is still what opens it. `deinit` stays as a backstop for an editor that is never presented; that case gets no lifecycle callback and still leaks, which the docs say plainly. Also restores `mediaProcessor`'s doc comment, which had been merged into `mediaUploader`'s as a single stranded block, leaving the primary public extension point undocumented. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `releaseMediaHandling()` drops the server and both handlers, and `viewDidDisappear` calls it once the editor is genuinely going away (`isBeingDismissed || isMovingFromParent`, both false when a view controller is merely presented over it — which is why stopping on a bare disappear previously left uploads broken on return). Both edges have to go: releasing only one leaves the cycle routed through the other. This mirrors Android, which tears the server down in `onDetachedFromWindow`. That — a lifecycle callback rather than a reachability event — is the actual asymmetry between the platforms, not the garbage collector. Both handlers move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. Android keeps its settable property and `check(...)` because a `View` is inflated, not constructed by the host. Constructor injection doesn't close the cycle — a host that owns the editor and is its own processor writes the same shape — so the release above is still what opens it. `deinit` stays as a backstop for an editor that is never presented; that case gets no lifecycle callback and still leaks, which the docs say plainly. Also restores `mediaProcessor`'s doc comment, which had been merged into `mediaUploader`'s as a single stranded block, leaving the primary public extension point undocumented. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `tearDown()` stops the server and drops both handlers, opening the cycle from the editor's side. It is the host's call to make, because the editor cannot detect its own teardown. An earlier revision of this branch tried to infer it from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`. That does not work: `isBeingDismissed` does not propagate down a containment chain, and the editor is a child view controller in every real host, so both flags read false on it while an ancestor carries the true value. Measured on a simulator across four presentation shapes, the SwiftUI demo, and the real editor — the guard blocked the release every time. WordPress-iOS hit the same UIKit behaviour and carries `isBeingDismissedDirectlyOrByAncestor()` for it; walking ancestors would fix those shapes and still misfire on containers that re-parent (`UIPageViewController` recycling a child sets `isMovingFromParent` while the editor survives) and stay silent when a stack is reset out from under a covered editor. A heuristic over host configurations we cannot enumerate is the wrong trade for a library, so this states the contract instead. Both handlers also move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. Android keeps its settable property and `check(...)` because a `View` is inflated, not constructed by the host. `deinit` is unchanged and remains the ordinary path: with no cycle, ARC releases the handlers and `deinit` stops the server, so a host that follows the documented rule — don't retain the editor from your handler — needs nothing. Verified in the demo app, where `deinit` runs on close. Also restores `mediaProcessor`'s doc comment, which had been merged into `mediaUploader`'s as a single stranded block, leaving the primary public extension point undocumented. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `stopMediaHandling()` stops the server and drops both handlers, opening the cycle from the editor's side. It is the host's call to make, because the editor cannot detect its own teardown. An earlier revision inferred it from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that does not work, because `isBeingDismissed` does not propagate down a containment chain and the editor is a child view controller in every real host. Measured across four presentation shapes, the SwiftUI demo, and the real editor: the guard blocked the release every time. Walking ancestors fixes those shapes but still misfires where a container re-parents, and stays silent when a stack is reset under a covered editor. No UIKit callback distinguishes teardown from being covered or re-parented, so this states the contract rather than guessing at it. Stopping now also withdraws the endpoint from the page. The port and token are injected once at document start, and `nativeMediaUploadMiddleware` refuses to retry a failed native upload directly, on the stated assumption that an advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so a stopped server left every image insert failing with a connection error on a working connection — and an uploader host's orphan-cleanup DELETE broken with it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage` copy `getGBKit()` falls back to, and the injected user script that would otherwise restore the dead port at the next document start, including the reload that recovers a terminated WebContent process. Uploads fall back to the default WebView path instead of failing. `MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint, matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason: a value type cannot participate in a reference cycle, so the question doesn't arise. It doesn't prevent the cycle — a struct holding a class reference closes it just as well — but it makes the acyclic shape expressible, and an `AnyObject` protocol taken at `init` reads like an invitation to pass `self`. Source-compatible: every existing class conformer still conforms. A DEBUG-only census counts live upload servers and logs a fault past four. Each live server is a bound loopback listener, one per editor, so monotone growth is this cycle and nothing else produces it — `warmup()` passes no handlers and starts no server. It is the only detectable symptom available: a `deinit` assertion cannot fire, because a cycle is what stops `deinit` from running. Logged, never fatal; the threshold is a heuristic, and crashing a host's debug build over a heuristic is a worse trade than the leak. Both handlers also move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. The retention rule now lives on the `init` parameters and in the integration guide, since the initializer is the only media call site a host writes. `deinit` is unchanged and remains the ordinary path: with no cycle, ARC releases the handlers and `deinit` stops the server. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `stopMediaHandling()` stops the server and drops both handlers, opening the cycle from the editor's side. It is the host's call to make, because the editor cannot detect its own teardown. An earlier revision inferred it from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that does not work, because `isBeingDismissed` does not propagate down a containment chain and the editor is a child view controller in every real host. Measured across four presentation shapes, the SwiftUI demo, and the real editor: the guard blocked the release every time. Walking ancestors fixes those shapes but still misfires where a container re-parents, and stays silent when a stack is reset under a covered editor. No UIKit callback distinguishes teardown from being covered or re-parented, so this states the contract rather than guessing at it. Stopping now also withdraws the endpoint from the page. The port and token are injected once at document start, and `nativeMediaUploadMiddleware` refuses to retry a failed native upload directly, on the stated assumption that an advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so a stopped server left every image insert failing with a connection error on a working connection — and an uploader host's orphan-cleanup DELETE broken with it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage` copy `getGBKit()` falls back to, and the injected user script that would otherwise restore the dead port at the next document start, including the reload that recovers a terminated WebContent process. Uploads fall back to the default WebView path instead of failing. `MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint, matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason: a value type cannot participate in a reference cycle, so the question doesn't arise. It doesn't prevent the cycle — a struct holding a class reference closes it just as well — but it makes the acyclic shape expressible, and an `AnyObject` protocol taken at `init` reads like an invitation to pass `self`. Source-compatible: every existing class conformer still conforms. A DEBUG-only census counts live upload servers and logs a fault past four. Each live server is a bound loopback listener, one per editor, so monotone growth is this cycle and nothing else produces it — `warmup()` passes no handlers and starts no server. It is the only detectable symptom available: a `deinit` assertion cannot fire, because a cycle is what stops `deinit` from running. Logged, never fatal; the threshold is a heuristic, and crashing a host's debug build over a heuristic is a worse trade than the leak. Both handlers also move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. The retention rule now lives on the `init` parameters and in the integration guide, since the initializer is the only media call site a host writes. Both also spell out what reuse across editor sessions requires: the editor drops only its own reference when it goes, so a host sharing one handler — the expected shape for an uploader, whose background session or offline queue outlives any editor — has to keep its own. Sharing is the safer shape, since a handler owned by something longer-lived than any editor is a leaf and cannot form the cycle at all. `deinit` is unchanged and remains the ordinary path: with no cycle, ARC releases the handlers and `deinit` stops the server. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `stopMediaHandling()` stops the server and drops both handlers, opening the cycle from the editor's side. It is the host's call to make, because the editor cannot detect its own teardown. An earlier revision inferred it from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that does not work, because `isBeingDismissed` does not propagate down a containment chain and the editor is a child view controller in every real host. Measured across four presentation shapes, the SwiftUI demo, and the real editor: the guard blocked the release every time. Walking ancestors fixes those shapes but still misfires where a container re-parents, and stays silent when a stack is reset under a covered editor. No UIKit callback distinguishes teardown from being covered or re-parented, so this states the contract rather than guessing at it. Stopping now also withdraws the endpoint from the page. The port and token are injected once at document start, and `nativeMediaUploadMiddleware` refuses to retry a failed native upload directly, on the stated assumption that an advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so a stopped server left every image insert failing with a connection error on a working connection — and an uploader host's orphan-cleanup DELETE broken with it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage` copy `getGBKit()` falls back to, and the injected user script that would otherwise restore the dead port at the next document start, including the reload that recovers a terminated WebContent process. Uploads fall back to the default WebView path instead of failing. `MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint, matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason: a value type cannot participate in a reference cycle, so the question doesn't arise. It doesn't prevent the cycle — a struct holding a class reference closes it just as well — but it makes the acyclic shape expressible, and an `AnyObject` protocol taken at `init` reads like an invitation to pass `self`. Source-compatible: every existing class conformer still conforms. A DEBUG-only census counts live upload servers and logs a fault past four. Each live server is a bound loopback listener, one per editor, so monotone growth is this cycle and nothing else produces it — `warmup()` passes no handlers and starts no server. It is the only detectable symptom available: a `deinit` assertion cannot fire, because a cycle is what stops `deinit` from running. Logged, never fatal; the threshold is a heuristic, and crashing a host's debug build over a heuristic is a worse trade than the leak. Both handlers also move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. The retention rule now lives on the `init` parameters and in the integration guide, since the initializer is the only media call site a host writes. Both also spell out what reuse across editor sessions requires: the editor drops only its own reference when it goes, so a host sharing one handler — the expected shape for an uploader, whose background session or offline queue outlives any editor — has to keep its own. Sharing is the safer shape, since a handler owned by something longer-lived than any editor is a leaf and cannot form the cycle at all. The rule is stated as what it actually is. Nothing in GutenbergKit hands a handler the editor — every value crossing that boundary is a `Sendable` value type — so the cycle is entirely host-authored: it forms only when the object that already holds the editor to drive it also conforms. "Don't conform the object that owns this editor" is the actionable phrasing, and it costs the host nothing, because these methods are called off the main actor and could not have reached that object's state anyway. `deinit` is unchanged and remains the ordinary path: with no cycle, ARC releases the handlers and `deinit` stops the server. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload can't lose its handler mid-request. That leaves a host object which holds the editor back closing a cycle ARC cannot break, and the host has no way out of it: dropping its own reference wouldn't help, because a running server holds one of its own through `editor -> uploadServer -> HTTPServer -> listener -> newConnectionHandler -> handler -> processor`. The only release point was `deinit` — exactly what a cycle prevents — so every leaked editor also stranded a bound loopback `NWListener` with a live token, one per post opened. `stopMediaHandling()` stops the server and drops both handlers, opening the cycle from the editor's side. It is the host's call to make, because the editor cannot detect its own teardown. An earlier revision inferred it from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that does not work, because `isBeingDismissed` does not propagate down a containment chain and the editor is a child view controller in every real host. Measured across four presentation shapes, the SwiftUI demo, and the real editor: the guard blocked the release every time. Walking ancestors fixes those shapes but still misfires where a container re-parents, and stays silent when a stack is reset under a covered editor. No UIKit callback distinguishes teardown from being covered or re-parented, so this states the contract rather than guessing at it. Stopping now also withdraws the endpoint from the page. The port and token are injected once at document start, and `nativeMediaUploadMiddleware` refuses to retry a failed native upload directly, on the stated assumption that an advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so a stopped server left every image insert failing with a connection error on a working connection — and an uploader host's orphan-cleanup DELETE broken with it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage` copy `getGBKit()` falls back to, and the injected user script that would otherwise restore the dead port at the next document start, including the reload that recovers a terminated WebContent process. Uploads fall back to the default WebView path instead of failing. `MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint, matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason: a value type cannot participate in a reference cycle, so the question doesn't arise. It doesn't prevent the cycle — a struct holding a class reference closes it just as well — but it makes the acyclic shape expressible, and an `AnyObject` protocol taken at `init` reads like an invitation to pass `self`. Source-compatible: every existing class conformer still conforms. A DEBUG-only census counts live upload servers and logs a fault past four. Each live server is a bound loopback listener, one per editor, so monotone growth is this cycle and nothing else produces it — `warmup()` passes no handlers and starts no server. It is the only detectable symptom available: a `deinit` assertion cannot fire, because a cycle is what stops `deinit` from running. Logged, never fatal; the threshold is a heuristic, and crashing a host's debug build over a heuristic is a worse trade than the leak. Both handlers also move into `init` and become `public private(set)`. They only take effect if they are in place before the editor loads, a contract that used to be enforced at runtime by a `precondition` on each setter; taking them at construction makes that failure unrepresentable rather than caught, so `hasStartedLoading` and the fail-fast go with it. The retention rule now lives on the `init` parameters and in the integration guide, since the initializer is the only media call site a host writes. Both also spell out what reuse across editor sessions requires: the editor drops only its own reference when it goes, so a host sharing one handler — the expected shape for an uploader, whose background session or offline queue outlives any editor — has to keep its own. Sharing is the safer shape, since a handler owned by something longer-lived than any editor is a leaf and cannot form the cycle at all. The rule is stated as what it actually is. Nothing in GutenbergKit hands a handler the editor — every value crossing that boundary is a `Sendable` value type — so the cycle is entirely host-authored: it forms only when the object that already holds the editor to drive it also conforms. "Don't conform the object that owns this editor" is the actionable phrasing, and it costs the host nothing, because these methods are called off the main actor and could not have reached that object's state anyway. `deinit` is unchanged and remains the ordinary path: with no cycle, ARC releases the handlers and `deinit` stops the server. BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable; pass them to `EditorViewController.init` instead.
The server reads the delegate three times per request — once at the admission gate (`handlesFile`), then again for `processFile` and `uploadFile` — and those reads are separated by a synchronous disk copy and an unbounded `processFile`. Held weakly, a host that released its delegate in that window changed the answer between reads: a file admitted for processing was forwarded to WordPress unprocessed. Hold it strongly, as Android already does with a plain `val`. Immutable strong references make the three reads agree by construction, and an in-flight upload keeps the delegate alive until it unwinds. The `weak` bought no leak protection to trade away. The cycle it named runs through `EditorViewController.mediaUploadDelegate` — a host object retaining the view controller forms `EditorViewController -> delegate -> EditorViewController` regardless of how this container holds it. What it did buy was the reference vanishing mid-request. So `mediaUploadDelegate` becomes strong too, and the machinery that existed only to police the old contract goes with it: `mediaUploadDelegateWasAssigned` and the released-before-load trap have nothing left to catch, because the editor now owns the delegate for its lifetime. Hosts no longer need to retain it themselves. `UploadContext` becomes a struct and drops its `@unchecked Sendable` opt-out: `MediaUploadDelegate` is `Sendable` and `DefaultMediaUploader` is `@unchecked Sendable`, so it is implicitly Sendable. `doesNotStronglyRetainDelegate` pinned the invariant being removed, so it is replaced by `retainsDelegateForServerLifetime`, asserting both halves — the server owns the delegate while it runs, and releases it afterward. `processesForHostReleasedDelegate` covers the bug directly; against a weak container it fails with the real symptom, `passthroughUploadCalled`. SwiftLint's `weak_delegate` is suppressed with the reasoning inline. The rule is arguably right that the name no longer fits — a later commit renames the property, and the suppression goes away with it.
`mediaUploadDelegate` is a strong `var`, which is only safe while the delegate does not retain the editor back. Assert that the editor still reaches `deinit` — and releases the delegate it owns — so a cycle introduced here fails a test instead of leaking silently. Extracted from the handler-ownership refactor that this branch drops: the server-side handler object lands further up the stack instead, but this half of the contract belongs with the change that creates it.
`NWListener.newConnectionHandler` retains the request handler and, through it, whatever the caller's closure captured — for the upload server, that is now the host's media delegate. `cancel()` does not drop the block: Network.framework holds the listener until cancellation completes on its own queue, so the final release landed there rather than on the thread that called `stop()`. A delegate reached through `EditorViewController.deinit` therefore deallocated off the main thread, measured at 46/50 on the listener's queue — `Timer.invalidate()` and `UIView` teardown in a host's `deinit` are both unsafe there. Clearing it after `cancel()` (not before — the listener is already torn down, so it is never live without a handler) makes teardown synchronous on the caller's thread. `retainsDelegateForServerLifetime` asserts the release outright instead of polling a one-second budget for it.
Silences the `#WeakMutability` warning this declaration emitted on every build — the only warning in the library and test targets.
Both comments claimed the retain cycle is one this code "can neither create nor prevent". Only the second half was true. Flipping `UploadContext` alone, with the property left `weak`, closes the ring through `uploadServer` and leaks the owner — that container's `weak` was its single weak link, so it demonstrably could prevent a cycle. The property is the same story in mirror image: strong here is exactly what lets a delegate that retains the editor back close the shorter ring, and `weak` would rule it out. Neither point argues against the change — the delegate vanishing mid-request is the failure that was actually being hit. But justifying it with a claim that does not hold is how the next investigation into a leaked editor gets misdirected.
`deinitReleasesEditorAndDelegate` passes unchanged against the pre-PR `weak` property, and passes with its `mediaUploadDelegate` assignment deleted outright. `LifetimeProbeDelegate` holds no reference to the editor, so the cycle the failure message names cannot be constructed in the fixture; and the test never touches `view`, so `viewDidLoad` never runs, `startUploadServer()` never runs, and the `UploadContext` this PR changes is never built. The ownership change is covered by `processesForHostReleasedDelegate` and `retainsDelegateForServerLifetime`, both of which fail against a weakly-held delegate with the real symptom. Covering the composite teardown path — editor loaded, server started, editor deallocated — needs a loaded editor and a real listener, which is E2E territory.
The editor holds `mediaUploadDelegate` strongly so an in-flight upload can't
lose it mid-request — losing the delegate mid-request was the failure actually
being hit, and `weak` would rule it out. That is a deliberate trade, and this
commit pays the rest of its cost rather than leaving it in the docs.
A host object that owns the editor and is also its delegate closes a cycle ARC
cannot break, so `deinit` never runs and every editor opened strands a bound
loopback `NWListener` with a live token. `stopMediaHandling()` is the way out:
it stops the server, drops the delegate, and withdraws the endpoint from the
page. It has to be the host's call. Not because UIKit can't report a teardown
— an ancestor walk plus an orphan check at `viewDidDisappear` fires correctly
across fourteen hosting shapes, including this editor's shape in WordPress-iOS
— but because it can't report whether a detachment is *permanent*. A host may
re-present the same editor, and since the call is terminal, guessing wrong
disables media in an editor that survived.
Nothing here hands the delegate the editor: every value crossing that boundary
is a value type. So the cycle is entirely host-authored, and the rule is
narrower than "don't retain the editor" — don't conform the object that owns
it. That costs nothing, because `processFile` runs off the main actor and
could not have reached that object's state anyway.
The delegate also moves into `init` and becomes `public private(set)`. It only
takes effect if it is in place before the editor loads, a contract that used to
be enforced at runtime by a `precondition` on the setter; taking it at
construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it.
Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it. A
stopped server left every image insert failing with a connection error on a
working connection. `revokeNativeUploadEndpoint()` clears the live page, the
`localStorage` copy `getGBKit()` falls back to, and the injected user script
that would otherwise restore the dead port at the next document start.
A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
delegate and starts no server. It is the only detectable symptom: a `deinit`
assertion cannot fire, because a cycle is what stops `deinit` from running.
Answers @dcalhoun's review question on this PR: ownership is the goal, and yes,
an explicit teardown was worth adding.
BREAKING CHANGE: `mediaUploadDelegate` is no longer settable; pass it to
`EditorViewController.init` instead.
Both delivery paths could put bytes on the wire after the editor was gone. `stopMediaHandling()` and `EditorViewController.deinit` both call `stop()`, which cancels the in-flight connection tasks, but Swift cancellation is cooperative: the body read is an uninterruptible loop and a host's `processFile` need not check at all, so a request can reach delivery well after teardown. Whether it then actually reached WordPress rested entirely on URLSession noticing the cancellation. That is not a guarantee the server can rely on. `URLSessionProtocol` is public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack — `withCheckedThrowingContinuation` around a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up. Check cancellation explicitly before delivery in `processAndUpload` and before the passthrough forward, so the guarantee comes from this file rather than from the HTTP client's behavior — and so `stopMediaHandling()`'s documented "any upload in flight is cancelled" holds for every host, not just those on a stock `URLSession`. `uploadErrorResponse` already logs CancellationError quietly, and HTTPServer drops the response for a cancelled task. Not covered by a test: reaching the window deterministically means driving teardown between the parse and the delivery of a live socket request, and a timing-based approximation would be flaky without pinning the behavior.
`retainsDelegateForServerLifetime` names two properties and only tested one. The delegate was bound to a strong local for the whole `do` block, so `#expect(weakDelegate != nil)` was satisfied by that local — the server's ownership was never what the assertion depended on. Confirmed by mutation. With `UploadContext(uploadDelegate: nil, ...)`, so the server holds no reference to the delegate at all, the test **passed**. Nil the host's reference before the assert — the way `processesForHostReleasedDelegate` already does — and the same mutation fails it. The release half was always live and is unchanged: no-op'ing `releaseConnectionHandler()` still fails the trailing `#expect(weakDelegate == nil)`, which is the regression 7124457 added it for. From 8827ba4, earlier in this branch — the commit that introduced the test to pin the strong-ownership fix it could not actually detect.
…legate The server holds the delegate strongly for the duration of a request, so a delegate that holds the server back closes a loop through the listener's captured blocks: `listener -> newConnectionHandler -> handler -> UploadContext -> delegate -> server`. Nothing else in the suite covers that edge — the editor-side tests never start a server, and `retainsDelegateForServerLifetime` uses a leaf delegate, so its release needs nothing to be broken first. Both assertions are load-bearing, confirmed by mutation. Building the context with `uploadDelegate: nil`, so the server holds no reference at all, fails the first: the delegate is freed as soon as the host's local goes out of scope. That stopping resolves the loop at all depends on Network.framework behaviour this package now relies on: for a deployment target of iOS 16 or later (this package requires 17), cancelling an NWListener releases the blocks it captured (rdar://89677097, documented in the macOS 13 release notes). No-op'ing `releaseConnectionHandler()` — the explicit clear added in 7124457 — still frees the delegate, one poll tick later, which is that behaviour doing the work. Before it, the blocks were held for the listener's lifetime. Pinned so a regression, or a lowered deployment target, fails loudly instead of quietly stranding listeners.
`releaseConnectionHandler()`'s doc claimed teardown releases the handler's captures on the caller's thread, unqualified. That holds for an idle server. A request in flight keeps its own copy of what the handler captured until the task unwinds, so a server stopped mid-request releases last on the task's executor no matter what the clear does — and when the editor is the delegate's only owner, that is where the host's `deinit` runs. Stated on each surface a host reads. The `GutenbergKitHTTP` doc scopes its claim to an idle server and drops the sentence naming GutenbergKit's upload server — the package doesn't otherwise know its consumers, and the preceding sentence already covers the effect. `mediaUploadDelegate` says the release can be late and off the main thread. `stopMediaHandling()` stops implying that "cancelled" means "stopped now": cancellation is cooperative, so a `processFile` that ignores it runs to completion and holds the delegate until it returns. Both points from @dcalhoun's review of this PR.
`revokeNativeUploadEndpoint()` passed `completionHandler: nil` and used `try?` on the configuration rebuild, so both of its steps could fail without saying so. Diagnostics, not a fix — the residual risk is narrow. The two plausible failures are self-healing: a terminated WebContent process reloads through `controllerWebContentProcessDidTerminate`, and a page that hasn't loaded yet has no endpoint to withdraw. Both land on the rebuilt user script, which advertises no port. What is left is a live page whose eval failed anyway, holding a port nothing is listening on until the next document start, and a rebuild that threw, leaving that next document start with no `window.GBKit` at all. Neither should be invisible, and the second is the more interesting of the two: the load path lets the same call throw and aborts, so a failure here is strictly quieter than the one the editor already refuses to ignore. It can't route through this file's `evaluate(_:isCritical:)` helper, which hands errors to `handleError` and presents a `UIAlertController`: this runs while the editor is going away.
`startUploadServer()` assigned `self.uploadServer` after awaiting the bind, so a `stopMediaHandling()` landing in that window was undone. The server was stored after the endpoint had already been withdrawn from the page, and because the context captured the delegate before the await, the cycle the call exists to open stayed closed — the property was nil, the server's reference was not. Small window, and nobody is in it today. Binding a loopback listener measures at or under a millisecond (`startAndStop` reports 0.001s); the five-second `defaultStartTimeout` is a ceiling for a listener that can't become ready, not a typical wait. `stopMediaHandling()` is new in this branch and has no callers, and WordPress-iOS sets no delegate at all. Reaching this needs a host that adopts the retaining shape the docs discourage and then tears the editor down inside that millisecond. Worth four lines anyway. `startUploadServer()` has exactly one call site and runs at most once per editor, so with the guard, "terminal" is a property of the code rather than of how fast a listener binds. Not covered by a test: suspending a real bind mid-flight is the only way into the window, and a timing-based approximation would pass whether or not it got there.
`DefaultMediaUploader` reads as an implementation of a host-facing protocol — the "default" one, as against a host's. It is not. It is GutenbergKit's own HTTP client for the configured site: it performs the uploads no host took over, and it relays every media delete, because the editor only ever asks to delete `/wp/v2/media/<id>` on the configured site. Rename it, and the `defaultUploader` parameters and properties that carry it, on both platforms. Sweep the prose and error strings that used the retired vocabulary too, including the `UploadContext` doc header and Android's three media-client messages. The host-facing docs still say "the default uploader" as a role: `InternalMediaClient` is internal on both platforms, so naming it in prose a host reads would be worse. On iOS this also narrows two signatures. `passthroughResponse` and `handleDelete` took the whole `UploadContext` and touched only the client. Pass it directly. On the delete path that is more than tidiness: a deletion always relays to the configured site, never to a delegate. That was a convention the signature let you break; now the type won't. The three functions that keep the context genuinely need every field. Android's server holds the client as a constructor property rather than threading a context, so it needs the rename only — and because its `handleDelete` is an instance method with the delegate in scope, the delete-path convention stays a convention there. The type-level guarantee is iOS-only.
* feat: add MediaUploader, for a host that owns the whole upload
Performing a media upload — and retrying it — should be a single,
all-or-nothing responsibility: either GutenbergKit performs the upload and
owns its retries, or the host does. Both go to the same configured site;
the only difference is who executes the requests.
`MediaUploadDelegate.uploadFile` doesn't offer that. A host performs the
`POST /wp/v2/media` and returns the raw response it received — then the
editor, reading that response, drives the `post-process` retries and the
orphan cleanup behind it, through the WebView rather than the host's
stack. A host that took over uploads to run them through its own
networking still didn't own the retries. It also receives no form fields,
so an attachment it uploads lands unattached to its post.
Add `MediaUploader`, which owns the upload end to end:
- `upload(_:)` returns the finished attachment or throws. There is no raw
response left for the editor to retry behind it, so the host drives its
own post-process recovery and force-deletes its own orphan on terminal
failure.
- It receives a `MediaUpload` carrying the file, its metadata, the
editor's non-file form fields (`post`, additionalData) and the request
query (`?_embed`) — everything needed to reproduce a native request.
- Fields are a `MediaUploadField` list rather than a dictionary, so
repeated names (a `field[]` array) survive verbatim and in order.
Additive for hosts: `uploadFile` still works and is marked deprecated,
pointing them at the replacement, and an uploader takes precedence when
both are set. Internally the upload server's startup gate widens to admit
an uploader as well as a delegate. GutenbergKit's own build keeps one
deprecation warning at the call site that supports the old hook — the
marker exists to tell hosts to migrate, and supporting the hook until it
is removed means calling it.
With an uploader set, the delegate's metadata gate can no longer decline a
file: the gate exists to skip a temp copy for a file the delegate won't
touch, but an uploader takes over delivery for *every* file, so passing
through would silently bypass it. Covered on both platforms.
`MediaUploadServerTest` crosses Detekt's LargeClass threshold; baselined
rather than split, which is its own change.
* fix: don't hand a declined file to processFile, and close the gaps around it
Follow-ups to the MediaUploader commit: a behavior bug in the metadata gate, a
cross-platform divergence, a missing cancellation check on Android, four
documentation defects, two test gaps, and a shadowed local.
- `processFile` ran on a file the delegate's metadata gate had declined. Widening
the gate to `uploader != nil || delegateWantsFile` left `processFile` called
unconditionally, so an image-only delegate paired with an uploader was handed
the `.mov` it had just said it won't touch — breaking the contract
`handlesFile` documents. `delegateWantsFile` is now carried into
`processAndUpload` and gates `processFile`. With an uploader set the file is
still delivered; it just skips processing on the way.
- iOS evaluated `handlesFile` eagerly while Android's `&&` short-circuited past
it, so the same host saw one callback per upload on iOS and zero on Android.
Android now binds it eagerly too: asked exactly once per upload on both.
- Android had no pre-flight cancellation check before handing work to the host
uploader, where iOS has `Task.checkCancellation()`. Added
`currentCoroutineContext().ensureActive()`, so a torn-down editor no longer
starts an upload whose attachment nobody would clean up.
- The recovery recipe omitted `post-process`'s required `action` parameter. Core
registers `action` as required, so a host following the doc verbatim would 400
five times and then run the doc's *other* instruction —
`DELETE /wp/v2/media/<id>?force=true` — destroying an attachment
`wp_update_image_subsizes()` would have recovered.
- `mediaUploader`'s doc had been appended to `mediaUploadDelegate`'s `///` block,
merging the two: `mediaUploadDelegate` shipped with no documentation and
`mediaUploader` opened by describing a delegate. Confirmed with
`swiftc -emit-symbol-graph` (`mediaUploadDelegate => None`); both now bind
their own 11 lines.
- `formFields` and `deprecatedUploadFile` were inserted between
`attachmentId(fromPath:)`'s doc and its declaration — merging into it on iOS,
dropping it outright on Android — costing the "deliberately narrow, not a
general REST proxy" rationale. Moved below their only caller, per AGENTS.md's
call-order rule.
- `ReplaceWith("MediaUploader")` takes a replacement *expression*; applying the
quick-fix drops all three arguments and leaves a type name where a
`MediaUploadResponse?` was expected. Removed, with a note so it doesn't return.
- Nothing pinned the Android gate or the uploader/deprecated-hook precedence:
deleting `&& mediaUploader == null` or reordering the two delivery paths left
the suite green. Three tests added; both mutations now fail.
- `DecliningDelegate` duplicated the pre-existing `DeclineByMetadataDelegate`
minus its `processFileCalled` recorder — the one probe that catches the
`processFile` bug above. Merged, and the declined-file test now asserts it.
- Two locals named `uploader` shadowed the new `MediaUploader` property, silently
(kotlinc has no diagnostic for it, detekt no rule). Renamed to `client`.
- `RecordingUploader`'s fixture carried no `title`, so the repo's only worked
example of an uploader result was a body that trips `transformAttachment`.
* feat!: remove MediaUploadDelegate.uploadFile `MediaUploader` replaces it. Returning a raw response split one upload's HTTP across two owners — the host performed the `POST`, the editor drove the `post-process` retries and orphan cleanup behind it — and the hook received no form fields, so an attachment it uploaded landed unattached to its post. Neither is fixable while the hook returns a raw response, which is what the replacement changes. What is left is a clean division: a delegate transforms bytes and GutenbergKit owns delivery and its retries; a `MediaUploader` owns delivery and its retries entirely. There is no longer an in-between where the host performs the upload but the editor retries it. `handlesFile` no longer gates the temp copy for two callers, only for `processFile` — and only when no uploader is set, since an uploader takes over delivery for every file. `MediaUploadResponse` drops to internal on both platforms: `uploadFile` was the only public API that named it. BREAKING CHANGE: hosts implementing `uploadFile` must conform to `MediaUploader` instead. Hosts that only implement `processFile` / `handlesFile` are unaffected. * docs: correct the handlesFile contract and the delegate's stale upload docs Review follow-ups to 6bcc210. No behavior change. `handlesFile`'s new doc said it is "only consulted when no `MediaUploader` is set". It is always consulted (`MediaUploadServer.swift:143`, `.kt:374`), and it still gates `processFile` (`.swift:306`, `.kt:534`) — a declined file reaches the uploader unprocessed. The implementation comment 160 lines away and the `an uploader sees a file the delegate's metadata gate would have declined` test on both platforms already said so. Replaced with wording lifted from that comment. Removing `uploadFile` also left the docs a host actually reads still advertising it: - The `mediaUploadDelegate` property summaries — what Xcode Quick Help and IDE hover show — said "customizing media file processing and upload behavior" (iOS) and "(resize, transcode, custom upload)" (Android). Both now describe transformation and point at `mediaUploader` for the upload case. - `MediaUploadResponse.statusCode` claimed the status could come from "the host's upload service". `MediaUploader.upload` returns `Data`, so the host path supplies a literal 201. - `MediaUploadServer`'s parameter docs, the `UploadResult.uploaded` doc, and Android's "won't process or upload" comment, whose iOS twin already read "won't process". Two non-doc changes ride along: - `UploadError.noUploader`'s message named a role the delegate no longer has: "No upload delegate or internal media client configured" becomes "No media uploader or ...". It reaches the editor in a 500 body; nothing asserts on it. - iOS's `MockUploadDelegate` became a duplicate of `ProcessOnlyDelegate` once `uploadFile` went. Android already consolidated on `ProcessOnlyDelegate`; iOS now matches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… class bound (#685) * refactor!: rename MediaUploadDelegate to MediaProcessor The protocol no longer uploads anything — the previous commit removed `uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now describes the one thing it can't do, and next to `MediaUploader` the two names read as variations on the same job rather than the two halves of a deliberate split. `MediaProcessor` says what is left: it transforms bytes, GutenbergKit delivers them. Mechanical throughout — the property becomes `mediaProcessor`, the server parameter `processor`, the file `MediaHandlers.swift` (it holds both protocols now), and Android's demo `DemoMediaProcessor`. Prose follows the types. The `weak_delegate` suppression added when the property became strong goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and `MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes beyond the name. * refactor(ios)!: drop the class requirement from the media protocols `MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and `EditorViewController` holds both strongly. A conformer that holds the view controller back therefore closes a retain cycle ARC cannot break: the editor is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only caller — never runs either, and a bound loopback `NWListener` outlives the editing session. Nothing needed class-boundness. There is no `weak`, `===`, or `ObjectIdentifier` use against either protocol anywhere in the tree, and every existing conformer is a class, which conforms unchanged. Dropping the requirement lets a host conform with a value type capturing only what the work needs — the shape that avoids the cycle, and the one a class-bound `Delegate` discouraged. This does not make the cycle impossible: a struct that stores the view controller cycles just the same. The docs say so rather than implying the type system settles it. * fix(ios): name every media handler in the leak census, not just the first `countServerStarted` resolved its name as `processor.map { … } ?? uploader.map { … }`, so with both supplied it always named the processor. The retainer is as likely to be the uploader — and after the class bound came off `MediaProcessor`, the processor it names may be a value type holding nothing at all, which is the one shape that provably cannot close the cycle the fault is reporting. A host following the docs hits this on the recommended shape: a leaf processor for the transform plus an uploader on the coordinator that owns the editor. The fault named the leaf, so the reader audits an object with no stored references, finds nothing, and concludes the census is broken. Names every handler that was supplied, and softens the assertion from "is its own media handler" to "is one of its own media handlers" — with two names it is a candidate list, not an accusation. DEBUG-only, and still behind the `count >= liveServerLeakThreshold` guard, so `String(describing: type(of:))` stays off the start path. * fix(ios): start the upload server for a host that supplies only an uploader `startUploadServer()` asks "did the host supply a media handler" twice — once before starting, once after the bind returns, because `stopMediaHandling()` can land while that `await` is suspended. The two reads had drifted. #628 widened the first to `delegate != nil || uploader != nil` and left the second checking the delegate alone. So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback listener, then failed the post-bind check and stopped the server it had just started. `uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort: nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)` was **never called, for any file** — no error, no log. Uploads appeared to work; they just never reached the host's background session, offline queue, or retry policy, which is the whole reason to supply an uploader. Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That is the actual defect — two hand-maintained copies of one predicate — and it is the same failure `MediaServerCredentials` was extracted for, where a check "diverged silently between iOS and Android once". `uploadServer` and `startUploadServer()` become internal so the suite can reach them. The test is parameterized over uploader-only, processor-only and both. Mutation-checked: restoring the old post-bind guard fails **only** the uploader-only case, which is the regression and nothing else. Android already pinned this gate (`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no delegate"); iOS had no equivalent, which is why the drift survived three commits green. * test: finish the rename in the media suites' own vocabulary The rename swept the helper types and left the names around them. 101 sites across four files: iOS test functions and `@Test` display strings, Kotlin backtick names, `let delegate = ProcessOnlyProcessor()` bindings that contradicted themselves on one line, `weakDelegate`, and a `// MARK: - Upload with delegate` header over code the production file had already renamed to `// MARK: - Processor Pipeline`. These are the strings CI prints. A red build named `retainsDelegateForServerLifetime` or `processes with the delegate, then delivers through the internal client` for a codebase where no symbol contains the word — Kotlin backticks are literally the JUnit report strings — so the first move on a failure was to grep for an API this stack deleted. Safe as a plain substring replacement: none of the four files reference a genuine delegate. `HttpServerDelegate` and `EditorViewControllerDelegate` live in other test files and are untouched. Two names would have read as stutters after a mechanical pass, so they say what the test does instead: `processesThenDelivers` and `processorRunsForUploader`. Test counts are unchanged — 590 iOS, and Android green on `--rerun-tasks` — so this renames tests rather than adding or dropping any. Note it does reset Buildkite Test Analytics history for the renamed cases, which is the deliberate cost. * test(ios): pin that a value type can conform to MediaProcessor Dropping `: AnyObject` is what the second commit here exists to deliver, and nothing exercised it — all eleven conformers in the tree were classes, so the boxed-existential path was never walked: copied into `UploadContext`, captured by the `@Sendable` handler closure, read again at `processFile`. Re-imposing the class bound, or breaking that path, would have compiled and passed green and surfaced only in a host's build. It now fails at compile time: error: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor' `ValueTypeProcessor` is `Sendable` without `@unchecked` — also the point, since that is the shape the protocol's documentation now recommends and the escape hatch it describes. The assertions run through `MockInternalMediaClient`'s recorded metadata rather than state on the processor, because a `struct` witnessing a non-mutating requirement cannot record anything. The transcoded mimeType and filename reaching the client could only come from `processFile` having actually run, so this pins invocation, not just storage.
…686) `MediaUploadServer` handled requests through static functions threading an `UploadContext` parameter through every call, because the closure form of `HTTPServer.start` can't capture the object that owns the server: the closure has to exist before the server does, and capturing `self` would form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer`, so `deinit` — and its `stop()` — would never run. Add an `HTTPRequestHandler` protocol to `GutenbergKitHTTP` and a `start` overload that takes one, then serve `MediaUploadServer` from it. The dependencies become stored properties on a `Handler` struct and the request logic becomes instance methods. The closure overload is unchanged and forwards to the same code path, so the addition is purely additive — no existing caller, test, or the debug server is affected. Request handling is mandatory, so it can't be a defaulted `HTTPServerDelegate` method the way optional customization points are; hence an overload rather than a new delegate requirement. The protocol is deliberately not `AnyObject`-constrained so a handler *can* be a struct holding only what it needs — not because a struct is safe by construction. A value type is not protection: the server captures the handler into a heap node, so a struct storing the server's owner closes the same ring a class would. Both shapes work, under the same leaf discipline `HTTPServerDelegate` already documents — a handler must not strongly hold the object that owns the server. `Handler` stores no reference back to the `MediaUploadServer`, which is why the helpers outside it stay static. Mechanically: `handleRequest` becomes `handle`, the functions that use the dependencies become instance methods, and the ones that don't (`attachmentId`, `relayResponse`, `uploadErrorResponse`, `formFields`) stay static. `UploadContext` goes away — `Handler` is what it was. Helpers outside the handler (`errorResponse`, `writeStream`, `sanitizeFilename`, `uploadsTempDirectory`) are qualified rather than moved. No behavior change.
Setting a `mediaUploader` means the host is taking over uploads. With no site credentials the server would previously just not start, silently dropping the uploader — and its media deletes still need the internal media client to reach the configured site, since every attachment lives there no matter who delivered it. So the behavior forks by intent. A `mediaProcessor` with no credentials leaves the server down and uploads fall to the default WebView path — there is nothing to deliver through, so nothing to process. A `mediaUploader` with no credentials is a configuration error and fails fast: `precondition` on iOS, `check` on Android. The check runs where the host states its intent, not at page load. iOS takes its handlers at `init` and holds them `private(set)`, so a non-nil uploader at load time was necessarily passed at construction — checking there puts the caller's own line in the stack trace instead of surfacing the mistake from inside a page-load callback that names only GutenbergKit. Android still takes its handlers as mutable properties, so the earliest equivalent point is the `mediaUploader` setter, beside the existing set-before-load `check`. This is the shape `359d89ad` already established for the set-before-load contract: enforce the rule where the host states its intent. Android gains a `MediaServerCredentials` of its own, mirroring iOS's. The two predicates had diverged — iOS required an absolute site root while Android checked only `isEmpty()` — so `siteApiRoot = "example.com/wp-json/"`, which is what a user types when asked for their site address, trapped on iOS and started a doomed server on Android. Every relayed delete then threw `IllegalArgumentException` out of OkHttp's `.url()`, which is not an `IOException`, so it escaped `handleDelete` and degraded to a plain-text 500 the editor cannot parse — orphan cleanup failing silently. Both sides now test scheme and host. Emptiness is tested alongside nullity because `Uri` and `URL` disagree on a missing authority: `file:///tmp/wp-json` yields a null host on iOS and an empty one on Android. The policies live outside the view types on both platforms so they are reachable from the host test suites. On iOS that is load-bearing: `EditorViewController` is `#if canImport(UIKit)` and therefore absent from the macOS host, the one platform that can run Swift Testing's exit tests, so the trap itself is testable rather than only the predicate. The two suites assert matching cases on purpose — this policy has diverged silently once, and matching cases make the next divergence a failing test rather than a crash on one platform and a broken server on the other. A `mediaUploader` can still be dropped without failing, at the cleartext guard: an app that has not permitted cleartext to localhost never reaches the loopback server, so the uploader is never called. That one logs and degrades rather than failing, and the distinction is the cause rather than the symptom — missing credentials is an incoherent configuration, while blocked cleartext is a sound configuration the app's network policy blocks, and permitting cleartext makes the same setup work unchanged. Nothing had told integrators to permit it, so `docs/integration.md` now does, including why the library cannot ship the config itself: `networkSecurityConfig` is a single-valued `<application>` attribute, so a library declaring it fails the manifest merge against the host's and against other libraries that declare one — `rs.wordpress.api` already does.
`formFields` decodes every non-file form part as UTF-8. That can only be lossless because of who is on the other end, and nothing in the code enforces it -- so write it down on both platforms, in plain terms: only the editor's own page can reach the server, the browser guarantees form-field text is valid Unicode, and raw bytes always arrive carrying a filename, which routes them to the file rather than to a field. Pin the third condition with tests, since it is the one this code could break on its own. The bodies are ordered the way `uploadToServer` actually emits them -- file first, then additionalData -- and assert both the uploaded filename and the decoded fields, so the suite fails if either the partition or the file-selection rule changes. A second test covers the other half: valid UTF-8 round-trips, so real captions and titles are unaffected. The `buildMultipart` comments also claimed raw bytes were appended so a non-UTF-8 value would be "forwarded verbatim rather than coerced". That is not why -- such a value cannot reach them. On iOS they avoid a failable `String(data:encoding:)` whose `?? ""` would quietly drop a whole field; on both platforms they keep the re-encode byte-for-byte identical to the passthrough it replaces.
…ck (#689) `TranscodingProcessor` duplicated `ResizingProcessor` — same `.processed(_, mimeType: "video/mp4", filename: "clip.mp4")` result, one call site — and was the weaker of the two. It wrote to a fixed `$TMPDIR/clip.mp4` instead of a per-call UUID path inside the managed upload directory, and swallowed the write with `try?`, so a failed write still returned `.processed(<nonexistent URL>, …)` and the test passed green against a file that never existed. `ResizingProcessor` uses `try` and a unique path. Also drops `@unchecked Sendable` from `ThrowingUploader`, which has no stored properties and so satisfies `MediaUploader`'s inherited `Sendable` conformance on its own. The escape hatch is only needed by the mocks holding `NSLock`-guarded state; carrying it on a stateless one normalizes it as boilerplate, which is how an unsynchronized property gets added later without a diagnostic. `ContentTypeDeleteClient` keeps it — it subclasses `InternalMediaClient`, itself an `@unchecked Sendable` class, and must restate the conformance.
#651) `viewDidDisappear` cancelled `dependencyTaskHandle`, the async editor dependency fetch. That callback fires whenever the editor is merely covered — a full-screen modal presented over it, a push on top of it, a tab switch — and the fetch has exactly one starting point, the "no dependencies" branch of `viewDidLoad`, with nothing that restarts it. Cover a still-loading editor that way and the load is over for good: with the fetch parked mid-flight and `viewDidDisappear` delivered, the simulator shows the progress view replaced by the load-error screen and the host told `didFailToLoad` with a cancellation error. Coming back to the editor does nothing. The fast path a few lines above already carried the fix for this class of failure — the same cancellation landing mid `startUploadServer()` silently disabled native uploads for the session (#357) — but the async path never got the same treatment. Its task ends in the same `loadEditor()`, so that reason covers it too; its comment now says so, along with its own: nothing restarts the fetch. Stop cancelling rather than cancel-and-restart. A restart path would have to be idempotent and not race a fetch already in flight — complexity with nothing to buy. `deinit` is not an alternative home for the cancellation either, which is why `dependencyTaskHandle` goes away with the override rather than moving there. The task body is `await self?.prepareEditor()`, and optional- chaining a weak `self` into an async call holds a *strong* `self` across every suspension inside it, so the editor cannot be deallocated while the fetch is running. `deinit` is reachable only once the task has already finished, where there is nothing left to cancel. Not cancelling has a cost. The same retain keeps an editor released mid-fetch alive until the fetch and the load after it finish, which only URL timeouts bound. Meanwhile it keeps writing to the site's caches, and once the fetch lands it binds its upload server: a host that retains its own editor strands one more listener, and the DEBUG leak census can fire on a slow network. `[weak self]` still makes a task that has not started yet a no-op on an editor released first. Gating the cancellation on `isBeingDismissed`/`isMovingFromParent` was not an option. Hosts install this controller as a child, so UIKit sets those flags on an ancestor and they read `false` here — the gate would never fire, which is this change with a misleading condition on top. `EditorViewControllerLifecycleTests` pins both halves: covering the editor leaves the fetch running, and the fetch holds the editor alive until it finishes and releases it then. Against the old code the first fails with the real symptom, a cancelled request. The tests inject a `URLSessionProtocol` that holds every request until released, so the editor runs its real fetch path, and cover the editor through `beginAppearanceTransition`/`endAppearanceTransition` — `begin` alone never delivers `viewDidDisappear`. Each uses a fresh site host and deletes what it wrote, since `EditorViewController` can't be pointed at a temporary directory.
b8d49ad to
23451f0
Compare
…iddleware' into fix/own-media-delegate-strongly
…g stops `revokeNativeUploadEndpoint()` cleared the endpoint from three places: the live page, the injected user script, and the `localStorage` copy of `GBKit` that `getGBKit()` fell back to. #613 removed that copy — `getGBKit()` reads `window.GBKit` alone, and the document-start script now removes the key so nothing session-scoped persists across launches. Merged together, the revoke no longer cleared anything there. It created the key instead: `JSON.parse(localStorage.getItem('GBKit') || '{}')` found nothing, so it stored `{"nativeUploadPort":null,"nativeUploadToken":null}`. No credential, and the next document start removed it again, but it is a write to storage #613 exists to keep empty, for a reader that is gone. Drop the block. Two copies hold the endpoint now, and the doc says so.
The last ten lines of the protocol's doc comment restated the two paragraphs above them — the `Sendable` requirement on a captured reference and the two `struct` mistakes that compile silently — starting mid-sentence after "a processor that is never called". Xcode Quick Help rendered both copies.
…701) (#752) * fix(ios): free editors mid-fetch, and share site requests in flight Follow-ups to keeping the fetch running, from reviewing #651: - The async dependency fetch no longer holds its editor. - A cancelled asset bundle build is never published. - Every cache for a site shares one SQLite store. - Identical requests and bundle builds in flight are shared. The fetch held its editor for as long as it ran: `await self?.prepareEditor()` optional-chains a weak `self` into an async call, which holds a strong `self` across every suspension inside it. A host that released the editor mid-fetch didn't free it until the fetch ended, and in between the full load tail — bundle provider, upload server bind, `loadFileURL` — still ran on a controller nobody held. The fetch now belongs to an `EditorDependencyLoader`, and the editor never awaits it. The editor owns the loader; the loader reaches back only through a `weak let delegate` whose requirements are all synchronous, so nothing it calls can suspend while holding the editor. A released editor is freed at once and nothing runs on it, while the fetch, still never cancelled, runs on and warms the cache for the next editor. The task starts from `fetch(from:)` rather than `init`, where a bare `delegate` would resolve to the strong parameter instead of the weak property. `prepareEditor()` goes away: the async flow is now "fetch, then the fast path", through `startLoadingEditor(dependencies:)`, which also takes over the #357 note about cancelling mid-`startUploadServer()`. The progress view now fades out as the load starts, rather than after `loadEditor` returns. `EditorAssetLibrary.buildBundle` published bundles from a cancelled build. Its task group swallows every per-asset failure, cancellation included, so a cancelled build reached `bundle.copy(to:)` with assets missing — and `readAssetBundles()` reads only the manifest, so every later launch served the gap. It now checks for cancellation before publishing. WordPress-iOS's `EditorDependencyManager._invalidate` can reach this today: it cancels an in-flight prefetch and purges without waiting for the task to finish. Every `EditorService` builds its own `EditorURLCache`, and each opened its own `SQLiteKVCache` on the site's `editorurlcache.sqlite` — which the store documents as undefined behavior, and measured, it is worse than contention. `connection()` opens lazily and caches the result, failure included, for the life of the instance, and nothing set a busy timeout. Two caches making their first read at the same moment left at least one of them broken in 50 runs out of 50, every later read and store throwing `databaseUnavailable`. Opened one after the other and then written concurrently, 189 of 400 writes still failed; through one instance, none did. That is the shape of WordPress-iOS's launch — `warmUpEditor(for:)` starts the warmup editor's fetch and the prefetch together, each with its own service — and a broken cache fails `prepare()` outright, since a read error is not a network error. Not reproduced in WordPress-iOS itself. `SQLiteKVCache.shared(handle:directory:diskCapacity:)` now hands every caller the live instance for its file, held weakly so a file no one is using is closed as before, and asserts that callers sharing a file ask for the same capacity. Being weak, it can hand out a fresh instance while the last one's `deinit` is still checkpointing the WAL, so the store now also sets a 5s busy timeout: reopening in that window failed 200 times in 200 without it, and never with it. The timeout doesn't replace `shared`. With it set, two instances opening at once still break one, because the switch to WAL returns `SQLITE_BUSY` without waiting on it. Nothing site-level was shared while in flight, so an editor opened mid-prefetch repeated the prefetch's requests and its bundle build, splitting the bandwidth the prefetch needed. Sharing now happens at the level of what goes over the wire and what lands on disk, which needs no analysis of the editor configuration: - `EditorHTTPClient.perform(_:)` joins an identical request already in flight. The key is the request as configured — URL, method, and headers, auth included — plus the session, and the timeout and network service type, which `URLRequest`'s own `==` ignores (measured). Only safe requests without a body are shared, and only from clients no delegate is watching. The table is process-wide, and since `EditorHTTPClient` is public, that includes a host's own GETs. - `EditorAssetLibrary.buildBundle(for:)` joins a build in flight for the same directory: storage root and manifest checksum. That holds whatever client either library has. The build downloads over the client of the library that started it, but the bundle is shared by site once it's on disk anyway, and builds kept apart by client race into the same directory through `copy(to:)`, where one can fail. Both go through `InFlightTasks`: cancelling a caller ends only that caller's wait, and shared work stops once no caller is left waiting on it. A caller that joins raises the work to its own priority, since waiting on a continuation doesn't escalate it the way awaiting `task.value` would; that needs iOS 26 or macOS 26. An editor opened mid-prefetch now joins the settings, theme, site settings, post types, and bundle build already in flight. It still fetches its own post, and the `editor-assets` manifest: that isn't cached, and the prefetch's request for it has usually finished by the time its build is running. The shared store is what makes this safe: a shared response reaches every waiter at the same instant, and each writes it through its own `EditorURLCache`. A shared task reports progress to its waiters one at a time, and a waiter can leave while an earlier one's callback is suspended. So `InFlightTasks` checks each waiter is still waiting just before its turn, and `EditorService.incrementProgress` drops progress that arrives after its `prepare()` has cleared it, rather than trapping on a precondition. Neither is enough alone: a call already under way when its caller leaves can't be recalled. With only the old precondition, a shared build reporting to a service whose `prepare()` had given up trapped, reproduced with two services sharing a build. The guard also fixes an older trap: two overlapping `prepare()` calls on one service, where the first to finish clears progress the second is still reporting. `theInFlightFetchKeepsTheEditorAlive` flips to `releasingTheEditorMidFetchFreesIt`: against the previous commit the editor is still alive after 2s; it now passes in 0.36s. Each of these fails against the code it pins: `buildBundlePublishesNothingWhenCancelled` with the cancelled bundle on disk, both new `EditorURLCacheTests` with one store per cache, `sharedReopensAFileWhileItCloses` without the busy timeout, `aCallerThatHasLeftHearsNoMoreProgress` without the re-check, `overlappingPrepareCallsDontTrap` without the guard, and `aHigherPriorityCallerRaisesTheTask` without the escalation. Mutation-tested too: a loader holding its delegate across the `await`, never sharing requests, and keying builds per library rather than per directory are each caught. `ParkedURLSession` moves to `Helpers/` so these suites can share it. * fix(ios): don't share a failed cache store, or the request for a post Follow-ups from review of the sharing this branch introduces. `SQLiteKVCache.shared` went on handing out an instance that could no longer work, to every caller for as long as anything held it: - One whose open failed. An instance keeps that failure for life, so a passing fault — a full disk, an I/O error — failed every later `prepare()` for the site, where each service used to get its own attempt. A failed instance now takes itself out of the registry. It has closed its handle, so the next caller's instance has the file to itself. `shared` doesn't ask the instance whether it failed: that would wait on `openLock`, held for the whole open and so for as long as the busy timeout, on the main thread where an editor builds its service. - One whose file `EditorViewController.deleteAllData()` had deleted. Both `get` and `put` through it throw "disk I/O error (code 10)", so the next editor failed to load rather than starting from an empty cache. `deleteAllData()` now goes through `EditorURLCache.deleteAll(in:)`, which stops sharing every store under the directory it removes. The stale instance closing later leaves the new file alone: 20 entries of 20 written beside it survived. `RESTAPIRepository.fetchPost` went through the shared `perform(_:)`, so an editor reopened on a post joined the GET an editor since closed still had in flight for it — a response that can predate an edit made in between. The post is deliberately never cached, and is now never shared either: `perform(_:)` sends a request alone when its cache policy asks to skip the cache, and the post request asks. A host's own requests through `EditorHTTPClient` can opt out the same way. `buildBundlePublishesNothingWhenCancelled` checked the disk as soon as its caller's wait ended, and `InFlightTasks` ends that wait before it cancels the build. With `try Task.checkCancellation()` removed the test still passed in 100 runs of 100 run one at a time, the bundle landing on disk moments later. It now waits for the abandoned build through a `task(for:)` test hook, and fails against that mutant in 20 runs of 20. `deliversTheError` gains the `defer` that `ParkedURLSession.release()` asks of every test. Each new test fails without its fix: a failed instance left in the registry, `deleteAll` not forgetting its stores, the forgotten prefix matching a sibling directory, the client ignoring the cache policy, and the post request keeping the default one.
Replaces
MediaUploadDelegatewith two host-facing protocols on both platforms: aMediaProcessortransforms a file before GutenbergKit uploads it, and aMediaUploaderperforms the whole upload on the host's own stack. On iOS the editor now takes both atinitand holds them strongly, so a host that releases its handler mid-upload no longer has the file delivered to WordPress unprocessed.Stacked on #594. This is the work from #621: the PRs it was split into (#682 through #689, and #651) merged into this branch, so it is reviewed here as one change. #669 is stacked on top.
Summary
init, as Android already does with a plain property.stopMediaHandling()for the retain cycle that ownership makes possible, and withdraw the upload endpoint from the page whenever the server stops.MediaUploaderand removeMediaUploadDelegate.uploadFile, so either GutenbergKit or the host owns an upload and its retries — never half each.MediaUploadDelegatetoMediaProcessor, and drop the class requirement from both protocols on iOS.mediaUploaderis supplied without usable site credentials, on both platforms, instead of silently never calling it.Why
A released delegate changed the answer mid-request
The upload server consults the host's handler more than once per request:
handlesFilebefore it copies the upload to disk, thenprocessFileafterwards. iOS held the handler weakly, so a host releasing it between those two reads changed the answer — a file admitted for processing was forwarded untouched.processesForHostReleasedProcessorreproduces it; against a weakly-held handler it fails withpassthroughUploadCalled.Holding it strongly makes the reads agree by construction. That is a trade rather than a free fix:
weakalso ruled out a retain cycle, and strong does not. A host object that owns the editor and is also its processor formshost → editor → processor → host,deinitnever runs, and every editor opened strands a bound loopbackNWListener. Most of the iOS work below exists to pay for that.uploadFilesplit one upload across two ownersA host implementing
uploadFileperformed thePOST /wp/v2/mediaand returned the raw response. The editor then drove thepost-processretries and the cleanup of a failed attachment behind it — through the WebView, not the host's stack. The hook also received no form fields, so an attachment it uploaded was never attached to its post. Neither is fixable while the hook returns a raw response.How it works
1. Ownership and teardown (iOS)
mediaProcessorandmediaUploaderareinitparameters, andpublic private(set). A handler only ever took effect if it was in place before the editor loaded, which apreconditionon the setter enforced at runtime. Taking it at construction removes that failure instead of catching it.stopMediaHandling()stops the server, drops the editor's references, and withdraws the endpoint from the page. It is terminal, and only needed by a host whose handler holds the editor back;deinitdoes the same work for everyone else.HTTPServer.stop()clears the listener'snewConnectionHandler.cancel()alone left the final release to Network.framework's queue, and a handler reached throughEditorViewController.deinitdeallocated off the main thread in 46 of 50 measured runs. Release is now synchronous for an idle server. A request in flight holds its own reference until it unwinds, which the docs on the property, onstopMediaHandling()and onHTTPServerall state.URLSessionProtocolis public, and a conformance that wraps a completion handler has no cancellation awareness. Android gains the same check before it calls a host uploader.structthat captures only what the work needs. A value type is not protection by itself — astructthat stores the view controller forms the same cycle — and the docs say so.deinitassertion cannot report this cycle, because the cycle is what stopsdeinitfrom running.2. Processor or uploader (iOS and Android)
MediaProcessoronly changes bytes. GutenbergKit delivers the result and keeps the retries and cleanup.MediaUploaderowns delivery end to end. It receives aMediaUpload— the file, its metadata, the editor's non-file form fields in order (postamong them) and the request query — and returns the finished attachment JSON or throws. The host runs its ownpost-processrecovery and deletes its own failed attachment; the protocol docs carry the recipe, including theactionparameter core requires.handlesFilestill reaches the uploader, unprocessed.3. Credentials
An uploader's deletes need the site root and auth header, so an uploader without them is a configuration that cannot work. iOS traps in
initand Android throws from themediaUploadersetter, which puts the host's own line in the stack trace. A processor without credentials is not an error: the server stays down and uploads take the WebView path.The two platforms' checks had diverged. Android tested only for an empty root, so
example.com/wp-json/started a server whose every relayed delete threw. Both now require a scheme and a host, and the two test suites assert matching cases.Android hosts must also permit cleartext to
localhost, or the handlers are never called.docs/integration.mdnow documents the entry, and why the library cannot ship it.4. The dependency fetch survives the editor being covered (iOS)
viewDidDisappearcancelled the async dependency fetch. That callback fires when the editor is merely covered — a modal, a push, a tab switch — and nothing restarts the fetch, so the editor showed the load-error screen and stayed there. The cancellation is removed rather than moved todeinit: the fetch task holds the editor strongly while it runs, sodeinitis unreachable until there is nothing left to cancel.5. Internal
GutenbergKitHTTPgains anHTTPRequestHandlerprotocol and astartoverload that takes one. The closure overload is unchanged.MediaUploadServeris now served from aHandlerstruct with stored dependencies, instead of static functions passing a context parameter through every call.DefaultMediaUploaderis renamedInternalMediaClienton both platforms. It is GutenbergKit's HTTP client for the configured site, not a default implementation of a host protocol.What we explored
weakand read the handler once per request (raised in review). It makes the reads agree, but leavesnilambiguous between "never configured" and "the host released it", frees a handler constructed inline at the call site immediately, and cannot hold a value-type conformer.stopMediaHandling(). UIKit can report it: an ancestor walk, plus a check atviewDidDisappearthat the editor has no parent, presenter or window, fired on every dismissal and pop across fourteen hosting shapes, with no false positive on the editor being covered. What it cannot report is whether a detachment is permanent. The call is terminal, so guessing wrong disables media in an editor that survived.isBeingDismissed/isMovingFromParent. A restart would have to be idempotent and not race the fetch already in flight. The flags readfalseon the editor because hosts install it as a child view controller, so the gate would never fire.Breaking changes
MediaUploadDelegateMediaProcessor— conformances need only the new nameeditor.mediaUploadDelegate = x(iOS)EditorViewController(configuration:…, mediaProcessor: x)view.mediaUploadDelegate = x(Android)view.mediaProcessor = xuploadFileon the delegateMediaUploader; passmediaUploader:(iOS) or setview.mediaUploader(Android)MediaUploadResponseis publicuploadFilewas the only public API that named itA code search of WordPress-iOS and WordPress-Android finds no reference to
mediaUploadDelegate. WordPress-iOS's twoGutenbergKit.EditorViewController(...)call sites use argument labels, so the added parameters are source-compatible there.Android needs no counterpart to
stopMediaHandling(): a tracing GC collects the cycle ARC cannot, andonDetachedFromWindowalready stops the server.Test plan
xcodebuild test(iPhone 18 Pro, iOS 27.0): 598 + 396 tests pass, including the UIKit-gated teardown and lifecycle suites the host run compiles outswift test: 587 + 396 tests pass, including the two exit tests that assert the uploader-credentials trap itself (they run only on macOS):Gutenberg:testDebugUnitTest: 741 tests passprocessesForHostReleasedProcessorfails against a weakly-held handler withpassthroughUploadCalledcoveringTheEditorDoesNotCancelTheDependencyFetchfails against the oldviewDidDisappearwith a cancelled requestuploadServerStartsForAnyHandlerruns for processor-only, uploader-only and both; restoring a post-bind check that looks at the processor alone fails only the uploader-only caseretainsProcessorForServerLifetimeandstopReleasesProcessorThatRetainsTheServer, and droppingmediaProcessor = nilfromstopMediaHandling()failsstopMediaHandlingBreaksTheOwnershipCyclenon-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor'Two paths are deliberately untested: the cancellation checks before delivery, and the guard that discards a server which finishes binding after
stopMediaHandling(). Each needs teardown driven between two points of a live in-flight request, and a timing-based approximation would pass whether or not it reached the window.