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/651")Built from 809f751 |
This was referenced Sep 15, 2026
jkmassel
force-pushed
the
jkmassel/dependency-fetch-cancelled
branch
from
September 15, 2026 22:39
f6f3539 to
a8f2beb
Compare
jkmassel
force-pushed
the
jkmassel/dependency-fetch-cancelled
branch
from
September 16, 2026 19:41
a8f2beb to
50e56cf
Compare
jkmassel
added this pull request to stack #690
September 17, 2026 18:33
`viewDidDisappear` cancelled `dependencyTaskHandle`, the async editor dependency fetch. That callback fires whenever the editor is merely covered — a 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. Present a media picker over a still-loading editor and the load is over for good: measured in the simulator, the progress view is replaced by the load-error screen and the host is 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 exact class of failure, with the incident written into its comment — the same cancellation landing a moment later, mid `startUploadServer()`, silently disabled native uploads for the session. The async path never got the same treatment. Stop cancelling rather than cancel-and-restart. The fetch is short, and 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. That same retain is why not cancelling leaks nothing: the work is bounded by the fetch, the editor is freed the moment it unwinds, and `[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 22-line block restated the rejected alternatives, which belong in the PR description rather than above the code. It also named a repro that cannot happen: the media picker is presented from the block inserter's hosting controller with .popover or .pageSheet, neither of which removes the editor's view, so it never delivered viewDidDisappear. The incident behind the fast path's fix was a full-screen modal. Trims the test file's doc comments to what each test pins, and records why beginAppearanceTransition is not used in place of the direct calls: it delivers nothing to a windowless, parentless controller, so it passes against the very regression the test exists to catch.
theInFlightFetchKeepsTheEditorAlive released inline, after two calls that can throw. A request left parked spins a 20ms sleep loop with no exit and holds its editor, which is what the double's own doc comment warns about. release() only sets a bool under a lock, so the explicit call stays as the test's trigger and the defer is a safety net. park() also drops its generic return. Every exit throws, so <T> was never produced — it existed only to satisfy the two call sites' return types, and a future 'return canned' would have typechecked against either of them.
EditorViewController builds its EditorService through the public init, which resolves storageRoot/cacheRoot to the real Paths defaults — there is no seam to point it at a temporary directory the way MakesTestFixtures.makeService does. Each test mints a fresh UUID site host so no earlier run's cache can serve the fetch, which also meant nothing ever reclaimed what it wrote: two directories per root per run, measured at 66 of them on one machine. The host has to be unique, so the fix is to remove the roots on the way out rather than to share them. Measured: directory count flat across three consecutive runs, previously +2 per root per run.
jkmassel
force-pushed
the
jkmassel/dependency-fetch-cancelled
branch
from
September 17, 2026 22:41
50e56cf to
809f751
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #637. Independent of the media-handler teardown work in #649 — either can land first.
Summary
viewDidDisappearcancelled the async dependency fetch. It fires when the editor is merely covered, not only when it is torn down.viewDidLoad— and no restart path, so that cancellation was terminal.dependencyTaskHandleit existed to hold.deinitis unchanged.Why?
Present a full-screen modal over a still-loading editor — or push another controller onto the navigation stack above it — and the load is over. Measured in the iOS Simulator against the unfixed code, with the dependency fetch parked mid-flight and
viewDidDisappeardelivered:viewDidDisappeareditor.view.subviews["GBWebView", "UIEditorProgressView"]["GBWebView", "_UIHostingView<AnyView>"]didFailToLoadeditorDidLoadwebView.alpha0.00.0The progress view is swapped for the load-error screen and the host is told
didFailToLoad. Returning to the editor does nothing — the symptom is a permanent error screen rather than a hung spinner, but either way the editor never loads again.Note the qualifier. UIKit delivers this pair when the editor's view leaves the window.
.pageSheet,.formSheet,.popoverand the.over*styles keep the presenter's view in place and never delivered it, so this editor's own block inserter was never a trigger. The incident that produced the fast path's fix was a full-screen modal; the async path never got the same treatment.What We Explored
1. Stop cancelling ✅
Consistent with the fast path's reasoning. The fetch is short and self-limiting, so there is nothing to reclaim by aborting it early.
2. Cancel, but restart on re-appearance ❌
Preserves the original intent — don't burn network on an off-screen editor — at the cost of a restart path that has to be idempotent and must not race a fetch already in flight.
viewDidAppearfires on every uncovering, not just the one after a cancellation. That complexity has nothing to buy here.3. Move the cancellation to
deinit❌The obvious replacement, and it does not work.
deinitis a genuine teardown signal — it is whereuploadServer?.stop()already lives — but it is unreachable while the fetch is running. The task body isawait self?.prepareEditor(), and optional-chaining a weakselfinto anasynccall holds a strongselffor the duration of that call, across every suspension inside it. The editor always outlives its own load, sodeinitruns only after the task has finished, wherecancel()is a no-op.Confirmed rather than reasoned: in
theInFlightFetchKeepsTheEditorAlive, dropping the last external reference while the fetch is parked leaves the editor alive; releasing the parked request frees it. Adeinitcancel would have been dead code, so the handle goes away with the override instead of moving there.4. Gate
viewDidDisappearonisBeingDismissed/isMovingFromParent❌Hosts install this controller as a child, so UIKit sets those flags on an ancestor and they read
falsehere. Walking up to that ancestor does work — #649 measured a probe across fourteen hosting shapes that separates detaching from being covered without a single false positive — but that only pays off once the action is recoverable. Cancelling here is terminal: nothing restarts the fetch anddisplayErroroffers no retry, so a wrong guess costs the session. Same conclusion as #649, and the same reason.How?
ios/Sources/GutenbergKit/Sources/EditorViewController.swift: delete the
viewDidDisappearoverride and thedependencyTaskHandleproperty. The fast path's 22-line comment collapses to two lines stating the claim — the reasoning lives in this description instead.deinitis unchanged.ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift: new.
ParkedURLSessionis aURLSessionProtocolwhose requests park until the test releases them and record whether the surrounding task was cancelled, injected through the existingEditorViewController(httpClient:)seam — so the editor runs its realviewDidLoad→EditorService→EditorHTTPClientpath and only the socket is stubbed. Each test uses a UUID site host so no earlier run's cache can serve the fetch, and removes the two roots it creates on the way out:EditorViewControllerbuilds itsEditorServicethrough the public init and exposes no seam to redirect storage the wayMakesTestFixtures.makeServicedoes.Both tests are
#if canImport(UIKit), like the existingEditorViewControllertest — a hostswift buildcompiles this file away.Test Plan
coveringTheEditorDoesNotCancelTheDependencyFetchfails against the unfixed code with the real symptom:Expectation failed: !cancelled— 3/3 runs, 72–118msxcodebuild test -scheme GutenbergKit-Package— 591 tests greenmake lint-swiftcleanviewWillDisappear/viewDidDisappeardirectly, which guards the regression — re-adding a cancel to either fails the test — but does not re-derive that UIKit sends them for a covering presentation.beginAppearanceTransitionis deliberately not used in their place: it delivers nothing to a controller with no window and no parent, so swapping it in makes the test pass against the very regression it exists to catch.Related
deinitreliably signals teardown, written up onEditorViewController.stopMediaHandling(). Not depended on here.dependencyTaskHandlewas introduced.Accessibility Testing Instructions
No UI changes. The paths this touches are the editor's own progress and error screens, which are unchanged.