Skip to content

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Apr 6, 2025

Summary

Resolves: #1477

tvOS seems to crash on first login due to a memory allocation issue. By wrapping these operations in DispatchQueue.main.async, we ensure that:

  1. UI Updates Happen on the Main Thread: SwiftUI requires all UI updates to happen on the main thread. When we modify Defaults or post notifications that trigger UI updates, doing this on a background thread can cause crashes.

  2. Thread Safety for Shared Resources: Container.shared.currentUserSession.reset() deals with a shared resource that might be accessed from multiple threads. The async wrapper ensures thread-safe access.

  3. Memory Access Synchronization: The crash was an EXC_BAD_ACCESS error, which often happens when memory is deallocated on one thread while being accessed on another. The async wrapper ensures proper synchronization.

  4. Deferred Execution: By deferring these operations to the next run loop cycle, we give any pending UI updates a chance to complete before changing application state.

(According to Claude) tvOS is particularly sensitive to threading issues because:

  1. Its rendering pipeline has different timing characteristics than iOS
  2. Memory management is more aggressive due to limited resources
  3. The focus system adds additional complexity to the UI update cycle

Does it make sense to, where possible, move these operations to the viewModel instead?

@JPKribs JPKribs added bug Something isn't working crash This issue results in a crash labels Apr 6, 2025
JPKribs added 2 commits April 6, 2025 12:40
Restore unnecessarily removed items
Remove unnecessary formatting changes.
@JPKribs
Copy link
Member Author

JPKribs commented Apr 6, 2025

Should we also add this for iOS? Are we just getting lucky this doesn't happen on iOS or is it actually structurally different enough this should be fine?

            DispatchQueue.main.async {
                Defaults[.lastSignedInUserID] = .signedIn(userID: user.id)
                Container.shared.currentUserSession.reset()
                Notifications[.didSignIn].post()
            }

@sjorge
Copy link

sjorge commented Apr 6, 2025

this does indeed resolve #1477 for me

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

Does it make sense to, where possible, move these operations to the viewModel instead?

No, as I've adopted the architecture that events that should cause view transitions should happen in views as reactions to received events.

@JPKribs
Copy link
Member Author

JPKribs commented Apr 6, 2025

No, as I've adopted the architecture that events that should cause view transitions should happen in views as reactions to received events.

Gotcha! That makes sense. Sorry, all the architecture side of things is beyond me lol. Does this MainActor need to exist on iOS as well or is this purely tvOS quirkiness?


Just as a second set of eyes, I tested with the revisions in 1870878 and this still works as expected!

@LePips LePips merged commit df89832 into jellyfin:main Apr 6, 2025
4 checks passed
@LePips
Copy link
Member

LePips commented Apr 6, 2025

I will have to investigate more threading issues on tvOS as I think this is somewhat also related to why #1090 (comment) was necessary.

@JPKribs JPKribs deleted the tvOSLoginCrash branch April 6, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working crash This issue results in a crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tvOS] (master) Crash on first login
3 participants