Merge pull request #88 from bitmovin/feature/fix-background-thread-pl…

…ayer-access Fix background thread player access which could result in a crash
bitmovin · Aug 26, 2024 · 24e2b88 · 24e2b88
2 parents ce872fb + 733202c
commit 24e2b88
Show file tree

Hide file tree

Showing 19 changed files with 198 additions and 137 deletions.
diff --git a/BitmovinConvivaAnalytics/Classes/ConvivaAnalytics.swift b/BitmovinConvivaAnalytics/Classes/ConvivaAnalytics.swift
@@ -130,10 +130,6 @@ public final class ConvivaAnalytics: NSObject {
             attach(player: player)
         }
 
-        adAnalytics.setUpdateHandler { [weak self] in
-            self?.handleAdUpdateRequest()
-        }
-
         ssai.delegate = self
     }
 
@@ -517,22 +513,18 @@ private extension ConvivaAnalytics {
         logger.debugLog(message: "Player state changed: \(playerState.rawValue)")
     }
 
-    private func handleAdUpdateRequest() {
-        guard let player, isAd else { return }
-
-        adAnalytics.reportPlaybackMetric(
-            CIS_SSDK_PLAYBACK_METRIC_PLAY_HEAD_TIME,
-            value: Int64(player.currentTime(.relativeTime) * 1_000)
-        )
-    }
-
     private func reportPlayHeadTime() {
         guard let player, isSessionActive else { return }
 
-        videoAnalytics.reportPlaybackMetric(
-            CIS_SSDK_PLAYBACK_METRIC_PLAY_HEAD_TIME,
-            value: Int64(player.currentTime(.relativeTime) * 1_000)
-        )
+        let reportPlayHeadTime: (_ analytics: CISStreamAnalyticsProtocol) -> Void = { analytics in
+            let currentTime = Int64(player.currentTime(.relativeTime) * 1_000)
+            analytics.reportPlaybackMetric(
+                CIS_SSDK_PLAYBACK_METRIC_PLAY_HEAD_TIME,
+                value: currentTime
+            )
+        }
+
+        reportPlayHeadTime(isAd ? adAnalytics : videoAnalytics)
     }
 
     private func buildAdInfo(adStartedEvent: AdStartedEvent, player: Player) -> [String: Any] {
@@ -625,11 +617,12 @@ extension ConvivaAnalytics: BitmovinPlayerListenerDelegate {
     }
 
     func onTimeChanged(player: Player) {
+        reportPlayHeadTime()
+
         guard !player.isAd else {
             return
         }
 
-        reportPlayHeadTime()
         updateSession()
     }
 

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [Unreleased]
+
+### Fixed
+- Potential crash during ad tracking due to accessing Player APIs on a background thread
+
 ## [3.4.0] - 2024-08-26
 
 ### Added

diff --git a/Example/BitmovinConvivaAnalytics.xcodeproj/project.pbxproj b/Example/BitmovinConvivaAnalytics.xcodeproj/project.pbxproj
@@ -16,6 +16,7 @@
 		37495D472C06010800B49BBE /* DefaultPlayerConfig.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37495D432C06010800B49BBE /* DefaultPlayerConfig.swift */; };
 		37495D482C06014100B49BBE /* DefaultAdConfig.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37495D422C06010800B49BBE /* DefaultAdConfig.swift */; };
 		37495D492C06014300B49BBE /* DefaultPlayerConfig.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37495D432C06010800B49BBE /* DefaultPlayerConfig.swift */; };
+		375B06562C7CCB3A0045768A /* TestConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 375B06552C7CCB3A0045768A /* TestConfiguration.swift */; };
 		37756EB0216F8E0D0041806D /* TestDouble.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EAF216F8E0D0041806D /* TestDouble.swift */; };
 		37756EB2216F8E2F0041806D /* Spy.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EB1216F8E2F0041806D /* Spy.swift */; };
 		37756EB4216F8E450041806D /* TestDoubleDataSource.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EB3216F8E450041806D /* TestDoubleDataSource.swift */; };
@@ -26,7 +27,6 @@
 		37756EBE216F99C80041806D /* PlayerEventsTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EBD216F99C80041806D /* PlayerEventsTest.swift */; };
 		37756EC0216F9A510041806D /* ContentMetadataTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EBF216F9A510041806D /* ContentMetadataTest.swift */; };
 		37756EC2216F9AA50041806D /* CustomEventsTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EC1216F9AA50041806D /* CustomEventsTest.swift */; };
-		37756EC42170764E0041806D /* SpecHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37756EC32170764E0041806D /* SpecHelper.swift */; };
 		37B615A02C775567009511D9 /* SourceTestDouble.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37B6159F2C77555E009511D9 /* SourceTestDouble.swift */; };
 		37C28E5B2C7885E800737FBC /* LatePlayerAttachingTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37C28E5A2C7885E800737FBC /* LatePlayerAttachingTest.swift */; };
 		37EB3CD3216B70D70093F085 /* TestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37EB3CD1216B70650093F085 /* TestHelper.swift */; };
@@ -72,6 +72,7 @@
 		37226EC822043A9D0072DE47 /* ExternallyManagedSessionTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ExternallyManagedSessionTest.swift; sourceTree = "<group>"; };
 		37495D422C06010800B49BBE /* DefaultAdConfig.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DefaultAdConfig.swift; sourceTree = "<group>"; };
 		37495D432C06010800B49BBE /* DefaultPlayerConfig.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DefaultPlayerConfig.swift; sourceTree = "<group>"; };
+		375B06552C7CCB3A0045768A /* TestConfiguration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestConfiguration.swift; sourceTree = "<group>"; };
 		37756EAF216F8E0D0041806D /* TestDouble.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = TestDouble.swift; path = Helpers/TestDouble.swift; sourceTree = "<group>"; };
 		37756EB1216F8E2F0041806D /* Spy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = Spy.swift; path = Helpers/Spy.swift; sourceTree = "<group>"; };
 		37756EB3216F8E450041806D /* TestDoubleDataSource.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = TestDoubleDataSource.swift; path = Helpers/TestDoubleDataSource.swift; sourceTree = "<group>"; };
@@ -82,7 +83,6 @@
 		37756EBD216F99C80041806D /* PlayerEventsTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PlayerEventsTest.swift; sourceTree = "<group>"; };
 		37756EBF216F9A510041806D /* ContentMetadataTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ContentMetadataTest.swift; sourceTree = "<group>"; };
 		37756EC1216F9AA50041806D /* CustomEventsTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomEventsTest.swift; sourceTree = "<group>"; };
-		37756EC32170764E0041806D /* SpecHelper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SpecHelper.swift; sourceTree = "<group>"; };
 		37B6159E2C774236009511D9 /* CHANGELOG.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = CHANGELOG.md; path = ../CHANGELOG.md; sourceTree = SOURCE_ROOT; };
 		37B6159F2C77555E009511D9 /* SourceTestDouble.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SourceTestDouble.swift; sourceTree = "<group>"; };
 		37C28E5A2C7885E800737FBC /* LatePlayerAttachingTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LatePlayerAttachingTest.swift; sourceTree = "<group>"; };
@@ -281,18 +281,18 @@
 		607FACE81AFB9204008FA782 /* Tests */ = {
 			isa = PBXGroup;
 			children = (
+				375B06552C7CCB3A0045768A /* TestConfiguration.swift */,
 				37756EBD216F99C80041806D /* PlayerEventsTest.swift */,
 				A8ECFE352C348BC50037C9D7 /* SsaiTest.swift */,
 				37756EBF216F9A510041806D /* ContentMetadataTest.swift */,
 				37756EC1216F9AA50041806D /* CustomEventsTest.swift */,
 				607FACEB1AFB9204008FA782 /* SeekTimeshiftTest.swift */,
 				37226EC822043A9D0072DE47 /* ExternallyManagedSessionTest.swift */,
-				37756EC32170764E0041806D /* SpecHelper.swift */,
 				37EB3CD1216B70650093F085 /* TestHelper.swift */,
+				37C28E5A2C7885E800737FBC /* LatePlayerAttachingTest.swift */,
 				37756EAE216F8DF00041806D /* Helpers */,
 				37EB3CC6216B6D2B0093F085 /* Doubles */,
 				607FACE91AFB9204008FA782 /* Supporting Files */,
-				37C28E5A2C7885E800737FBC /* LatePlayerAttachingTest.swift */,
 			);
 			path = Tests;
 			sourceTree = "<group>";
@@ -741,8 +741,8 @@
 				44D4DC562841201F00C0BE9D /* CISAdAnalyticsTestDouble.swift in Sources */,
 				A8ECFE362C348BC50037C9D7 /* SsaiTest.swift in Sources */,
 				44D4DC502841032B00C0BE9D /* CISAnalyticsCreatorTestDouble.swift in Sources */,
+				375B06562C7CCB3A0045768A /* TestConfiguration.swift in Sources */,
 				37756EBC216F90020041806D /* MockTracker.swift in Sources */,
-				37756EC42170764E0041806D /* SpecHelper.swift in Sources */,
 				37756EB6216F8E700041806D /* CustomNimbleMatchers.swift in Sources */,
 				607FACEC1AFB9204008FA782 /* SeekTimeshiftTest.swift in Sources */,
 				37756EC0216F9A510041806D /* ContentMetadataTest.swift in Sources */,

diff --git a/Example/BitmovinConvivaAnalytics/ViewController.swift b/Example/BitmovinConvivaAnalytics/ViewController.swift
@@ -114,6 +114,7 @@ class ViewController: UIViewController {
 
     @IBAction func destroyPlayer(_ sender: Any) {
         convivaAnalytics?.release()
+        convivaAnalytics = nil
         player?.unload()
         player?.destroy()
         player = nil

diff --git a/Example/Tests/CISAdAnalyticsTestDouble.swift b/Example/Tests/CISAdAnalyticsTestDouble.swift
@@ -124,9 +124,8 @@ class CISAdAnalyticsTestDouble: NSObject, CISAdAnalyticsProtocol, TestDoubleData
 
     func reportPlaybackMetric(_ key: String, value: Any?) {
         spy(functionName: "reportPlaybackMetric", args: [
-               "key": key,
-               "value": "\(value ?? "")"
-           ])
+            key: "\(value ?? "")"
+        ])
     }
 
     func setUpdateHandler(_ updateHandler: @escaping UpdateHandler) {

diff --git a/Example/Tests/CISAnalyticsTestDouble.swift b/Example/Tests/CISAnalyticsTestDouble.swift
@@ -63,5 +63,7 @@ class CISAnalyticsTestDouble: NSObject, CISAnalyticsProtocol, TestDoubleDataSour
     func setUserPrefsForDataDeletion(_ userPrefs: [AnyHashable: Any]) {
     }
 
-    func cleanup() {}
+    func cleanup() {
+        spy(functionName: "cleanup")
+    }
 }
diff --git a/Example/Tests/CISVideoAnalyticsTestDouble.swift b/Example/Tests/CISVideoAnalyticsTestDouble.swift
@@ -115,5 +115,6 @@ class CISVideoAnalyticsTestDouble: NSObject, CISVideoAnalyticsProtocol, TestDoub
     }
 
     func cleanup() {
+        spy(functionName: "cleanup")
     }
 }
diff --git a/Example/Tests/ContentMetadataTest.swift b/Example/Tests/ContentMetadataTest.swift
@@ -344,7 +344,7 @@ class ContentMetadataTest: QuickSpec {
                         let sourceConfig = SourceConfig(url: URL(string: "http://a.url")!, type: .hls)
                         sourceConfig.title = "MyTitle"
                         let source = SourceFactory.createSource(from: sourceConfig)
-                        playerDouble.load(source: source)
+                        _ = TestDouble(aClass: playerDouble!, name: "source", return: source)
 
                         _ = TestDouble(aClass: playerDouble!, name: "config", return: playerConfig)
 

diff --git a/Example/Tests/Doubles/BitmovinPlayerTestDouble.swift b/Example/Tests/Doubles/BitmovinPlayerTestDouble.swift
@@ -284,6 +284,22 @@ class BitmovinPlayerTestDouble: BitmovinPlayerStub, TestDoubleDataSource {
         }
         return player.currentTime
     }
+
+    override var isAd: Bool {
+        if let mockedValue = mocks["isAd"] {
+            // swiftlint:disable:next force_cast
+            return mockedValue as! Bool
+        }
+        return player.isAd
+    }
+
+    override func currentTime(_ timeMode: TimeMode) -> TimeInterval {
+        if let mockedValue = mocks["currentTime(_:)"] {
+            // swiftlint:disable:next force_cast
+            return mockedValue as! TimeInterval
+        }
+        return player.currentTime(timeMode)
+    }
 }
 
 class TestAdBreak: NSObject, AdBreak {
@@ -337,7 +353,7 @@ class BitmovinPlayerStub: NSObject, Player {
     override init() {
         let config = PlayerConfig()
         config.key = "foobar"
-        player = PlayerFactory.createPlayer(playerConfig: config)
+        player = PlayerFactory.createPlayer(playerConfig: config, analytics: .disabled)
     }
 
     var isDestroyed: Bool {
@@ -503,15 +519,12 @@ class BitmovinPlayerStub: NSObject, Player {
     }
 
     func load(sourceConfig: SourceConfig) {
-        player.load(sourceConfig: sourceConfig)
     }
 
     func load(source: Source) {
-        player.load(source: source)
     }
 
     func load(playlistConfig: PlaylistConfig) {
-        player.load(playlistConfig: playlistConfig)
     }
 
     func unload() {

diff --git a/Example/Tests/ExternallyManagedSessionTest.swift b/Example/Tests/ExternallyManagedSessionTest.swift
@@ -82,7 +82,7 @@ class ExternallyManagedSessionTest: QuickSpec {
                         let sourceConfig = SourceConfig(url: URL(string: "http://a.url")!, type: .hls)
                         sourceConfig.title = "MyTitle"
                         let source = SourceFactory.createSource(from: sourceConfig)
-                        playerDouble.load(source: source)
+                        _ = TestDouble(aClass: playerDouble!, name: "source", return: source)
                         try? convivaAnalytics.initializeSession()
                         expect(spy).to(haveBeenCalled(withArgs: ["assetName": "MyTitle"]))
                     }
@@ -93,7 +93,7 @@ class ExternallyManagedSessionTest: QuickSpec {
                         convivaAnalytics.updateContentMetadata(metadataOverrides: metadata)
                         let sourceConfig = SourceConfig(url: URL(string: "http://a.url")!, type: .hls)
                         let source = SourceFactory.createSource(from: sourceConfig)
-                        playerDouble.load(source: source)
+                        _ = TestDouble(aClass: playerDouble!, name: "source", return: source)
 
                         try? convivaAnalytics.initializeSession()
                         expect(spy).to(haveBeenCalled(withArgs: ["assetName": "A Override"]))
@@ -102,7 +102,7 @@ class ExternallyManagedSessionTest: QuickSpec {
                     it("throw error without title in the source and without asset name") {
                         let sourceConfig = SourceConfig(url: URL(string: "http://a.url")!, type: .hls)
                         let source = SourceFactory.createSource(from: sourceConfig)
-                        playerDouble.load(source: source)
+                        _ = TestDouble(aClass: playerDouble!, name: "source", return: source)
                         expect { try convivaAnalytics.initializeSession() }.to(throwError())
                         expect(spy).toNot(haveBeenCalled())
                     }
@@ -157,7 +157,7 @@ class ExternallyManagedSessionTest: QuickSpec {
                     let sourceConfig = SourceConfig(url: URL(string: "http://a.url")!, type: .hls)
                     sourceConfig.title = "MyTitle"
                     let source = SourceFactory.createSource(from: sourceConfig)
-                    playerDouble.load(source: source)
+                    _ = TestDouble(aClass: playerDouble!, name: "source", return: source)
                     try? convivaAnalytics.initializeSession()
                     expect(spy).to(haveBeenCalled(withArgs: ["assetName": "MyTitle"]))
                 }

diff --git a/Example/Tests/Helpers/CustomNimbleMatchers.swift b/Example/Tests/Helpers/CustomNimbleMatchers.swift
@@ -11,40 +11,41 @@ import Nimble
 
 public func haveBeenCalled<T>(withArgs args: [String: String]? = nil) -> Matcher<T> {
     Matcher { (actualExpression: Nimble.Expression<T>) throws -> MatcherResult in
-        if let functionName = (try? actualExpression.evaluate() as? Spy)??.functionName {
-            let spyTracker = TestHelper.shared.spyTracker
+        guard let spy = try? actualExpression.evaluate() as? Spy else {
+            let message = ExpectationMessage.fail("Invalid Spy")
+            return MatcherResult(bool: false, message: message)
+        }
+
+        let functionName = spy.functionName
+        let spyTracker = TestHelper.shared.spyTracker
 
-            var spyResult = spyTracker.hasCalledFunction(functionName)
-            let spyWasCalled: Bool = spyResult.success
-            var argsAreMatching = true // expect best case for the case no args where expected
+        var spyResult = spyTracker.hasCalledFunction(spy: spy, functionName)
+        let spyWasCalled: Bool = spyResult.success
+        var argsAreMatching = true // expect best case for the case no args where expected
 
-            let message: ExpectationMessage!
-            if spyWasCalled {
-                if let expectedArgs = args {
-                    spyResult = spyTracker.hasCalledFunction(functionName, withArgs: expectedArgs)
-                    argsAreMatching = spyResult.success
+        let message: ExpectationMessage!
+        if spyWasCalled {
+            if let expectedArgs = args {
+                spyResult = spyTracker.hasCalledFunction(spy: spy, functionName, withArgs: expectedArgs)
+                argsAreMatching = spyResult.success
 
-                    let messageString = """
-                                        have called <\(functionName)> \
-                                        with args<\(expectedArgs.toStringWithStableOrder())> \
-                                        got <\(spyResult.trackedArgs)>
-                                        """
-                    message = ExpectationMessage.expectedTo(messageString)
-                } else {
-                    // Success message (will never be shown but is needed)
-                    message = ExpectationMessage.expectedTo("have called <\(functionName)>")
-                }
+                let messageString = """
+                                    have called <\(functionName)> \
+                                    with args<\(expectedArgs.toStringWithStableOrder())> \
+                                    got <\(spyResult.trackedArgs)>
+                                    """
+                message = ExpectationMessage.expectedTo(messageString)
             } else {
-                message = ExpectationMessage.expectedTo("have called <\(functionName)> but was not called")
+                // Success message (will never be shown but is needed)
+                message = ExpectationMessage.expectedTo("have called <\(functionName)>")
             }
-
-            return MatcherResult(
-                bool: spyWasCalled && argsAreMatching,
-                message: message
-            )
+        } else {
+            message = ExpectationMessage.expectedTo("have called <\(functionName)> but was not called")
         }
 
-        let message = ExpectationMessage.fail("Invalid Spy")
-        return MatcherResult(bool: false, message: message)
+        return MatcherResult(
+            bool: spyWasCalled && argsAreMatching,
+            message: message
+        )
     }
 }
diff --git a/Example/Tests/Helpers/Spy.swift b/Example/Tests/Helpers/Spy.swift
@@ -9,6 +9,6 @@
 import Foundation
 
 struct Spy {
-    let aClass: Any
+    let aClass: AnyObject
     let functionName: String
 }
diff --git a/Example/Tests/Helpers/SpyTracker.swift b/Example/Tests/Helpers/SpyTracker.swift
@@ -9,23 +9,24 @@
 import Foundation
 
 class SpyTracker {
-    var spies: [String: [[String: String]?]] = [:]
+    var spies: [ObjectIdentifier: [String: [[String: String]?]]] = [:]
 
-    func track(functionName: String, args: [String: String]? = nil) {
-        spies[functionName, default: []].append(args)
+    func track(aClass: AnyObject, functionName: String, args: [String: String]? = nil) {
+        spies[ObjectIdentifier(aClass), default: [:]][functionName, default: []].append(args)
     }
 
     func hasCalledFunction(
+        spy: Spy,
         _ name: String,
         withArgs args: [String: String]? = nil
     ) -> (success: Bool, trackedArgs: [[String: String]?]) {
-        let called = spies.keys.contains(name)
+        let called = spies[ObjectIdentifier(spy.aClass)]?.keys.contains(name) ?? false
         if !called {
             return (false, [])
         }
 
         if let expectedArgs = args {
-            if let spyCalls = spies[name] {
+            if let spyCalls = spies[ObjectIdentifier(spy.aClass)]?[name] {
                 for calledArgs in spyCalls {
                     guard let calledArgs else { continue }
 

diff --git a/Example/Tests/Helpers/TestDoubleDataSource.swift b/Example/Tests/Helpers/TestDoubleDataSource.swift
@@ -8,7 +8,7 @@
 
 import Foundation
 
-protocol TestDoubleDataSource {
+protocol TestDoubleDataSource: AnyObject {
     var mocks: [String: Any] { get }
     func spy(functionName: String, args: [String: String]?)
 }
@@ -19,6 +19,10 @@ extension TestDoubleDataSource {
     }
 
     func spy(functionName: String, args: [String: String]? = nil) {
-        TestHelper.shared.spy(functionName: functionName, args: args)
+        TestHelper.shared.spy(
+            aClass: type(of: self),
+            functionName: functionName,
+            args: args
+        )
     }
 }
diff --git a/Example/Tests/Info.plist b/Example/Tests/Info.plist
@@ -22,5 +22,7 @@
 	<string>PLAYER-LICENSE-KEY</string>
 	<key>CFBundleVersion</key>
 	<string>1</string>
+	<key>NSPrincipalClass</key>
+	<string>BitmovinConvivaAnalytics_Tests.TestConfiguration</string>
 </dict>
 </plist>
-Original file line number
+Diff line change
@@ Expand Up @@
         }
         func cleanup() {
+            spy(functionName: "cleanup")
         }
     }