Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: fix memory leak issues #486

Merged
merged 4 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions ios/core/Sources/Player/HeadlessPlayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,7 @@ public protocol HeadlessPlayer {
public extension HeadlessPlayer {
/// The current state of Player
var state: BaseFlowState? {
guard
let jsState = jsPlayerReference?.invokeMethod("getState", withArguments: [])
else { return nil }
return BaseFlowState.createInstance(value: jsState)
return jsPlayerReference?.getState()
}

/**
Expand Down Expand Up @@ -221,6 +218,10 @@ public extension HeadlessPlayer {
- completion: A completion handler for when the flow has completed
*/
func start(flow: String, completion: @escaping (Result<CompletedState, PlayerError>) -> Void) {
// declare these variables outside and reference them inside the errorHandler to prevent retain cycle
let playerRef = jsPlayerReference
let loggerRef = logger

let promiseHandler: @convention(block) (JSValue?) -> Void = { completedState in
guard
let result = CompletedState.createInstance(from: completedState)
Expand All @@ -231,17 +232,17 @@ public extension HeadlessPlayer {
}
let errorHandler: @convention(block) (JSValue?) -> Void = { _ in
guard
let result = self.state as? ErrorState
else {
let result = playerRef?.getState() as? ErrorState else {
return completion(.failure(PlayerError.jsConversionFailure))
}
logger.e(result.error)

loggerRef.e(result.error)
completion(.failure(PlayerError.promiseRejected(error: result)))
}

// Ensure these get created because otherwise we will never know when the flow ends/errors
guard
let context = jsPlayerReference?.context,
let context = playerRef?.context,
let flowObject = context.evaluateScript("(\(flow))"),
!flowObject.isUndefined,
let callback = JSValue(object: promiseHandler, in: context),
Expand All @@ -251,7 +252,7 @@ public extension HeadlessPlayer {
}

// Should not be possible due to fatalError in constructor, but just for handling optionals safely
guard let player = jsPlayerReference else { return completion(.failure(PlayerError.playerNotInstantiated)) }
guard let player = playerRef else { return completion(.failure(PlayerError.playerNotInstantiated)) }
player
.invokeMethod("start", withArguments: [flowObject])
.invokeMethod("then", withArguments: [callback])
Expand Down Expand Up @@ -347,3 +348,12 @@ internal extension JSContext {
return value
}
}

private extension JSValue {
// put getState in extension so it can be accessed by the computed property in HeadlessPlayer.state and also inside the ErrorHandler by the playerReference
func getState() -> BaseFlowState? {
guard let jsState = self.invokeMethod("getState", withArguments: [])
else { return nil }
return BaseFlowState.createInstance(value: jsState)
}
}
3 changes: 3 additions & 0 deletions ios/swiftui/Sources/ManagedPlayer/ManagedPlayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ internal struct ManagedPlayer14<Loading: View, Fallback: View>: View {

public var body: some View {
bodyContent(viewModel.stateTransition.call() ?? .identity)
.onDisappear {
context.clearExceptionHandler()
}
}

private func bodyContent(_ transitionInfo: PlayerViewTransition) -> some View {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,12 @@ public class ManagedPlayerViewModel: ObservableObject, NativePlugin {
}

public func apply<P>(player: P) where P: HeadlessPlayer {
player.hooks?.state.tap({ (state) in
player.hooks?.state.tap({ [weak self] (state) in
guard let inProgress = state as? InProgressState else {
self.currentState = nil
self?.currentState = nil
return
}
self.currentState = inProgress
self?.currentState = inProgress
})
}

Expand Down
10 changes: 8 additions & 2 deletions ios/swiftui/Sources/SwiftUIPlayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ public struct SwiftUIPlayer: View, HeadlessPlayer {
self.onViewController(controller)
}

hooks.state.tap { newState in
self.state = newState
hooks.state.tap { [weak self] newState in
self?.state = newState
}

guard !flow.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {
Expand All @@ -109,6 +109,12 @@ public struct SwiftUIPlayer: View, HeadlessPlayer {
registry.resetView()
}

/// Clear the exceptionHandler of the context to remove reference to the logger
/// should be called when ManagedPlayer gets tore down
public func clearExceptionHandler() {
player?.context.exceptionHandler = nil
}

/// Returns `player` but asserts that it is not nil. Used from methods that should not be called
/// when we are unloaded.
private var expectedPlayer: JSValue? {
Expand Down