RUM-3531 WebView slot cache #1760
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 2839 Passed, 0 Skipped, 12m 30.32s Wall Time 🔻 Code Coverage Decreases vs Default Branch (2) |
maxep
force-pushed
the
maxep/RUM-3531/hide-webview-wireframe
branch
2 times, most recently
from
9eb8ddd to
7fca08d
Compare
maxep
force-pushed
the
maxep/RUM-3531/hide-webview-wireframe
branch
from
54cf77c to
6eff630
Compare
| @@ -165,10 +165,7 @@ class WebViewEventReceiverTests: XCTestCase { | |||
|
|
|||
| func testGivenRUMContextAvailable_whenReceivingWebEvent_itGetsEnrichedWithOtherMobileContextAndWritten() throws { | |||
| let core = PassthroughCoreMock( | |||
| context: .mockWith( | |||
| source: "react-native", | |||
| serverTimeOffset: .mockRandom(min: -10, max: 10).rounded() | |||
There was a problem hiding this comment.
any reason for dropping the fuzzy offset?
There was a problem hiding this comment.
I have noticed it to be flaky..
DatadogSessionReplay/Sources/Processor/Builders/WireframesBuilder.swift
Outdated
Show resolved
Hide resolved
...ces/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/WKWebViewRecorder.swift
Outdated
Show resolved
Hide resolved
| let node = Node(viewAttributes: attributes, wireframesBuilder: builder) | ||
| return SpecificElement(subtreeStrategy: .ignore, nodes: [node]) | ||
| } | ||
| } | ||
|
|
||
| /// The slot recorded for a `WKWebView`. | ||
| internal struct WKWebViewSlot: WebViewSlot { |
There was a problem hiding this comment.
I think it can be potentially solved with NSHashTable:
https://developer.apple.com/documentation/foundation/nshashtable
NSHashTable<WKWebview>(options: .weakMemory)
Should ease the implementation as it's relying on NSObject's hash internally if I'm not mistaken.
There's also NSMapTable if we want to control keys.
There was a problem hiding this comment.
That's a good call! It indeed simplifies things, thanks for the suggestion.
| } | ||
|
|
||
| // This test should pass but it doesn't because `WKWebView` apparently leaks. |
There was a problem hiding this comment.
Can we confirm the behaviour in the real usage?
There was a problem hiding this comment.
The commented code was showcasing the leak, webkit is probably holding the reference. Now we rely on NSHashTable which states:
The major option is to provide for "weak" references that are removed automatically, but at some indefinite point in the future.
Which is completely acceptable for our use case.
maxep
force-pushed
the
maxep/RUM-3531/hide-webview-wireframe
branch
from
238684f to
62185d6
Compare
What and why?
The player requires that we keep sending webview wireframes throughout the view instance lifecycle.
A
WKWebViewinstance can exist in multiple RUM views, e.g in case of navigation stack. If the webview becomes invisible, we will send a 'remove' mutation resulting in a drop of the webview slot in the player. When the player drops a slot, it also drops the fullsnapshot of the webpage. If the webview ever come back to a visible state, the player won't be able to recover the previous state since it has lost the fullsnapshot.More details in this RFC.
How?
The easiest solution for the SDKs and the player is to keep sending webview wireframe updates until the webview is deallocated, this way the player will be able to keep the slot's snapshots in memory and to recover the content of the web page if the webview is back to a visible state.
To achieve this, the
ViewTreeSnapshotBuilderwill keep all webview slots in cache between snapshots. TheSnapshotProcessorwill use that cache to keep including hidden webviews to the list of wireframes.The visible webview wireframes are treated as regular wireframes and placed at the right index of the tree.
Review checklist
Custom CI job configuration (optional)
tools/