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..22d9ddb17 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -22,6 +22,16 @@ final class MediaUploadServer: Sendable { private let server: HTTPServer + /// The request handler and the dependencies it was started with. + /// + /// Owned **here**, not by the listener's handler closure — that closure borrows + /// it weakly. A started `NWListener` is retained by Network.framework until + /// `cancel()` completes asynchronously, so anything the closure owns outlives + /// this server by that teardown window. This server, by contrast, is released + /// synchronously when `EditorViewController` is, so owning the handler here + /// means the host's delegate is too. + private let handler: UploadRequestHandler + /// Sweeps crash-orphaned upload temp files off the editor-startup path. /// Exposed so tests can await completion. (Mirrors Android's `cleanupJob`.) let cleanupTask: Task @@ -45,7 +55,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let handler = UploadRequestHandler(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -62,16 +72,30 @@ final class MediaUploadServer: Sendable { bodyReadTimeout: bodyReadTimeout, cors: .permissive, delegate: ServerDelegate(), - handler: { request in - await Self.handleRequest(request, context: context) + // Borrowed weakly: see `handler`. `guard let` pins it strongly for the + // whole request, so an upload in flight cannot lose the delegate + // mid-way. + // + // The 503 is a narrow backstop, not the usual path. Releasing this + // server releases `HTTPServer`, whose `deinit` cancels the listener, + // so a request sent after teardown is normally refused at connect. This + // branch covers only the race where a connection was already accepted + // and reaches the handler before its task observes cancellation — + // better a refusal than serving against a half-torn-down editor. + handler: { [weak handler] request in + guard let handler else { + return Self.errorResponse(status: 503, message: "The editor is no longer available.") + } + return await handler.handle(request) } ) - return MediaUploadServer(server: server, cleanupTask: cleanupTask) + return MediaUploadServer(server: server, handler: handler, cleanupTask: cleanupTask) } - private init(server: HTTPServer, cleanupTask: Task) { + private init(server: HTTPServer, handler: UploadRequestHandler, cleanupTask: Task) { self.server = server + self.handler = handler self.port = server.port self.token = server.token self.cleanupTask = cleanupTask @@ -82,9 +106,161 @@ final class MediaUploadServer: Sendable { server.stop() } - // MARK: - Request Handling + fileprivate static func errorResponse(status: Int, message: String) -> HTTPResponse { + // Emit a WordPress-REST-style error object so the JS middleware normalizes + // it (and surfaces `message`) the same way it does a relayed WordPress + // error — the local server's own errors need no special-casing. + let payload = ["code": "upload_error", "message": message] + let body = (try? JSONSerialization.data(withJSONObject: payload)) + ?? Data(#"{"code":"upload_error","message":"Upload failed"}"#.utf8) + return HTTPResponse( + status: status, + headers: [("Content-Type", "application/json")], + body: body + ) + } + + /// Answers the server's recoverable parse errors (e.g. an over-limit body) + /// with the same JSON `{code, message}` shape the editor expects, so the + /// middleware surfaces a real message ("The file is too large…") instead of a + /// generic parse-failure. A leaf object — the HTTP server retains it. + private final class ServerDelegate: HTTPServerDelegate { + func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { + let message: String = switch error { + case .payloadTooLarge: "The file is too large to upload in the editor." + default: "\(error.httpStatusText)" + } + return MediaUploadServer.errorResponse(status: error.httpStatus, message: message) + } + } + + // MARK: - Helpers + + /// Directory for staging uploaded files, under the system temp dir. + fileprivate static var uploadsTempDirectory: URL { + FileManager.default.temporaryDirectory + .appending(component: "GutenbergKit-uploads", directoryHint: .isDirectory) + } + + /// Deletes upload temp files left behind by a prior crash. Files still in + /// flight (only seconds old) are preserved by the age threshold, so this is + /// safe even if another editor instance is mid-upload. + private static func cleanOrphanedUploads() { + let cutoff = Date(timeIntervalSinceNow: -3600) // 1 hour ago + guard let files = try? FileManager.default.contentsOfDirectory( + at: uploadsTempDirectory, + includingPropertiesForKeys: [.contentModificationDateKey] + ) else { return } + for file in files { + let modified = (try? file.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate + if let modified, modified < cutoff { + try? FileManager.default.removeItem(at: file) + } + } + } + + /// Sanitizes a filename to prevent path traversal. + fileprivate static func sanitizeFilename(_ name: String) -> String { + let safe = (name as NSString).lastPathComponent + .replacingOccurrences(of: "/", with: "") + .replacingOccurrences(of: "\\", with: "") + return safe.isEmpty ? "upload" : safe + } + + /// Streams an InputStream to a file URL. + fileprivate static func writeStream(_ inputStream: InputStream, to url: URL) throws { + inputStream.open() + defer { inputStream.close() } + + // `OutputStream(url:append:)` returns nil if the file can't be opened for + // writing (e.g. the uploads directory was removed after it was created, or + // a permissions/sandbox failure). Throw rather than force-unwrap so the + // caller returns a clean 500 instead of trapping the process. + guard let outputStream = OutputStream(url: url, append: false) else { + throw UploadError.streamWriteFailed + } + outputStream.open() + defer { outputStream.close() } + + let bufferSize = 65_536 + let buffer = UnsafeMutablePointer.allocate(capacity: bufferSize) + defer { buffer.deallocate() } + + // Use read() return value as the sole termination signal. Do NOT check + // hasBytesAvailable — for piped streams (used by file-slice RequestBody), + // it can return false before the writer thread has pumped the next chunk, + // causing an early exit and a truncated file. + while true { + let bytesRead = inputStream.read(buffer, maxLength: bufferSize) + if bytesRead < 0 { + throw inputStream.streamError ?? UploadError.streamReadFailed + } + if bytesRead == 0 { break } + + var totalWritten = 0 + while totalWritten < bytesRead { + let written = outputStream.write(buffer.advanced(by: totalWritten), maxLength: bytesRead - totalWritten) + if written < 0 { + throw outputStream.streamError ?? UploadError.streamWriteFailed + } + totalWritten += written + } + } + } +} + +// MARK: - Errors + +/// Errors from the native media upload pipeline. +enum UploadError: Error, LocalizedError { + case noUploader + case streamReadFailed + case streamWriteFailed + + var errorDescription: String? { + switch self { + case .noUploader: "No upload delegate or default uploader configured" + case .streamReadFailed: "Failed to read upload stream" + case .streamWriteFailed: "Failed to write upload to disk" + } + } +} + +// MARK: - Upload Request Handler + +/// Handles requests for ``MediaUploadServer``, owning the delegate and default +/// uploader it was started with. +/// +/// A reference type so the listener's handler closure can borrow it weakly. That +/// weak borrow is the whole point: Network.framework retains a started +/// `NWListener` until `cancel()` completes asynchronously, so anything the closure +/// *owns* outlives the editor by that window. Borrowing keeps the host's delegate +/// on the editor's lifetime instead. +/// +/// Within a request the reference is strong — `handle` is called on a `guard let` +/// binding — so `uploadDelegate` is pinned for the request's duration and the +/// reads at the admission gate, `processFile`, and `uploadFile` cannot disagree. +/// This matches Android, which holds its `uploadDelegate` as a plain `val`. +/// +/// `Sendable` without qualification: both properties are `let`, `MediaUploadDelegate` +/// is a `Sendable` protocol, and `DefaultMediaUploader` is `@unchecked Sendable`. +private final class UploadRequestHandler: Sendable { + // Strong on purpose: an upload that cleared the admission gate must still have + // its delegate at `processFile` and `uploadFile`. The cycle `weak_delegate` + // guards against runs the other way (a delegate retaining its owner), which + // this type can neither create nor prevent — and holding the delegate here + // does not extend its life past the editor's, because the listener's closure + // only borrows this handler weakly. + // swiftlint:disable:next weak_delegate + let uploadDelegate: (any MediaUploadDelegate)? + let defaultUploader: DefaultMediaUploader? - private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { + init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { + self.uploadDelegate = uploadDelegate + self.defaultUploader = defaultUploader + } + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { let parsed = request.parsed // Routes: POST /upload, and DELETE /media/ for the editor's orphan @@ -94,28 +270,28 @@ final class MediaUploadServer: Sendable { let method = parsed.method.uppercased() if method == "POST", parsed.path == "/upload" { - return await handleUpload(request, context: context) + return await handleUpload(request) } - if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) + if method == "DELETE", let attachmentId = Self.attachmentId(fromPath: parsed.path) { + return await handleDelete(attachmentId, query: parsed.query) } - return errorResponse(status: 404, message: "Not found") + return MediaUploadServer.errorResponse(status: 404, message: "Not found") } - private static func handleUpload(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { + private func handleUpload(_ request: HTTPServer.Request) async -> HTTPResponse { let parts: [MultipartPart] do { parts = try request.parsed.multipartParts() } catch { Logger.uploadServer.error("Multipart parse failed: \(error)") - return errorResponse(status: 400, message: "Expected multipart/form-data") + return MediaUploadServer.errorResponse(status: 400, message: "Expected multipart/form-data") } // Find the file part (the first part with a filename). guard let filePart = parts.first(where: { $0.filename != nil }) else { - return errorResponse(status: 400, message: "No file found in request") + return MediaUploadServer.errorResponse(status: 400, message: "No file found in request") } // The non-file parts (post, additionalData) and the original query @@ -130,11 +306,11 @@ final class MediaUploadServer: Sendable { // like this. If not, forward the original upload to WordPress directly, // skipping a full temp-file copy of a file the delegate won't process or // upload (e.g. a video handed to an image-only delegate). - guard context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { + guard uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { do { - return try await passthroughResponse(request, query: query, context: context) + return try await passthroughResponse(request, query: query) } catch { - return uploadErrorResponse(error) + return Self.uploadErrorResponse(error) } } @@ -142,17 +318,17 @@ final class MediaUploadServer: Sendable { // file for it — the library's RequestBody may be a byte-range slice of a // larger temp file whose lifecycle is tied to ARC, so the delegate needs a // standalone file that outlives the handler return. - let tempDir = uploadsTempDirectory + let tempDir = MediaUploadServer.uploadsTempDirectory try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(sanitizeFilename(filename))") + let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(MediaUploadServer.sanitizeFilename(filename))") do { let inputStream = try filePart.body.makeInputStream() - try writeStream(inputStream, to: fileURL) + try MediaUploadServer.writeStream(inputStream, to: fileURL) } catch { try? FileManager.default.removeItem(at: fileURL) Logger.uploadServer.error("Failed to write upload to disk: \(error)") - return errorResponse(status: 500, message: "Failed to save file") + return MediaUploadServer.errorResponse(status: 500, message: "Failed to save file") } // From here on always clean up the original temp file. The processed @@ -163,19 +339,19 @@ final class MediaUploadServer: Sendable { do { let uploadResult = try await processAndUpload( fileURL: fileURL, mimeType: mimeType, filename: filename, - extraParts: extraParts, query: query, context: context + extraParts: extraParts, query: query ) switch uploadResult { case .uploaded(let uploaded): Logger.uploadServer.debug("Uploaded file to WordPress") - return relayResponse(uploaded) + return Self.relayResponse(uploaded) case .passthrough: // Delegate didn't modify the file — forward the original request // body to WordPress without re-encoding. - return try await passthroughResponse(request, query: query, context: context) + return try await passthroughResponse(request, query: query) } } catch { - return uploadErrorResponse(error) + return Self.uploadErrorResponse(error) } } @@ -183,17 +359,17 @@ final class MediaUploadServer: Sendable { /// re-encoding) and relays the response. Used when the delegate won't touch /// the file — it declined by metadata (`handlesFile` returned false) or /// `processFile` returned `.original`. - private static func passthroughResponse( - _ request: HTTPServer.Request, query: String, context: UploadContext + private func passthroughResponse( + _ request: HTTPServer.Request, query: String ) async throws -> HTTPResponse { Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") guard let body = request.parsed.body, let contentType = request.parsed.header("Content-Type"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + let defaultUploader else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } let response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: query) - return relayResponse(response) + return Self.relayResponse(response) } /// The attachment ID in a `/media/` path, or `nil` if the path is not one. @@ -215,17 +391,17 @@ final class MediaUploadServer: Sendable { /// request directly — api-fetch tunnels `DELETE` as a `POST` carrying /// `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the /// browser blocks it at preflight. Relaying it here lets the cleanup run. - private static func handleDelete( - _ attachmentId: String, query: String, context: UploadContext + private func handleDelete( + _ attachmentId: String, query: String ) async -> HTTPResponse { - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + guard let defaultUploader else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } do { let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) - return relayResponse(response) + return Self.relayResponse(response) } catch { - return uploadErrorResponse(error) + return Self.uploadErrorResponse(error) } } @@ -261,7 +437,7 @@ final class MediaUploadServer: Sendable { } else { Logger.uploadServer.error("Upload processing failed: \(error)") } - return errorResponse(status: 500, message: error.localizedDescription) + return MediaUploadServer.errorResponse(status: 500, message: error.localizedDescription) } // MARK: - Delegate Pipeline @@ -276,13 +452,13 @@ final class MediaUploadServer: Sendable { case passthrough } - private static func processAndUpload( + private func processAndUpload( fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, context: UploadContext + extraParts: [MultipartPart], query: String ) async throws -> UploadResult { // Step 1: Process (resize, transcode, etc.) let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { + if let delegate = uploadDelegate { processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { processed = .original @@ -314,10 +490,10 @@ final class MediaUploadServer: Sendable { } // Step 2: Upload to remote WordPress - if let delegate = context.uploadDelegate, + if let delegate = uploadDelegate, let result = try await delegate.uploadFile(at: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) { return .uploaded(result) - } else if let defaultUploader = context.defaultUploader { + } else if let defaultUploader { // Unmodified — forward the original request body directly, skipping // multipart re-encoding. if case .original = processed { @@ -329,149 +505,6 @@ final class MediaUploadServer: Sendable { throw UploadError.noUploader } } - - private static func errorResponse(status: Int, message: String) -> HTTPResponse { - // Emit a WordPress-REST-style error object so the JS middleware normalizes - // it (and surfaces `message`) the same way it does a relayed WordPress - // error — the local server's own errors need no special-casing. - let payload = ["code": "upload_error", "message": message] - let body = (try? JSONSerialization.data(withJSONObject: payload)) - ?? Data(#"{"code":"upload_error","message":"Upload failed"}"#.utf8) - return HTTPResponse( - status: status, - headers: [("Content-Type", "application/json")], - body: body - ) - } - - /// Answers the server's recoverable parse errors (e.g. an over-limit body) - /// with the same JSON `{code, message}` shape the editor expects, so the - /// middleware surfaces a real message ("The file is too large…") instead of a - /// generic parse-failure. A leaf object — the HTTP server retains it. - private final class ServerDelegate: HTTPServerDelegate { - func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { - let message: String = switch error { - case .payloadTooLarge: "The file is too large to upload in the editor." - default: "\(error.httpStatusText)" - } - return MediaUploadServer.errorResponse(status: error.httpStatus, message: message) - } - } - - // MARK: - Helpers - - /// Directory for staging uploaded files, under the system temp dir. - private static var uploadsTempDirectory: URL { - FileManager.default.temporaryDirectory - .appending(component: "GutenbergKit-uploads", directoryHint: .isDirectory) - } - - /// Deletes upload temp files left behind by a prior crash. Files still in - /// flight (only seconds old) are preserved by the age threshold, so this is - /// safe even if another editor instance is mid-upload. - private static func cleanOrphanedUploads() { - let cutoff = Date(timeIntervalSinceNow: -3600) // 1 hour ago - guard let files = try? FileManager.default.contentsOfDirectory( - at: uploadsTempDirectory, - includingPropertiesForKeys: [.contentModificationDateKey] - ) else { return } - for file in files { - let modified = (try? file.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate - if let modified, modified < cutoff { - try? FileManager.default.removeItem(at: file) - } - } - } - - /// Sanitizes a filename to prevent path traversal. - private static func sanitizeFilename(_ name: String) -> String { - let safe = (name as NSString).lastPathComponent - .replacingOccurrences(of: "/", with: "") - .replacingOccurrences(of: "\\", with: "") - return safe.isEmpty ? "upload" : safe - } - - /// Streams an InputStream to a file URL. - private static func writeStream(_ inputStream: InputStream, to url: URL) throws { - inputStream.open() - defer { inputStream.close() } - - // `OutputStream(url:append:)` returns nil if the file can't be opened for - // writing (e.g. the uploads directory was removed after it was created, or - // a permissions/sandbox failure). Throw rather than force-unwrap so the - // caller returns a clean 500 instead of trapping the process. - guard let outputStream = OutputStream(url: url, append: false) else { - throw UploadError.streamWriteFailed - } - outputStream.open() - defer { outputStream.close() } - - let bufferSize = 65_536 - let buffer = UnsafeMutablePointer.allocate(capacity: bufferSize) - defer { buffer.deallocate() } - - // Use read() return value as the sole termination signal. Do NOT check - // hasBytesAvailable — for piped streams (used by file-slice RequestBody), - // it can return false before the writer thread has pumped the next chunk, - // causing an early exit and a truncated file. - while true { - let bytesRead = inputStream.read(buffer, maxLength: bufferSize) - if bytesRead < 0 { - throw inputStream.streamError ?? UploadError.streamReadFailed - } - if bytesRead == 0 { break } - - var totalWritten = 0 - while totalWritten < bytesRead { - let written = outputStream.write(buffer.advanced(by: totalWritten), maxLength: bytesRead - totalWritten) - if written < 0 { - throw outputStream.streamError ?? UploadError.streamWriteFailed - } - totalWritten += written - } - } - } -} - -// MARK: - Errors - -/// Errors from the native media upload pipeline. -enum UploadError: Error, LocalizedError { - case noUploader - case streamReadFailed - case streamWriteFailed - - var errorDescription: String? { - switch self { - case .noUploader: "No upload delegate or default uploader configured" - case .streamReadFailed: "Failed to read upload stream" - case .streamWriteFailed: "Failed to write upload to disk" - } - } -} - -// MARK: - Upload Context - -/// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and re-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. -/// -/// `@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)? - 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/EditorViewControllerMediaLifetimeTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaLifetimeTests.swift new file mode 100644 index 000000000..489e67514 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaLifetimeTests.swift @@ -0,0 +1,55 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +#if canImport(UIKit) + +/// Pins the ownership contract documented on ``EditorViewController/mediaUploadDelegate``: +/// the editor holds the delegate strongly for its lifetime and lets go on `deinit`. +/// +/// The server-side half of this — that `MediaUploadServer` frees its handler, and +/// therefore the delegate, the moment the host releases it — is covered on the host +/// platform by `MediaUploadServerTests`. This suite covers the half that only exists +/// under UIKit: that owning the delegate strongly does not keep the editor itself +/// alive, so `deinit` actually runs and the release actually happens. +@Suite("EditorViewController media upload lifetime") +struct EditorViewControllerMediaLifetimeTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://test.example.com")! + static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! + + @MainActor + @Test("deinit releases the editor and the media upload delegate it owns") + func deinitReleasesEditorAndDelegate() throws { + weak var weakEditor: EditorViewController? + weak var weakDelegate: LifetimeProbeDelegate? + + do { + let editor = EditorViewController(configuration: makeConfiguration()) + let delegate = LifetimeProbeDelegate() + editor.mediaUploadDelegate = delegate + weakEditor = editor + weakDelegate = delegate + } + + // `mediaUploadDelegate` is a strong `var` (the `weak_delegate` rule is + // disabled on it deliberately). That is only safe while the delegate does + // not retain the editor back, so pin that the editor is still freed. + #expect(weakEditor == nil, "EditorViewController leaked — check for a cycle through mediaUploadDelegate") + + // And that the host does not have to hold the delegate itself: assigning it + // and dropping every other reference must not leak it for the process's life. + #expect(weakDelegate == nil, "mediaUploadDelegate outlived the editor that owned it") + } +} + +/// A delegate that does nothing but be observed for deallocation. +private final class LifetimeProbeDelegate: MediaUploadDelegate, @unchecked Sendable { + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} + +#endif diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 60040e670..917c5c4be 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -385,24 +385,172 @@ 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("releases the delegate synchronously when the server is released") + func releasesDelegateSynchronously() 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 + + // Asserted outright, not polled. The listener's handler closure borrows the + // handler weakly, so the delegate hangs off `MediaUploadServer` alone — and + // that is released synchronously here, exactly as it is in + // `EditorViewController.deinit`. If the closure owned the handler instead, + // this would fail: a started NWListener is retained by Network.framework + // until `cancel()` completes asynchronously (~17ms), so the release would + // trail `stop()` and only a polled assertion would pass. #expect(weakDelegate == nil) } + @Test("releases the server object itself when the host lets go (no retain cycle)") + func releasesServerItself() async throws { + weak var weakServer: MediaUploadServer? + do { + let server = try await MediaUploadServer.start(uploadDelegate: MockUploadDelegate()) + weakServer = server + // Deliberately no `stop()`: this pins that plain ARC frees the server, so a + // cycle cannot hide behind an explicit teardown call. + } + + // Asserted directly rather than inferred from the delegate going away, so a + // future strong capture of `handler` in the listener closure — which would + // reintroduce MediaUploadServer -> HTTPServer -> closure -> MediaUploadServer + // — fails here with the real cause rather than as a confusing delegate leak. + #expect(weakServer == nil, "MediaUploadServer leaked — something is holding it past the host's last reference") + } + + @Test("a request arriving after release is refused and never reaches the uploader") + func refusesRequestsAfterRelease() async throws { + let mockUploader = MockDefaultUploader() + var server: MediaUploadServer? = try await MediaUploadServer.start( + uploadDelegate: MockUploadDelegate(), defaultUploader: mockUploader + ) + let port = server!.port + let token = server!.token + + server = nil + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "a.jpg", mimeType: "image/jpeg", data: Data("x".utf8)) + var request = URLRequest(url: URL(string: "http://127.0.0.1:\(port)/upload")!) + request.httpMethod = "POST" + request.setValue("Bearer \(token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + // In practice this is a refused connection, not a 503: releasing the server + // releases `HTTPServer`, whose `deinit` cancels the listener, so the socket is + // gone before a new request can land. The `guard let handler` fallback in + // `start` covers the far narrower race where a connection was already accepted + // and reaches the handler before its task observes cancellation — reachable in + // principle, not deterministically from out here. Assert what is observable: + // either outcome is a refusal, and neither uploads anything. + if let (_, response) = try? await URLSession.shared.data(for: request) { + let status = (response as? HTTPURLResponse)?.statusCode + #expect(status == 503, "served a request after release with status \(String(describing: status))") + } + #expect(!mockUploader.uploadCalled) + #expect(!mockUploader.passthroughUploadCalled) + } + + @Test("pins the delegate for an in-flight request when the server is released mid-upload") + func pinsDelegateAcrossInFlightRequest() async throws { + let mockUploader = MockDefaultUploader() + // Held by the test *and* the delegate, so the test can observe and unblock + // the request without keeping a strong reference to the delegate itself. + let gate = UploadGate() + weak var weakDelegate: BlockingDelegate? + + var server: MediaUploadServer? + do { + let delegate = BlockingDelegate(gate: gate) + weakDelegate = delegate + server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + } + let port = server!.port + let token = server!.token + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + var request = URLRequest(url: URL(string: "http://127.0.0.1:\(port)/upload")!) + request.httpMethod = "POST" + request.setValue("Bearer \(token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let call = Task { try await URLSession.shared.data(for: request) } + + // Wait until the delegate is inside processFile — the request is committed. + for _ in 0..<500 where !gate.didStart { + try await Task.sleep(for: .milliseconds(10)) + } + #expect(gate.didStart, "request never reached the delegate") + + // Now drop the server, as `EditorViewController.deinit` does, while the + // delegate is mid-process. The handler is pinned by the `guard let` binding + // for the duration of this request, so the delegate must survive it. + server?.stop() + server = nil + + #expect(weakDelegate != nil, "delegate was released while a request was still inside processFile") + + gate.unblock() + _ = try? await call.value + + // Only once the request has unwound does the last reference go. + for _ in 0..<200 where weakDelegate != nil { + try await Task.sleep(for: .milliseconds(10)) + } + #expect(weakDelegate == nil, "delegate leaked after the request unwound") + } + + @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 +939,51 @@ 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. +/// Shared between the test and ``BlockingDelegate`` so the test can watch a +/// request enter `processFile`, and let it out, without retaining the delegate. +private final class UploadGate: @unchecked Sendable { + private let lock = NSLock() + private var _didStart = false + private var _unblocked = false + + var didStart: Bool { lock.withLock { _didStart } } + var isUnblocked: Bool { lock.withLock { _unblocked } } + func markStarted() { lock.withLock { _didStart = true } } + func unblock() { lock.withLock { _unblocked = true } } +} + +private final class BlockingDelegate: MediaUploadDelegate, @unchecked Sendable { + private let gate: UploadGate + + init(gate: UploadGate) { + self.gate = gate + } + + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + gate.markStarted() + while !gate.isUnblocked { + try await Task.sleep(for: .milliseconds(5)) + } + return .original + } +} + +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