Skip to content

fix(ios): own the media upload delegate instead of holding it weakly - #625

Open
jkmassel wants to merge 6 commits into
fix/media-server-site-root-guardfrom
fix/own-media-delegate-strongly
Open

fix(ios): own the media upload delegate instead of holding it weakly#625
jkmassel wants to merge 6 commits into
fix/media-server-site-root-guardfrom
fix/own-media-delegate-strongly

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Stacked on #624. Second of ten PRs splitting #621.

What?

A host that releases its mediaUploadDelegate while an upload is in flight gets the file forwarded to WordPress unprocessed. Hold the delegate strongly, as Android already does.

Why?

The server reads the delegate three times per request — once at the admission gate (handlesFile), then again for processFile and uploadFile. Those reads are separated by a synchronous disk copy and an unbounded processFile. Held weakly, a host releasing its delegate in that window changes the answer between them: a file admitted for processing is delivered untouched.

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 the server's container holds it. What it did buy was the reference vanishing mid-request.

How?

  • MediaUploadServer.swift: UploadContext holds the delegate strongly and becomes a struct, dropping its @unchecked Sendable opt-out — MediaUploadDelegate is Sendable and DefaultMediaUploader is @unchecked Sendable, so it is implicitly Sendable. Immutable strong references make the three reads agree by construction.
  • EditorViewController.swift: mediaUploadDelegate becomes strong. 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. Hosts no longer need to retain the delegate themselves.

SwiftLint's weak_delegate is suppressed with the reasoning inline. The rule is arguably right that the name no longer fits; #630 renames the property and the suppression goes away with it.

HTTPServer.stop() now also clears the listener's newConnectionHandler. That block holds the request handler and, through it, the delegate — and cancel() alone does not drop it, so Network.framework performed the final release on its own queue after cancellation completed. A delegate reached through EditorViewController.deinit deallocated off the main thread in 46 of 50 measured runs. Clearing the handler after cancel() makes teardown synchronous on the caller's thread, so retainsDelegateForServerLifetime asserts the release outright instead of polling for it.

Known issue

Owning the delegate means a host whose conformer retains the EditorViewController back forms EditorViewController → mediaUploadDelegate → EditorViewController, and neither is freed. deinit is the only caller of uploadServer.stop(), so a leaked editor also strands its WKWebView and a bound loopback NWListener. Reproduced directly: the same test passes with weak restored and fails without it.

Nothing hits this today — mediaUploadDelegate has no callers in WordPress-iOS, and the demo's Coordinator holds only the view model. But PostGBKEditorViewController already owns its EditorViewController and is its delegate, so the adoption line is one assignment away.

#630 addresses it by dropping : AnyObject from MediaProcessor and MediaUploader, so a host can conform with a value type that captures only what the work needs rather than the view controller. That relaxes the constraint rather than making the cycle impossible, and #630's docs say so.

Testing Instructions

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.

  • Both fail against a weak container, with the real symptom: passthroughUploadCalled → true
  • swift test — host suite green
  • iOS Simulator xcodebuild
  • SwiftLint clean

@jkmassel jkmassel added [Type] Bug An existing feature does not function as intended iOS labels Sep 5, 2026
@jkmassel jkmassel self-assigned this Sep 5, 2026
@wpmobilebot

wpmobilebot commented Sep 5, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/625")

Built from a89459c

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.
@jkmassel
jkmassel force-pushed the fix/own-media-delegate-strongly branch from 95ae0f0 to 8827ba4 Compare September 8, 2026 16:11
`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.
@jkmassel
jkmassel force-pushed the fix/own-media-delegate-strongly branch from f15a637 to 9944c98 Compare September 8, 2026 17:43
`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.
@jkmassel
jkmassel marked this pull request as ready for review September 8, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants