From 95ae0f0d988bae3ff5f59850715b21840f2d8275 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:24:53 -0600 Subject: [PATCH] fix(ios): own the media upload delegate instead of holding it weakly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Sources/EditorViewController.swift | 36 ++++----- .../Sources/Media/MediaUploadServer.swift | 31 ++++--- .../Media/MediaUploadServerTests.swift | 81 +++++++++++++++++-- 3 files changed, 102 insertions(+), 46 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 09ec7766a..f1c357473 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -109,11 +109,6 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// take effect, so its setter traps if written. private var hasStartedLoading = false - /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load - /// path tell "the delegate was released before load" (a retention mistake to - /// trap) apart from "no delegate was configured" (a valid opt-out). - private var mediaUploadDelegateWasAssigned = false - /// Delegate for customizing media file processing and upload behavior. /// /// Provide this **before the editor loads** — typically right after `init`, the @@ -121,16 +116,17 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// once, when the editor begins loading, and injected into the page's initial /// configuration; setting it afterward has no effect, so the setter traps. /// - /// - Important: This is a `weak` reference — you must hold a strong reference to - /// your delegate until the editor has loaded, or native uploads are silently - /// disabled. To surface that mistake, the editor traps at load time if a - /// delegate that was assigned here has already been deallocated. - public weak var mediaUploadDelegate: (any MediaUploadDelegate)? { + /// The editor **owns** this for its lifetime and releases it on `deinit`, so you + /// don't need to keep a reference after assigning it. The one rule: your delegate + /// must not strongly retain this `EditorViewController` in return, or the two form + /// a retain cycle and neither is freed. + // Ownership here is the point: the editor holds this for its lifetime so an + // in-flight upload can't lose the delegate mid-request. The cycle `weak_delegate` + // guards against runs the other way (a delegate retaining the editor), which this + // property can neither create nor prevent. + // swiftlint:disable:next weak_delegate + public var mediaUploadDelegate: (any MediaUploadDelegate)? { didSet { - // Record whether a delegate was provided so the load path can tell a - // premature deallocation apart from a deliberate opt-out (see - // `startUploadServer`). - mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil // Deliberate fail-fast, not a defensive check. The delegate is captured // into the page's initial configuration when the editor begins loading, // so a delegate assigned afterward would silently never take effect; @@ -451,14 +447,10 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { - // A delegate that was provided but is already nil here was deallocated before - // the editor finished loading — the host didn't hold a strong reference to it. - // That silently disables native uploads, so trap loudly instead. - precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." - ) - + // Nothing to route through the native server unless the host provided a + // delegate. The editor owns it — `mediaUploadDelegate` is strong — so there's + // no released-before-load case to guard against; it lives as long as the + // editor does. guard mediaUploadDelegate != nil else { return } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 3318aeac8..d01c415bc 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -453,25 +453,24 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context /// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and re-read on each request. +/// HTTPServer handler closure and read on each request. /// -/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is -/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly -/// here would silently defeat that contract and, worse, risk a retain cycle -/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext → -/// delegate → EditorViewController`) that would keep the view controller — and -/// therefore the server — alive forever, so `deinit` would never stop it. +/// Both are held **strongly**, so a delegate that admitted a file for processing +/// will process it — the three reads within a request can't disagree, and an +/// in-flight upload keeps the host's delegate alive until it unwinds. This matches +/// Android, which holds its `uploadDelegate` as a plain `val` for the same reason. /// -/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read -/// afterwards; weak-reference reads are thread-safe at runtime. -private final class UploadContext: @unchecked Sendable { - weak var uploadDelegate: (any MediaUploadDelegate)? +/// Strong is safe because `EditorViewController` owns `mediaUploadDelegate` strongly +/// too. A host object that retains the view controller back already forms +/// `EditorViewController → mediaUploadDelegate → EditorViewController`, a cycle this +/// container can neither create nor prevent — so holding weak here bought no leak +/// protection, only the risk of the delegate vanishing mid-request. +/// +/// A `struct`, so it is implicitly `Sendable`: `MediaUploadDelegate` is a `Sendable` +/// protocol and `DefaultMediaUploader` is `@unchecked Sendable`. +private struct UploadContext: Sendable { + let uploadDelegate: (any MediaUploadDelegate)? let defaultUploader: DefaultMediaUploader? - - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate - self.defaultUploader = defaultUploader - } } // MARK: - Default Media Uploader diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 60040e670..a44bfd0fb 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -385,24 +385,75 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } - @Test("does not strongly retain the upload delegate (weak — preserves deinit teardown)") - func doesNotStronglyRetainDelegate() async throws { + @Test("retains the delegate for the server's lifetime, and releases it after") + func retainsDelegateForServerLifetime() async throws { weak var weakDelegate: MockUploadDelegate? - let server: MediaUploadServer + var server: MediaUploadServer? do { let delegate = MockUploadDelegate() weakDelegate = delegate server = try await MediaUploadServer.start(uploadDelegate: delegate) } - defer { server.stop() } - // UploadContext holds the delegate weakly, so releasing the host's strong - // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController - // cycle, so deinit would never fire and the server would never stop. + // The server owns the delegate while it runs: the host can assign one and drop + // its own reference, and every request still sees it. + #expect(weakDelegate != nil) + + server?.stop() + server = nil + + // …and lets go when it does, so the delegate isn't leaked for the process's + // lifetime. The cycle the old `weak` was defending against runs through + // `EditorViewController.mediaUploadDelegate`, which this container can neither + // create nor prevent. + // + // Polled rather than asserted outright: the handler closure is captured by the + // listener's `newConnectionHandler`, and `NWListener.cancel()` is asynchronous — + // the framework holds the listener until cancellation completes, so the release + // trails `stop()` by a beat. A leak still fails this, just after a second. + for _ in 0..<100 where weakDelegate != nil { + try await Task.sleep(for: .milliseconds(10)) + } #expect(weakDelegate == nil) } + @Test("still processes for a delegate the host has dropped its reference to") + func processesForHostReleasedDelegate() async throws { + // The delegate is read at the admission gate and again at processFile and + // uploadFile, separated by a synchronous disk copy and an unbounded processFile. + // Held weakly, a host that dropped its reference changed the answer between + // those reads: a file admitted for processing was forwarded unprocessed. The + // host dropping it before the request is the same condition, deterministically. + let mockUploader = MockDefaultUploader() + var delegate: TranscodingDelegate? = TranscodingDelegate() + weak var weakDelegate = delegate + let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + defer { server.stop() } + + // Drop the host's only strong reference. Under the documented contract the + // server owns the delegate from here, so the upload must still be processed. + delegate = nil + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The server kept it alive, so the processed metadata reached the uploader. + // Against a weak container this fails with the real symptom: the passthrough + // branch runs and the original video/quicktime is forwarded unprocessed. + #expect(weakDelegate != nil) + #expect(mockUploader.uploadCalled) + #expect(mockUploader.lastUploadMimeType == "video/mp4") + #expect(!mockUploader.passthroughUploadCalled) + } + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { var body = Data() body.append("--\(boundary)\r\n") @@ -791,6 +842,20 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks +/// A delegate that transcodes, used to check the server holds it across the whole +/// request rather than re-reading a reference the host may have dropped. +private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable { + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + true + } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let processed = FileManager.default.temporaryDirectory.appendingPathComponent("clip.mp4") + try? Data("transcoded".utf8).write(to: processed) + return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4") + } +} + private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false