Skip to content

Avoid task delegates for background sessions#25774

Merged
crazytonyli merged 3 commits into
trunkfrom
fix/background-session-task-delegate-crash
Jul 14, 2026
Merged

Avoid task delegates for background sessions#25774
crazytonyli merged 3 commits into
trunkfrom
fix/background-session-task-delegate-crash

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Description

I noticed this share extension crash in the App Store Connection crash reporter in Xcode.

@crazytonyli crazytonyli added this to the 27.1 milestone Jul 13, 2026
@crazytonyli crazytonyli requested a review from jkmassel July 13, 2026 00:04
@crazytonyli crazytonyli enabled auto-merge July 13, 2026 00:11
@wpmobilebot

wpmobilebot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number33125
VersionPR #25774
Bundle IDorg.wordpress.alpha
Commit0c00b5f
Installation URL382em8pcsorp8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number33125
VersionPR #25774
Bundle IDcom.jetpack.alpha
Commit0c00b5f
Installation URL0s8ttpfebtep8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jkmassel jkmassel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I left two nits for your consideration


task.delegate = wpkURLSessionNotifyingDelegate
// Background tasks only support the delegate assigned to their session at creation time.
if configuration.identifier == nil, let delegate = wpkURLSessionNotifyingDelegate {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we use configuration.identifier == nil in a few different places in this file – WDYT about introducing isBackgroundSession to make it a bit more clear at the call site?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another nit – do we need to unwrap wpkURLSessionNotifyingDelegate? Assigning nil to task.delegate wouldn't cause problems, would it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly, it would crash.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted a helper in 0c00b5f

@crazytonyli crazytonyli added this pull request to the merge queue Jul 13, 2026
@jkmassel jkmassel removed this pull request from the merge queue due to a manual request Jul 13, 2026
@jkmassel

Copy link
Copy Markdown
Contributor

disabled auto-merge in case you want to address the nits. Feel free to merge if not

@crazytonyli crazytonyli enabled auto-merge July 13, 2026 23:17
@crazytonyli crazytonyli added this pull request to the merge queue Jul 14, 2026
Merged via the queue into trunk with commit dfea201 Jul 14, 2026
24 of 26 checks passed
@crazytonyli crazytonyli deleted the fix/background-session-task-delegate-crash branch July 14, 2026 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants