Skip to content

Conversation

@jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Dec 10, 2025

Description

One Line Summary

Fixed a rare NPE from PermissionsViewModel.

Details

Motivation

Fixed an NPE introduced in 5.4.0 with the new PermissionsActivity + PermissionsViewModel flow for handling permission prompts. This refactor introduced a rare lifecycle edge case involving process death during a permission prompt.

Edge Case Explain

  1. The app shows the permission prompt.
  2. While the prompt is visible, the app process is killed (e.g., by the system or via adb).
  3. Android later recreates PermissionsActivity to deliver onRequestPermissionsResult.
  4. Because onCreate only called handleBundleParams(...) when savedInstanceState == null, the recreated activity did not re-run initialize(...) on the PermissionsViewModel.
  5. As a result, permissionRequestType remained null, but onRequestPermissionsResult still executed and called
    executeCallback(permissionRequestType!!) → NPE.

Scope

Two changes:

  1. Always initialize the ViewModel in onCreate, removing the savedInstanceState == null guard.
    This is safe because shouldRequestPermission() already prevents duplicate permission requests.
  2. Added a null-safe check for permissionRequestType before resolving the callback inside
    PermissionsViewModel.executeCallback(...).
    This provides defensive hardening if onRequestPermissionsResult is invoked before initialization completes. This also acts as a safety net for any future lifecycle edge cases that might call onRequestPermissionsResult before initialization completes.

Testing

Unit testing

Added a few test cases covering PermissionsViewModel.onRequestPermissionsResult().

Manual testing

Step to reproduce on emulator Pixel 9 API 35:

  1. Fresh install the Android example app.
  2. Request notification permission and observe that the permission prompt shows up.
  3. Run adb shell ps | grep com.onesignal.sdktest to find the app process ID, then run adb shell run-as com.onesignal.sdktest kill -9 xxxxx (replace xxxxx with the PID) to kill the app process.
  4. Allow the permission. Observe that the app now crashes.
image

After the fix, no crash is thrown and the permission is granted correctly after restarting the app. No duplicate request on config change.

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@jinliu9508 jinliu9508 added the WIP Work In Progress label Dec 10, 2025
@jinliu9508 jinliu9508 force-pushed the fix-PermissionsViewModel-crash branch from 7a0d475 to b1374ac Compare December 11, 2025 05:12
@jinliu9508 jinliu9508 changed the title Tests: add test to PermissionViewModelTests to test onRequestPermissi… Fix: a rare NPE from PermissionViewModel Dec 11, 2025
@jinliu9508 jinliu9508 removed the WIP Work In Progress label Dec 11, 2025
@github-actions
Copy link
Contributor

📊 Diff Coverage Report

Diff Coverage Report

Threshold: 80%

Changed Files Coverage

  • PermissionsActivity.kt: 0/71 lines (0.0%)
    • ⚠️ Below threshold: 71 uncovered lines
  • PermissionsViewModel.kt: 71/76 lines (93.4%)

Overall Coverage

71/147 lines covered (48.3%)

❌ Coverage Check Failed

Files below 80% threshold:

  • PermissionsActivity.kt: 0.0% (71 uncovered lines)

📥 View workflow run

@jinliu9508
Copy link
Contributor Author

Pipeline failed due to code coverage failure. Merging as we will not add test for PermissionsActivity for now.

@jinliu9508 jinliu9508 merged commit 8305074 into main Dec 11, 2025
1 check failed
@jinliu9508 jinliu9508 deleted the fix-PermissionsViewModel-crash branch December 11, 2025 21:44
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