Skip to content

feat: [SDK-4978] add iOS KMP crash capture and upload - #1713

Open
fadi-george wants to merge 7 commits into
mainfrom
fadi/sdk-4978
Open

feat: [SDK-4978] add iOS KMP crash capture and upload#1713
fadi-george wants to merge 7 commits into
mainfrom
fadi/sdk-4978

Conversation

@fadi-george

@fadi-george fadi-george commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Description

One Line Summary

Adds iOS crash persistence and next-launch upload through the shared KMP logger pipeline.

Details

Motivation

Capture OneSignal-related uncaught Objective-C exceptions synchronously before process termination, then upload retained crash records through /sdk/log on the next enabled SDK initialization.

Scope

  • Composes the KMP crash reporter and uploader in the iOS remote-logging lifecycle.
  • Preserves existing host exception handlers and avoids POSIX signal interception because the Swift/Kotlin persistence path is not async-signal-safe.
  • Serializes uploader runs and prevents requests after remote logging shuts down.
  • Keeps all changes internal with no public API changes.

Crash attribution

  • Resolves exception return addresses to Mach-O images and symbols using dladdr.
  • Treats a crash as OneSignal-related when any frame has an exact known OneSignal image or a constrained OneSignal symbol identity, matching Android's coverage-oriented attribution policy.
  • Supports static linkage when symbols retain Objective-C OneSignal*, known Swift module, Kotlin com.onesignal, or onesignal_ C identities.
  • This favors diagnostic coverage over strict blame attribution. A host crash with a deeper OneSignal callback frame may also be retained.

Limitations and follow-ups

  • Attribution is best effort for statically linked builds. Stripped symbols and generic compiler-generated frames may no longer identify the originating SDK module.
  • Swift and Kotlin symbol mangling can change across compiler versions, so optimized or unfamiliar mangled names may not match the constrained fallback.
  • Fully stripped static frames that resolve only to the host executable cannot be attributed safely.
  • Capture currently covers uncaught Objective-C exceptions only. Native traps, POSIX signals, non-fatal errors, and ANRs remain out of scope.
  • The exception handler is active only while remote logging is enabled. Crashes before remote configuration enables logging are not retained.
  • Notification Service Extensions run in a separate process and require separate crash-handler composition.

Testing

Unit testing

  • Added coverage for synchronous persistence, dynamic and static OneSignal attribution, issue [Bug]: OneSignalCore NSKeyedArchiver Crash #1366-style Foundation stacks, host callback stacks, stripped/unresolved frames, host-handler preservation, exception-handler chaining and reentrancy, uploader serialization, disabled transport, and winning logger activation.
  • Ran OSLogCrashHandlerTests and OSLogCrashAttributionTests: 23 tests passed.
  • Ran OSLoggerAdaptersTests and OSRemoteLoggingControllerTests: 28 tests passed.
  • Verified release iOS Simulator and Mac Catalyst builds.

Manual testing

Not run on a physical device. End-to-end validation requires enabling the remote logging feature flag, forcing an uncaught Objective-C exception, relaunching, and confirming the /sdk/log upload and crash-file deletion.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

@fadi-george
fadi-george requested a review from nan-li August 12, 2026 20:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-model adversarial review (interrogate)

PR: feat: [SDK-4978] add iOS KMP crash capture and upload
Models: claude-fable-5-thinking-xhigh · gpt-5.6-sol-xhigh · cursor-grok-4.5-high-fast · claude-opus-5-thinking-high
Base...Head: 110a56a6...30e95514

Intent

Compose the KMP crash reporter/uploader into the iOS remote-logging lifecycle so OneSignal-related uncaught Objective-C exceptions are persisted synchronously before termination, then uploaded via /sdk/log on the next enabled init. Preserve the host exception handler, avoid POSIX signals, serialize uploader runs, and refuse HTTP after shutdown — with no public API changes.

Reviewers

  • A claude-fable-5-thinking-xhigh — 7 findings
  • B gpt-5.6-sol-xhigh — 5 findings
  • C cursor-grok-4.5-high-fast — 6 findings
  • D claude-opus-5-thinking-high — 9 findings

Act On

  1. cancel never releases activeOwner / process-wide wedge (A, C, D)cancel only strips pendingUploads. If the active uploader completion never fires (including after shutdown disables transport), crash upload is stuck for the rest of the process. Also: finish-then-cancel race can start work after telemetry.shutdown().
  2. isOneSignalAtFault is too coarse (A, B, C, D) — whole-stack substring "OneSignal" via localizedCaseInsensitiveContains: false positives for host exceptions inside swizzled/callback stacks; false negatives under static linking / stripped symbols; locale-sensitive matching for a programmatic identifier.
  3. shutdown flips lifecycle inactive before telemetry.shutdown() (B, D) — final export/shutdown sends are refused as retryable transport failure (statusCode: -1). In-flight send can also race past the isEnabled check.
  4. Fatal-path logging via OneSignalLog (C)capture catch (and reporter -> IOSLogger) can walk listeners and present alert UI when _alertLogLevel allows — unsafe on the uncaught-exception path.
  5. saveCrash success is discarded (C, D)_ = try ...; FileLogStore.save converts I/O failures to false and does not throw, so persist failures can be silent.

Consider

  • 5s minFileAgeForReadMillis + once-per-start uploader (B) — fast relaunch can skip finalized .otlp until another init.
  • Public OSRemoteLoggerProtocol.start() (A, B, D) — contradicts no public API changes; two-phase init silently no-ops HTTP until start().
  • initialize() silent no-op when Self.active != nil, no re-arm (A, D).
  • Unregister mid-flight drops persistence (C).
  • start() holds lifecycleOperationLock across uploader kickoff (C).
  • Crash dir not partitioned by app id (B).
  • exception.reason ?? exception.description may embed userInfo (D).

Noted

  • currentThreadName duplicated vs OSLoggerPlatformProvider (A, D)
  • Hand-authored pbxproj IDs (D)
  • Coordinator may be heavier than a process-level drain latch (D)
  • Composition path largely untested end-to-end (A)

Dismissed

  • Pure structure preference without a concrete failure mode beyond the coordinator latch note.
  • Mac Catalyst gaps already gated by #if !targetEnvironment(macCatalyst).

Agreement map

Strongest consensus: fault attribution (4/4) and uploader lifecycle/cancel vs active slot (3/4). Transport/shutdown ordering and silent persist failure are next. Unit tests cover pieces well; they miss shared-coordinator + shutdown races and real release-build stacks.

Inline comments mark Act On anchors. No code changes from this automation.

Open in Web View Automation 

Sent by Cursor Automation: Untitled

Comment thread iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/OSLogCrashHandler.swift Outdated

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Fadi — composition looks right (sync persist on the crashing thread, KMP encode/upload, host handler chaining, no POSIX signals). I am not restating the Cursor bot Act Ons; those still stand, especially shutdown-vs-transport ordering, cancel/activeOwner, and fault attribution.

Requested change

CI is red on this PR: SwiftLint type_body_length on OSLoggerAdaptersTests (386 / 350). The job fails at lint, before tests run. Please split the new crash-handler tests into OSLogCrashHandlerTests.swift (that also clears the 549-line file_length warning).

Notes the bot did not cover

  • Dismiss the 5s min-age consider. LogCrashUploader.internalStart() already does send → delay(minFileAgeForReadMillis) → send again in the same start(), so a fast relaunch is not skipped until the next init.
  • Persist-failure catch does run. KMP LogTelemetryCrashImpl throws if FileLogStore.save returns false, so the Swift catch is not dead. The remaining issue is the fatal-path OneSignalLog / alert UI the bot already flagged.
  • Stacked on #1703. Crash uploads go through the same private URLSession as remote logs. No need to relitigate consent here, but this is more /sdk/log traffic on that ungated path.

Ticket vs PR body

SDK-4978 done-when mentions non-fatal capture and signal handlers. Worth one line in the description that those are explicitly out of scope (ANR follow-up; signals skipped because the persist path is not async-signal-safe) so it does not read as incomplete ticket work.

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Fadi — the harden commits landed the first-round Act Ons. Two-phase shutdown, the distinct −2 disabled signal, OSCrashLogger on the fatal path, first-throwing-module attribution, and the test split are the right shape.

Still a few items before merge:

  1. start() holds lifecycleOperationLock across the uploader kickoff (same deadlock class as the diagnostic listener test).
  2. cancel(active) starts the next LogCrashUploader while the cancelled job is still in its 5s min-age delay, so two file stores can POST the same crash directory.
  3. OneSignalKMP is missing from the attribution module set.

Inline comments have the suggested fixes. CI red is the same unrelated IAM/User suite as #1703 — OSCore’s 52 tests passed.

Nit: one sentence in the PR body that non-fatal capture and POSIX signals are out of scope (ANR follow-up; signals skipped because persist is not async-signal-safe) so this does not read as incomplete ticket work.

Comment thread iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/OSRemoteLogger.swift Outdated
Base automatically changed from fadi/sdk-4977 to main August 14, 2026 01:11
fadi-george and others added 4 commits August 13, 2026 18:12
Harden shutdown and uploader cancellation races, constrain crash attribution, and keep fatal-path logging isolated from application listeners.

Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid treating host exceptions as OneSignal faults merely because a deeper callback frame belongs to the SDK.

Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid startup reentrancy deadlocks, keep active uploads serialized through completion, and recognize crashes originating in the KMP image.

Co-authored-by: Cursor <cursoragent@cursor.com>

@nan-li nan-li left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things on whether we'll actually see SDK crashes in /sdk/log without waiting for GitHub.

  1. To consider: Attribution is stricter than Android and will drop real OneSignal crashes. We only keep the first frame that isn't CoreFoundation/libobjc; Android persists if any frame is com.onesignal.*. That misses IAM unarchive, NSMallocException while archiving, etc. — first frame is Foundation. Let's match Android: persist if any stack image is in oneSignalModules. We'll pick up some host crashes that just have a OneSignal swizzle on the stack; that's the same tradeoff Android already made.
    Would capture these:
  1. To consider: The handler only exists while remote logging is on, so first session it's after ios_params. Init/migration crashes in that window never hit disk. Can we always install the handler at OneSignal.initialize and keep upload gated on log level?
    Would have captured these:

Not asking to change these here, just so we don't think merge means we'll start seeing them, maybe explicitly call out in PR description

  • This is NSException only. The v5 crashes people have been filing (encode / OSOperationRepo / EXC_BREAKPOINT) are native traps. Follow-up, not Mach ports in this PR.
  • NSE is a different process, so extension crashes never hit this handler. Also a follow-up.

Comment thread iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj Outdated
@fadi-george
fadi-george dismissed abdulraqeeb33’s stale review August 14, 2026 22:42

Addressed your changes. CI has unrelated flaky tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants