fix: surface an error when a picked media item can't be loaded - #25997
Open
jkmassel wants to merge 8 commits into
Open
fix: surface an error when a picked media item can't be loaded#25997jkmassel wants to merge 8 commits into
jkmassel wants to merge 8 commits into
Conversation
`ItemProviderMediaExporter.export` loads the picked file via
`NSItemProvider.loadFileRepresentation`. On failure its completion handler
logged at debug level and returned without calling `onError`, so the export
never completed and the Media Library import stayed pending indefinitely — the
upload spinner never resolved.
Call `onError` on the failure path (and log at error level), and add
`ExportError.cannotLoadItem` with a user-facing message instead of surfacing the
raw `NSItemProviderError` text ("Cannot load representation of type public.data").
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34393 | |
| Version | PR #25997 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | f83fdd8 | |
| Installation URL | 499nhnufpedq0 |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34393 | |
| Version | PR #25997 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | f83fdd8 | |
| Installation URL | 6ekure78210gg |
Only route a picked-media load failure to `cannotLoadItem` when the item provider's process died — an XPC connection error (`NSCocoaErrorDomain` `xpcConnectionInterrupted`/`Invalid`/`ReplyInvalid`) nested under the `NSItemProviderError`, which is the "too large to import" case. Emit a `media_import_item_unavailable` Tracks event on that path, with the error domains/codes and type identifiers, so it can be told apart from ordinary load failures — which now surface as `underlyingError` with the raw message. Make `underlyingError` carry a non-optional `Error` and add an explicit `unknown` case for the (unexpected) no-URL/no-error result. Add a Swift Testing suite covering the XPC-error detection: nested chains, all three XPC codes, and negative cases (unrelated Cocoa error, matching code in a different domain).
Read the system's global LDMGlobalEnabled user-defaults flag and attach it as a lockdown_mode property, so Tracks can confirm this failure correlates with Lockdown Mode being enabled.
When the provider-death path is hit with Lockdown Mode enabled, surface a message naming Lockdown Mode instead of the generic 'may be too large' hint. The size hint remains the fallback for an XPC failure outside Lockdown Mode.
Lockdown Mode can only change with a device restart, so the LDMGlobalEnabled default is constant for the process lifetime. LockdownHelper reads it once via a static let — lazy, thread-safe, and immutable — instead of hitting UserDefaults on each failure. ItemProviderMediaExporter now uses it.
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
WPAnalytics is vended by WordPressShared; ItemProviderMediaExporter imported only Foundation/PhotosUI/WordPressData, so the app target failed to compile with 'cannot find WPAnalytics in scope'. Every other WPAnalytics caller imports WordPressShared.
Two fixes from reviewing the load-failure handling on this branch: - handleLoadFailure now ignores cancellations (CancellationError, NSUserCancelledError, NSURLErrorCancelled) rather than surfacing them. Cancelling an in-flight import cancels this request's Progress, which fires the load completion with a cancellation error; the upload coordinator already reports the cancellation, so calling onError here produced a spurious "failed" notice (and a re-wrapped error whose code no longer matched the editors' NSURLErrorCancelled filter). The load-failure logging also moves into handleLoadFailure so a cancel logs at info instead of a false error. - errorChain no longer reads NSUnderlyingErrorKey separately. NSError.underlyingErrors already includes it, so the extra read visited every wrapped error twice and halved the depth reachable before the 16-node bound, letting deeply nested XPC errors slip past. Adds tests for cancellation classification and a five-level-deep XPC error.
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
2 participants
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.
Fixes a bug where a device photo that fails to load from the Photos picker leaves a Media Library upload spinning forever instead of reporting a failure — and distinguishes the specific failure that motivated this from ordinary load errors.
Root Cause
ItemProviderMediaExporter.exportloads the picked file viaNSItemProvider.loadFileRepresentation. On failure its completion handler logged at debug level andreturned without callingonError, so the export never completed and the import stayedpendingindefinitely — the spinner never resolved.Fix
The load-failure path now always calls
onError, and splits by cause:NSCocoaErrorDomainxpcConnectionInterrupted/Invalid/ReplyInvalid, nested under theNSItemProviderError), surface a user-facing error — a Lockdown-Mode-specific message ("…while Lockdown Mode is on.") when Lockdown Mode is enabled, otherwise thecannotLoadItemsize hint ("…It may be too large to import.") — and emit amedia_import_item_unavailableTracks event carrying the error domains/codes, the provider's registered type identifiers, and whether Lockdown Mode is enabled (LDMGlobalEnabled). This is the case that motivated the PR.underlyingError, showing the system's own message. No event.underlyingErrornow carries a non-optionalError; a newunknowncase covers the (contract-violating) no-URL/no-error result.What triggers it
This only reproduces with iOS Lockdown Mode enabled. Lockdown Mode hardens how the system materializes media for apps; loading the photo fails and the
PhotosFileProviderprocess is killed, soloadFileRepresentationreturnsNSItemProviderError -1000with an underlyingNSCocoaErrorDomain 4099— the provider's XPC connection dying. The repro used a 36 MP JPEG; outside Lockdown Mode the same photo imports normally.This PR makes the failure graceful and observable; it does not change what Lockdown Mode allows. Actually importing these under Lockdown Mode would need an import path that streams the original (e.g.
PHAsset/PHAssetResourceManager) — a separate follow-up.Test plan
Failed to load file representation … NSItemProviderError -1000 / 4099. (This is the XPC path, so it resolves tocannotLoadItem.)media_import_item_unavailablefires in Tracks (withlockdown_mode = true) when reproducing the case.