RUM-1978 Fix RUM ViewController leak

CHANGELOG.md

@@ -2,6 +2,7 @@
 - [BUGFIX] Optimize Session Replay diffing algorithm. See [#1524][]
 - [FEATURE] Add network instrumentation for async/await URLSession APIs. See [#1394][]
 - [FEATURE] Change default tracing headers for first party hosts to use both Datadog headers and W3C `tracecontext` headers. See [#1529][]
+- [BUGFIX] Fix RUM ViewController leaks. See [#1533][]
 
 # 2.4.0 / 18-10-2023
 
@@ -550,6 +551,7 @@ Release `2.0` introduces breaking changes. Follow the [Migration Guide](MIGRATIO
 [#1394]: https://github.com/DataDog/dd-sdk-ios/pull/1394
 [#1524]: https://github.com/DataDog/dd-sdk-ios/pull/1524
 [#1529]: https://github.com/DataDog/dd-sdk-ios/pull/1529
+[#1533]: https://github.com/DataDog/dd-sdk-ios/pull/1533
 [@00fa9a]: https://github.com/00FA9A
 [@britton-earnin]: https://github.com/Britton-Earnin
 [@hengyu]: https://github.com/Hengyu

DatadogCore/Tests/Datadog/Mocks/RUMFeatureMocks.swift

@@ -168,7 +168,7 @@ extension RUMStartViewCommand: AnyMockable, RandomMockable {
         return .mockWith(
             time: .mockRandomInThePast(),
             attributes: mockRandomAttributes(),
-            identity: String.mockRandom(),
+            identity: String.mockRandom().asRUMViewIdentity(),
             name: .mockRandom(),
             path: .mockRandom()
         )
@@ -177,7 +177,7 @@ extension RUMStartViewCommand: AnyMockable, RandomMockable {
     static func mockWith(
         time: Date = Date(),
         attributes: [AttributeKey: AttributeValue] = [:],
-        identity: RUMViewIdentifiable = mockView,
+        identity: RUMViewIdentity = mockViewIdentity,
         name: String = .mockAny(),
         path: String? = nil
     ) -> RUMStartViewCommand {
@@ -198,14 +198,14 @@ extension RUMStopViewCommand: AnyMockable, RandomMockable {
         return .mockWith(
             time: .mockRandomInThePast(),
             attributes: mockRandomAttributes(),
-            identity: String.mockRandom()
+            identity: String.mockRandom().asRUMViewIdentity()
         )
     }
 
     static func mockWith(
         time: Date = Date(),
         attributes: [AttributeKey: AttributeValue] = [:],
-        identity: RUMViewIdentifiable = mockView
+        identity: RUMViewIdentity = mockViewIdentity
     ) -> RUMStopViewCommand {
         return RUMStopViewCommand(
             time: time, attributes: attributes, identity: identity
@@ -786,6 +786,7 @@ func createMockView(viewControllerClassName: String) -> UIViewController {
 
 ///// Holds the `mockView` object so it can be weakly referenced by `RUMViewScope` mocks.
 let mockView: UIViewController = createMockViewInWindow()
+let mockViewIdentity = mockView.asRUMViewIdentity()
 
 extension RUMViewScope {
     static func mockAny() -> RUMViewScope {
@@ -802,7 +803,7 @@ extension RUMViewScope {
         isInitialView: Bool = false,
         parent: RUMContextProvider = RUMContextProviderMock(),
         dependencies: RUMScopeDependencies = .mockAny(),
-        identity: RUMViewIdentifiable = mockView,
+        identity: RUMViewIdentity = mockViewIdentity,
         path: String = .mockAny(),
         name: String = .mockAny(),
         attributes: [AttributeKey: AttributeValue] = [:],

DatadogCore/Tests/Datadog/RUM/RUMDebuggingTests.swift

@@ -22,7 +22,7 @@ class RUMDebuggingTests: XCTestCase {
             dependencies: .mockWith(rumApplicationID: "rum-123")
         )
         _ = applicationScope.process(
-            command: RUMStartViewCommand.mockWith(identity: mockView, name: "FirstView"),
+            command: RUMStartViewCommand.mockWith(identity: mockViewIdentity, name: "FirstView"),
             context: .mockAny(),
             writer: FileWriterMock()
         )
@@ -56,7 +56,7 @@ class RUMDebuggingTests: XCTestCase {
             dependencies: .mockWith(rumApplicationID: "rum-123")
         )
         _ = applicationScope.process(
-            command: RUMStartViewCommand.mockWith(identity: mockView, name: "FirstView"),
+            command: RUMStartViewCommand.mockWith(identity: mockViewIdentity, name: "FirstView"),
             context: context,
             writer: writer
         )
@@ -66,7 +66,7 @@ class RUMDebuggingTests: XCTestCase {
             writer: writer
         )
         _ = applicationScope.process(
-            command: RUMStartViewCommand.mockWith(identity: mockView, name: "SecondView"),
+            command: RUMStartViewCommand.mockWith(identity: mockViewIdentity, name: "SecondView"),
             context: context,
             writer: writer
         )

DatadogRUM/Sources/Instrumentation/Views/RUMViewsHandler.swift

@@ -151,14 +151,14 @@ internal final class RUMViewsHandler {
             return
         }
 
-        guard let identity = view.identity.identifiable else {
+        guard view.identity.isIdentifiable else {
             return
         }
 
         subscriber.process(
             command: RUMStartViewCommand(
                 time: dateProvider.now,
-                identity: identity,
+                identity: view.identity,
                 name: view.name,
                 path: view.path,
                 attributes: view.attributes
@@ -167,7 +167,7 @@ internal final class RUMViewsHandler {
     }
 
     private func stop(view: View) {
-        guard let identity = view.identity.identifiable else {
+        guard view.identity.isIdentifiable else {
             return
         }
 
@@ -179,7 +179,7 @@ internal final class RUMViewsHandler {
             command: RUMStopViewCommand(
                 time: dateProvider.now,
                 attributes: [:],
-                identity: identity
+                identity: view.identity
             )
         )
     }
@@ -201,14 +201,15 @@ internal final class RUMViewsHandler {
 
 extension RUMViewsHandler: UIViewControllerHandler {
     func notify_viewDidAppear(viewController: UIViewController, animated: Bool) {
-        if let view = stack.first(where: { $0.identity.equals(viewController) }) {
+        let identity = viewController.asRUMViewIdentity()
+        if let view = stack.first(where: { $0.identity.equals(identity) }) {
             // If the stack already contains the view controller, just restarts the view.
             // This prevents from calling the predicate when unnecessary.
             add(view: view)
         } else if let rumView = predicate?.rumView(for: viewController) {
             add(
                 view: .init(
-                    identity: viewController.asRUMViewIdentity(),
+                    identity: identity,
                     name: rumView.name,
                     path: rumView.path,
                     isUntrackedModal: rumView.isUntrackedModal,
@@ -218,7 +219,7 @@ extension RUMViewsHandler: UIViewControllerHandler {
         } else if #available(iOS 13, tvOS 13, *), viewController.isModalInPresentation {
             add(
                 view: .init(
-                    identity: viewController.asRUMViewIdentity(),
+                    identity: identity,
                     name: "RUMUntrackedModal",
                     path: nil,
                     isUntrackedModal: true,

DatadogRUM/Sources/RUMMonitor/Monitor.swift

@@ -221,7 +221,7 @@ extension Monitor: RUMMonitorProtocol {
         process(
             command: RUMStartViewCommand(
                 time: dateProvider.now,
-                identity: viewController,
+                identity: viewController.asRUMViewIdentity(),
                 name: name,
                 path: nil,
                 attributes: attributes
@@ -234,7 +234,7 @@ extension Monitor: RUMMonitorProtocol {
             command: RUMStopViewCommand(
                 time: dateProvider.now,
                 attributes: attributes,
-                identity: viewController
+                identity: viewController.asRUMViewIdentity()
             )
         )
     }
@@ -243,7 +243,7 @@ extension Monitor: RUMMonitorProtocol {
         process(
             command: RUMStartViewCommand(
                 time: dateProvider.now,
-                identity: key,
+                identity: key.asRUMViewIdentity(),
                 name: name ?? key,
                 path: key,
                 attributes: attributes
@@ -256,7 +256,7 @@ extension Monitor: RUMMonitorProtocol {
             command: RUMStopViewCommand(
                 time: dateProvider.now,
                 attributes: attributes,
-                identity: key
+                identity: key.asRUMViewIdentity()
             )
         )
     }

DatadogRUM/Sources/RUMMonitor/RUMCommand.swift

@@ -54,7 +54,7 @@ internal struct RUMStartViewCommand: RUMCommand, RUMViewScopePropagatableAttribu
     let isUserInteraction = true // a new View means there was a navigation, it's considered a User interaction
 
     /// The value holding stable identity of the RUM View.
-    let identity: RUMViewIdentifiable
+    let identity: RUMViewIdentity
 
     /// The name of this View, rendered in RUM Explorer as `VIEW NAME`.
     let name: String
@@ -64,7 +64,7 @@ internal struct RUMStartViewCommand: RUMCommand, RUMViewScopePropagatableAttribu
 
     init(
         time: Date,
-        identity: RUMViewIdentifiable,
+        identity: RUMViewIdentity,
         name: String?,
         path: String?,
         attributes: [AttributeKey: AttributeValue]
@@ -84,7 +84,7 @@ internal struct RUMStopViewCommand: RUMCommand, RUMViewScopePropagatableAttribut
     let isUserInteraction = false // a view can be stopped and in most cases should not be considered an interaction (if it's stopped because the user navigate inside the same app, the startView will happen shortly after this)
 
     /// The value holding stable identity of the RUM View.
-    let identity: RUMViewIdentifiable
+    let identity: RUMViewIdentity
 }
 
 internal struct RUMAddCurrentViewErrorCommand: RUMCommand {

DatadogRUM/Sources/RUMMonitor/Scopes/RUMSessionScope.swift

@@ -83,14 +83,13 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider {
             didStartWithReplay: hasReplay
         )
 
-        if let viewScope = resumingViewScope,
-           let viewIdentifiable = viewScope.identity.identifiable {
+        if let viewScope = resumingViewScope, viewScope.identity.isIdentifiable {
             viewScopes.append(
                 RUMViewScope(
                     isInitialView: false,
                     parent: self,
                     dependencies: dependencies,
-                    identity: viewIdentifiable,
+                    identity: viewScope.identity,
                     path: viewScope.viewPath,
                     name: viewScope.viewName,
                     attributes: viewScope.attributes,
@@ -121,14 +120,14 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider {
 
         // Transfer active Views by creating new `RUMViewScopes` for their identity objects:
         self.viewScopes = expiredSession.viewScopes.compactMap { expiredView in
-            guard let expiredViewIdentifiable = expiredView.identity.identifiable else {
+            guard expiredView.identity.isIdentifiable else {
                 return nil // if the underlying identifiable (`UIVIewController`) no longer exists, skip transferring its scope
             }
             return RUMViewScope(
                 isInitialView: false,
                 parent: self,
                 dependencies: dependencies,
-                identity: expiredViewIdentifiable,
+                identity: expiredView.identity,
                 path: expiredView.viewPath,
                 name: expiredView.viewName,
                 attributes: expiredView.attributes,
@@ -233,7 +232,7 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider {
             isInitialView: true,
             parent: self,
             dependencies: dependencies,
-            identity: RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewURL,
+            identity: RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewURL.asRUMViewIdentity(),
             path: RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewURL,
             name: RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName,
             attributes: command.attributes,
@@ -278,7 +277,7 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider {
                 isInitialView: isStartingInitialView,
                 parent: self,
                 dependencies: dependencies,
-                identity: RUMOffViewEventsHandlingRule.Constants.backgroundViewURL,
+                identity: RUMOffViewEventsHandlingRule.Constants.backgroundViewURL.asRUMViewIdentity(),
                 path: RUMOffViewEventsHandlingRule.Constants.backgroundViewURL,
                 name: RUMOffViewEventsHandlingRule.Constants.backgroundViewName,
                 attributes: command.attributes,

DatadogRUM/Sources/RUMMonitor/Scopes/RUMViewScope.swift

@@ -96,7 +96,7 @@ internal class RUMViewScope: RUMScope, RUMContextProvider {
         isInitialView: Bool,
         parent: RUMContextProvider,
         dependencies: RUMScopeDependencies,
-        identity: RUMViewIdentifiable,
+        identity: RUMViewIdentity,
         path: String,
         name: String,
         attributes: [AttributeKey: AttributeValue],
@@ -107,7 +107,7 @@ internal class RUMViewScope: RUMScope, RUMContextProvider {
         self.parent = parent
         self.dependencies = dependencies
         self.isInitialView = isInitialView
-        self.identity = identity.asRUMViewIdentity()
+        self.identity = identity
         self.attributes = attributes
         self.customTimings = customTimings
         self.viewUUID = dependencies.rumUUIDGenerator.generateUnique()

DatadogRUM/Sources/RUMMonitor/Scopes/Utils/RUMViewIdentity.swift

@@ -10,7 +10,7 @@ import UIKit
 /// Based on the `equals(_:)` implementation, it decides if two `RUMViewIdentifiables` identify the same
 /// RUM View or not. Each implementation of the `RUMViewIdentifiable` decides by its own if it should use
 /// reference or value semantic for the comparison.
-internal protocol RUMViewIdentifiable {
+fileprivate protocol RUMViewIdentifiable {
     /// Compares the instance of this identifiable with another `RUMViewIdentifiable`.
     /// It returns `true` if both identify the same RUM View and `false` otherwise.
     func equals(_ otherIdentifiable: RUMViewIdentifiable?) -> Bool
@@ -27,7 +27,7 @@ internal protocol RUMViewIdentifiable {
 
 /// Extends `UIViewController` with the ability to identify the RUM View.
 extension UIViewController: RUMViewIdentifiable {
-    func equals(_ otherIdentifiable: RUMViewIdentifiable?) -> Bool {
+    fileprivate func equals(_ otherIdentifiable: RUMViewIdentifiable?) -> Bool {
         if let otherViewController = otherIdentifiable as? UIViewController {
             // Two `UIViewController` identifiables indicate the same RUM View only if their references are equal.
             return self === otherViewController
@@ -47,7 +47,7 @@ extension UIViewController: RUMViewIdentifiable {
 
 /// Extends `String` with the ability to identify the RUM View.
 extension String: RUMViewIdentifiable {
-    func equals(_ otherIdentifiable: RUMViewIdentifiable?) -> Bool {
+    fileprivate func equals(_ otherIdentifiable: RUMViewIdentifiable?) -> Bool {
         if let otherString = otherIdentifiable as? String {
             // Two `String` identifiables indicate the same RUM View only if their values are equal.
             return self == otherString
@@ -85,23 +85,27 @@ internal struct RUMViewIdentity {
     }
 
     /// Returns `true` if a given identifiable indicates the same RUM View as the identifiable managed internally.
-    func equals(_ identifiable: RUMViewIdentifiable?) -> Bool {
-        if let selfObject = object as? RUMViewIdentifiable {
-            return selfObject.equals(identifiable)
-        } else if let selfValue = value as? RUMViewIdentifiable {
-            return selfValue.equals(identifiable)
-        } else {
-            return false
-        }
+    fileprivate func equals(_ identifiable: RUMViewIdentifiable?) -> Bool {
+        return self.identifiable?.equals(identifiable) ?? false
     }
 
     /// Returns `true` if a given identity indicates the same RUM View as the identifiable managed internally.
     func equals(_ identity: RUMViewIdentity?) -> Bool {
         return equals(identity?.identifiable)
     }
 
+    /// Return default path name for the managed identifiable.
+    var defaultViewPath: String {
+        return identifiable?.defaultViewPath ?? ""
+    }
+
+    /// Returns `true` if the managed identifiable is still available.
+    var isIdentifiable: Bool {
+        return identifiable != nil
+    }
+
     /// Returns the managed identifiable.
-    var identifiable: RUMViewIdentifiable? {
+    private var identifiable: RUMViewIdentifiable? {
         return (object as? RUMViewIdentifiable) ?? (value as? RUMViewIdentifiable)
     }
 }