Skip to content

fix(reminders): catch BadParcelableException in NotificationService#20793

Merged
lukstbit merged 1 commit intoankidroid:mainfrom
ericli3690:ericli3690-bad-parcelable
Apr 20, 2026
Merged

fix(reminders): catch BadParcelableException in NotificationService#20793
lukstbit merged 1 commit intoankidroid:mainfrom
ericli3690:ericli3690-bad-parcelable

Conversation

@ericli3690
Copy link
Copy Markdown
Member

Purpose / Description

If an old intent for a review reminder notification somehow sticks around after a schema migration update, the ReviewReminder stored in the intent will become out of date and cause a BadParcelableException.

Fixes

Approach

While we could implement custom logic to migrate that reminder, that would duplicate the logic for schema migrations currently present in ReviewRemindersDatabase. It's simpler to just fallback on that already-existing logic by triggering a total review reminder schedule and returning early. Hence, this code change.

How Has This Been Tested?

I cannot reproduce this bug personally, as whenever I trigger an update to the schema, the entire app uninstalls and re-installs, which throws out the old intents. I assume the original cause of the bug was some transient error. Nonetheless, this defensive try-catch should be a good addition to the codebase here.

Learning (optional, can help others)

Pertinent code in the AOSP

Quoting from my initial response to this issue:

I can think of two ways to solve this, both of which should work:

  1. Try-catch, and when a BadParcelableException is thrown, simply trigger a AlarmManagerService.scheduleAllNotifications and return. Once refactor(reminders): remove race conditions, simplify database interface #20755 is merged, what scheduleAllNotifications does is fire any notifications that should have recently been fired and then schedule any future ones that need to happen. This is the simple solution.
  2. Create a proper getParcelableReminder helper that successively attempts to de-parcelize using every schema from newest to oldest, and which only throws if all old schemas fail. Once a schema successfully parses the reminder, use .migrate() to bring it up to date. This is the most correct solution.

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

If an old intent for a review reminder notification somehow sticks around after a schema migration update, the ReviewReminder stored in the intent will become out of date. While we could implement custom logic to migrate that reminder, that would duplicate the logic for schema migrations currently present in ReviewRemindersDatabase. It's simpler to just fallback on that already-existing logic by triggering a total review reminder schedule and returning early. Hence, this code change.
@ericli3690 ericli3690 self-assigned this Apr 20, 2026
Copy link
Copy Markdown
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

LGTM, cheers!

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Review labels Apr 20, 2026
@lukstbit lukstbit added this pull request to the merge queue Apr 20, 2026
Merged via the queue into ankidroid:main with commit a4b1b55 Apr 20, 2026
19 checks passed
@github-actions github-actions Bot added this to the 2.24 release milestone Apr 20, 2026
@github-actions github-actions Bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 20, 2026
Copy link
Copy Markdown
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I have been through all of these things with Notifee (react-native notification library) before - it's like seeing old friends 😆

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.

NotificationService: Crash when reading the bytes of an old alarm after a schema change

4 participants