Repository navigation
fix: clear the replay screenshot bitmap and skip failed draws - #999
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 42d957f: 1 Consider.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
posthog-ios Compliance ReportDate: 2026-10-30T10:35:17.651791+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 3540ms |
| Format Validation.Event Has Uuid | ✅ | 3177ms |
| Format Validation.Event Has Lib Properties | ✅ | 2813ms |
| Format Validation.Distinct Id Is String | ✅ | 2878ms |
| Format Validation.Token Is Present | ✅ | 2903ms |
| Format Validation.Custom Properties Preserved | ✅ | 2818ms |
| Format Validation.Event Has Timestamp | ✅ | 2838ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 370ms |
| Retry Behavior.Retries On 503 | ✅ | 11856ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 4859ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 4947ms |
| Retry Behavior.Respects Retry After Header | ✅ | 7942ms |
| Retry Behavior.Implements Backoff | ✅ | 21892ms |
| Retry Behavior.Retries On 500 | ✅ | 9364ms |
| Retry Behavior.Retries On 502 | ✅ | 9180ms |
| Retry Behavior.Retries On 504 | ✅ | 9314ms |
| Retry Behavior.Max Retries Respected | ❌ | 21773ms |
| Deduplication.Generates Unique Uuids | ✅ | 2978ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 9201ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 16791ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 7880ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 2962ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 472ms |
| Compression.Sends Gzip When Enabled | ✅ | 2913ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 2788ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 274ms |
| Batch Format.Multiple Events Batched Together | ✅ | 2947ms |
| Error Handling.Does Not Retry On 403 | ✅ | 4787ms |
| Error Handling.Does Not Retry On 413 | ✅ | 4844ms |
| Error Handling.Retries On 408 | ✅ | 9383ms |
Failures
retry_behavior.max_retries_respected
Expected 4 requests, got 6
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 2792ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 2838ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 2871ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 2883ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 2850ms |
| Request Payload.Groups Round Trip | ✅ | 2899ms |
| Request Payload.Groups Default To Empty Object | ✅ | 2899ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 2866ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 2921ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 2922ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 91ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 2830ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 5604ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 2774ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 2985ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 3007ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 3089ms |
dustinbyrne
left a comment
There was a problem hiding this comment.
The bitmap initialization and failed-draw handling look correct, including the distinction between attempted and deferred rendering. No actionable findings.
AI-assisted review.
Conflict resolutions: - UIView+Util toImage: keep the renderer switch and record drawHierarchy's result in both drawHierarchy branches, so #999's failed-draw skip applies to the fidelity and bridge renders. - Replay integration: becoming active wakes the capture backoff (#896), then resumes plugins and prewarms GPU capture; collectScreenshotMetadata keeps the renderer API and skips a tick whose attempted render failed (#999). - Tests: touch tests use the shared ReplayTestSupport helpers with main's new cases, PresentingController, makeWindow and the maskAllTextInputs parameter; the screenshot size test draws only the presentation-tree renderer, since failed drawHierarchy renders are now dropped in the hostless bundle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019xLynxqQ97w5iqqo4Dxyi5
💡 Motivation and Context
PostHogGraphicsImageRenderer(added in #578 to replaceUIGraphicsImageRendereron the screenshot hot path) allocates its bitmap withmallocand never clears it. Core Graphics only writes the pixels a draw covers, so a draw that fails or leaves part of the canvas untouched handsmakeImage()whatever the heap held before, and that ends up in a$snapshot. Sentry's renderer, which ours is derived from, usescalloc; the port dropped that.It reproduces on the simulator: the default upload resolution (one pixel per point, 402×874 on an iPhone 17 Pro) is a 1.4 MB buffer the allocator recycles, and an empty draw into it came back dirty in 30 of 30 runs. The native-scale 12.6 MB buffer came back clean every time because it comes from fresh zero pages.
I don't think anything actually leaked in practice. A normal capture of a visible, opaque window paints every pixel, so there's nothing left over to read. The exposure is limited to draws that fail or leave gaps: a
drawHierarchythat returnsfalse(hidden window, no window scene, nothing committed yet), transparent window regions, or source transparency in the mask and resample redraws inPostHog/Replay/RRWireframe.swift.Two commits, one concern each:
mallocbecomescalloc, so undrawn pixels are transparent instead of leftover memory. No change to what gets masked.UIView.toImageignored theBoolthatdrawHierarchy(in:afterScreenUpdates:)returns, so a failed draw still produced an image andrenderAndEnqueueScreenshotuploaded it (before this, leftover memory; after commit 1 alone, a blank frame). It now returnsnil, which the capture path already treats as "skip this tick". This matches posthog-android, where aPixelCopyfailure leaves the screenshot discarded, and the fail-closed rule in thesession-replay-privacyspec. In the example-app hostdrawHierarchyreturnstrueon a visible window both on and off the main thread, so the background-capture mode is unaffected.Cost of
callocon the simulator, median of 60 renders with no drawing (allocation plusmakeImage()):malloc(today)callocLarge buffers come from fresh zero pages, so
callocis free there; the 1.4 MB case pays one memset of a recycled block.CGContext(data: nil, …)measured faster still (0.025 ms / 0.014 ms) becausemakeImage()can share the buffer instead of copying it, and came back zeroed, but that isn't a documented guarantee, so I left it as a follow-up rather than mixing it into a privacy fix.Worth a follow-up to add an app-hosted test that drives
captureBridgeSnapshotto a successful upload; the hostless runner can't, sincedrawHierarchyhas no window scene there.💚 How did you test it?
PostHogGraphicsImageRendererTest: an empty draw and a partial draw at the default upload size must come back fully transparent outside the drawn area. Both fail onmainin 3 of 3 runs and pass with the fix in 3 of 3 runs (iOS 26.5 simulator).PostHogScreenshotFailedDrawTest:toImageon a windowdrawHierarchycan't render returnsnilthrough bothdrawHierarchypaths, and still returns an image through the presentation-tree renderer.drawHierarchycall that always fails in that runner, so they were asserting on blank frames. They now observe the same behaviour through the presentation-tree renderer, and the touch-capture emission test hands its frame tocaptureSnapshotdirectly, as the fail-closed suite already does.PostHogStorageTestapp-group cases.make lintis clean for the changed files.make buildSdkpasses the iOS, macOS and Mac Catalyst lanes under Xcode 26.2; the tvOS and watchOS lanes need platform components this machine doesn't have, and the change is#if os(iOS)only.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)