Prevent TransactionTooLargeException crash when backgrounding after deep navigation#863
Open
herrerad85 wants to merge 2 commits into
Open
Prevent TransactionTooLargeException crash when backgrounding after deep navigation#863herrerad85 wants to merge 2 commits into
herrerad85 wants to merge 2 commits into
Conversation
…ackground The reporter's crash log (TransactionTooLargeException, data parcel size 618244 bytes, thrown at ActivityClient.activityStopped) shows the app dies when sent to the background: deep navigation accumulates fragment + back-stack state that, once persisted over the Binder transaction on stop, exceeds the ~1MB buffer limit. This is a different crash from the null-args resume NPE fixed earlier, and is not an OOM. MainActivity.onSaveInstanceState now measures the saved bundle and, if it exceeds 400KB (well under the 618KB that crashed), drops the restorable fragment state so backgrounding can never throw, worst case the app reopens at the start destination instead of crashing. Verified on the emulator: with a lowered test threshold, backgrounding logged "Saved instance state is 44244 bytes ... clearing it" and the app backgrounded and foregrounded with no crash and no TransactionTooLargeException.
Album/artist detail pages were handed the full Parcelable domain objects as navigation arguments, which the Navigation component persists in the back-stack saved state. AlbumID3 carries several nested lists (genres, recordLabels, artists, release types, moods, disc titles); a list of them (ALBUMS_OBJECT) reached ~34KB in the crash log, and ArtistWithAlbumsID3 carried its full album/appearsOn lists (~9KB). Add AlbumID3/ArtistID3.strippedForNav() that copy only the lightweight fields, and pass those at the nav sites: the 10 album-detail adapter taps, the artist "most streamed" link, and the artist albums list. Safe because every detail page re-fetches the full object via its repository (AlbumPageViewModel.setAlbum / ArtistPageViewModel re-observe getAlbum/getArtist by id), so nothing visible is lost. This reduces how often the saved-state cap has to drop state. Also keep the small discTitles list on the stripped nav album so multi-disc headers survive offline (folds in the former disc-titles follow-up).
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/cappielloantonio/tempo/subsonic/models/ArtistID3.kt (1)
18-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider named arguments for consistency with
AlbumID3.strippedForNav().
AlbumID3.strippedForNav()uses named arguments while this method uses positional ones. If theArtistID3constructor parameter order ever changes, the positional call could silently assign wrong values. Minor, but aligning both methods improves safety and consistency.♻️ Optional refactor: use named arguments
- fun strippedForNav(): ArtistID3 = ArtistID3(id, name, coverArtId, albumCount, starred) + fun strippedForNav(): ArtistID3 = ArtistID3( + id = id, + name = name, + coverArtId = coverArtId, + albumCount = albumCount, + starred = starred, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/subsonic/models/ArtistID3.kt` around lines 18 - 27, Update ArtistID3.strippedForNav() to construct ArtistID3 using named arguments, matching AlbumID3.strippedForNav() and preserving the current field mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/cappielloantonio/tempo/ui/activity/MainActivity.java`:
- Around line 87-113: Replace the hardcoded "MainActivity" log tag in
onSaveInstanceState with the existing class-level TAG constant, preserving the
warning message and behavior.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/ArtistPageFragment.java`:
- Around line 377-388: Update navigation argument bundling in
AssetLinkNavigator, ArtistHorizontalAdapter, ArtistAdapter,
ArtistBottomSheetDialog, and AlbumPageFragment to pass strippedForNav() copies
of AlbumID3 and ArtistID3 objects instead of full instances. Preserve the
existing navigation destinations and argument keys while ensuring every bundled
album/artist object uses its corresponding stripped representation.
---
Nitpick comments:
In `@app/src/main/java/com/cappielloantonio/tempo/subsonic/models/ArtistID3.kt`:
- Around line 18-27: Update ArtistID3.strippedForNav() to construct ArtistID3
using named arguments, matching AlbumID3.strippedForNav() and preserving the
current field mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c533c03-5407-4ce6-9ef5-eb42528cbf16
📒 Files selected for processing (9)
app/src/main/java/com/cappielloantonio/tempo/subsonic/models/AlbumID3.ktapp/src/main/java/com/cappielloantonio/tempo/subsonic/models/ArtistID3.ktapp/src/main/java/com/cappielloantonio/tempo/ui/activity/MainActivity.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/AlbumAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/AlbumArtistPageOrSimilarAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/AlbumCarouselAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/AlbumCatalogueAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/AlbumHorizontalAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/ArtistPageFragment.java
Comment on lines
+87
to
+113
| // #688: Deep navigation accumulates fragment + back-stack state in the saved | ||
| // instance Bundle. When the activity is stopped (app sent to background) the | ||
| // platform persists it over a Binder transaction that throws | ||
| // TransactionTooLargeException once it passes the buffer limit (~1MB), crashing | ||
| // the app on background. Cap it: if the saved state is dangerously large, drop the | ||
| // restorable fragment state so backgrounding can never crash — worst case the app | ||
| // reopens at the start destination instead of dying. | ||
| private static final int MAX_SAVED_STATE_BYTES = 400 * 1024; | ||
|
|
||
| @Override | ||
| protected void onSaveInstanceState(@NonNull Bundle outState) { | ||
| super.onSaveInstanceState(outState); | ||
|
|
||
| Parcel parcel = Parcel.obtain(); | ||
| try { | ||
| parcel.writeBundle(outState); | ||
| int size = parcel.dataSize(); | ||
| if (size > MAX_SAVED_STATE_BYTES) { | ||
| Log.w("MainActivity", "Saved instance state is " + size + " bytes (limit " | ||
| + MAX_SAVED_STATE_BYTES + "); clearing it to avoid TransactionTooLargeException on background (#688)"); | ||
| outState.clear(); | ||
| } | ||
| } finally { | ||
| parcel.recycle(); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the class TAG constant instead of a hardcoded string.
Line 105 uses "MainActivity" as the log tag, but the class already defines TAG = "MainActivityLogs" at line 61. Use TAG for consistency with the rest of the class's logging.
🔧 Proposed fix
- Log.w("MainActivity", "Saved instance state is " + size + " bytes (limit "
+ Log.w(TAG, "Saved instance state is " + size + " bytes (limit "
+ MAX_SAVED_STATE_BYTES + "); clearing it to avoid TransactionTooLargeException on background (`#688`)");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #688: Deep navigation accumulates fragment + back-stack state in the saved | |
| // instance Bundle. When the activity is stopped (app sent to background) the | |
| // platform persists it over a Binder transaction that throws | |
| // TransactionTooLargeException once it passes the buffer limit (~1MB), crashing | |
| // the app on background. Cap it: if the saved state is dangerously large, drop the | |
| // restorable fragment state so backgrounding can never crash — worst case the app | |
| // reopens at the start destination instead of dying. | |
| private static final int MAX_SAVED_STATE_BYTES = 400 * 1024; | |
| @Override | |
| protected void onSaveInstanceState(@NonNull Bundle outState) { | |
| super.onSaveInstanceState(outState); | |
| Parcel parcel = Parcel.obtain(); | |
| try { | |
| parcel.writeBundle(outState); | |
| int size = parcel.dataSize(); | |
| if (size > MAX_SAVED_STATE_BYTES) { | |
| Log.w("MainActivity", "Saved instance state is " + size + " bytes (limit " | |
| + MAX_SAVED_STATE_BYTES + "); clearing it to avoid TransactionTooLargeException on background (#688)"); | |
| outState.clear(); | |
| } | |
| } finally { | |
| parcel.recycle(); | |
| } | |
| } | |
| // `#688`: Deep navigation accumulates fragment + back-stack state in the saved | |
| // instance Bundle. When the activity is stopped (app sent to background) the | |
| // platform persists it over a Binder transaction that throws | |
| // TransactionTooLargeException once it passes the buffer limit (~1MB), crashing | |
| // the app on background. Cap it: if the saved state is dangerously large, drop the | |
| // restorable fragment state so backgrounding can never crash — worst case the app | |
| // reopens at the start destination instead of dying. | |
| private static final int MAX_SAVED_STATE_BYTES = 400 * 1024; | |
| `@Override` | |
| protected void onSaveInstanceState(`@NonNull` Bundle outState) { | |
| super.onSaveInstanceState(outState); | |
| Parcel parcel = Parcel.obtain(); | |
| try { | |
| parcel.writeBundle(outState); | |
| int size = parcel.dataSize(); | |
| if (size > MAX_SAVED_STATE_BYTES) { | |
| Log.w(TAG, "Saved instance state is " + size + " bytes (limit " | |
| MAX_SAVED_STATE_BYTES + "); clearing it to avoid TransactionTooLargeException on background (`#688`)"); | |
| outState.clear(); | |
| } | |
| } finally { | |
| parcel.recycle(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/cappielloantonio/tempo/ui/activity/MainActivity.java`
around lines 87 - 113, Replace the hardcoded "MainActivity" log tag in
onSaveInstanceState with the existing class-level TAG constant, preserving the
warning message and behavior.
Comment on lines
377
to
388
| private void navigateToAlbumList(String title, List<com.cappielloantonio.tempo.subsonic.models.AlbumID3> albums) { | ||
| Bundle bundle = new Bundle(); | ||
| bundle.putString(Constants.ALBUM_LIST_TITLE, title); | ||
| bundle.putParcelableArrayList(Constants.ALBUMS_OBJECT, new ArrayList<>(albums)); | ||
| // #688: strip each album's heavy nested lists so this whole list (the single | ||
| // biggest nav argument, ~34KB) stays small in the saved-state Bundle. | ||
| ArrayList<com.cappielloantonio.tempo.subsonic.models.AlbumID3> albumsForNav = new ArrayList<>(albums.size()); | ||
| for (com.cappielloantonio.tempo.subsonic.models.AlbumID3 albumForNav : albums) { | ||
| albumsForNav.add(albumForNav.strippedForNav()); | ||
| } | ||
| bundle.putParcelableArrayList(Constants.ALBUMS_OBJECT, albumsForNav); | ||
| Navigation.findNavController(requireView()).navigate(R.id.albumListPageFragment, bundle); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all putParcelable calls with ALBUM_OBJECT or ARTIST_OBJECT that do NOT use strippedForNav
rg -n 'putParcelable.*ALBUM_OBJECT|putParcelable.*ARTIST_OBJECT' --type java -A2 | grep -v 'strippedForNav' || echo "All occurrences use strippedForNav"Repository: eddyizm/tempus
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map where album/artist navigation payloads are defined and used.
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'ALBUM_OBJECT|ARTIST_OBJECT|strippedForNav\(|putParcelableArrayList\(|putParcelable\(' \
app/src/main/java
# Show the files that contain the relevant constants and models.
fd -a 'Constants.java|AlbumID3.java|Artist.java|ArtistID3.java' app/src/main/javaRepository: eddyizm/tempus
Length of output: 17713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the nav-stripping helpers and the main navigation entry points that still pass raw artist/album objects.
sed -n '1,140p' app/src/main/java/com/cappielloantonio/tempo/subsonic/models/AlbumID3.kt
printf '\n---\n'
sed -n '1,120p' app/src/main/java/com/cappielloantonio/tempo/subsonic/models/ArtistID3.kt
printf '\n---\n'
sed -n '1,240p' app/src/main/java/com/cappielloantonio/tempo/util/AssetLinkNavigator.java
printf '\n---\n'
sed -n '1,220p' app/src/main/java/com/cappielloantonio/tempo/ui/adapter/ArtistHorizontalAdapter.java
printf '\n---\n'
sed -n '1,190p' app/src/main/java/com/cappielloantonio/tempo/ui/adapter/ArtistAdapter.java
printf '\n---\n'
sed -n '1,210p' app/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/ArtistBottomSheetDialog.javaRepository: eddyizm/tempus
Length of output: 26698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the files that accept artist/album nav args to see whether the raw payload is actually consumed immediately or forwarded deeper.
sed -n '1,220p' app/src/main/java/com/cappielloantonio/tempo/ui/fragment/AlbumPageFragment.java
printf '\n---\n'
sed -n '1,220p' app/src/main/java/com/cappielloantonio/tempo/ui/fragment/ArtistPageFragment.java
printf '\n---\n'
sed -n '1,180p' app/src/main/java/com/cappielloantonio/tempo/ui/fragment/AlbumListPageFragment.java
printf '\n---\n'
sed -n '1,220p' app/src/main/java/com/cappielloantonio/tempo/ui/fragment/SongListPageFragment.javaRepository: eddyizm/tempus
Length of output: 36915
Strip the remaining album/artist nav args
Most album adapters already use strippedForNav(), but AssetLinkNavigator, ArtistHorizontalAdapter, ArtistAdapter, ArtistBottomSheetDialog, and AlbumPageFragment still bundle full AlbumID3/ArtistID3 objects. Use the stripped copies there too to keep saved-state Bundles under the Binder limit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/ArtistPageFragment.java`
around lines 377 - 388, Update navigation argument bundling in
AssetLinkNavigator, ArtistHorizontalAdapter, ArtistAdapter,
ArtistBottomSheetDialog, and AlbumPageFragment to pass strippedForNav() copies
of AlbumID3 and ArtistID3 objects instead of full instances. Preserve the
existing navigation destinations and argument keys while ensuring every bundled
album/artist object uses its corresponding stripped representation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #688 (a
TransactionTooLargeExceptionon background, not an OOM).Problem
When the app is sent to the background, Android persists the fragment back stack into the saved-instance-state Bundle over a Binder transaction. After browsing several album/artist pages that Bundle crosses the ~1MB Binder limit, and
activityStoppedthrowsTransactionTooLargeException. The reporter's log showed ~618KB, mostly the NavHost back stack plus heavy Parcelable nav args (ALBUMS_OBJECT~34KB,ARTIST_OBJECT~9KB). That fits why it only appears after backgrounding from deep navigation and is hard to reproduce on purpose.Fix
MainActivity.onSaveInstanceStatemeasures the saved Bundle and, if it exceeds 400KB, drops the restorable fragment state so backgrounding can never throw. Worst case the app reopens at the start destination instead of crashing.strippedForNav(), since every detail page re-fetches the full object by id (AlbumPageViewModel.setAlbum/ArtistPageViewModelre-observe by id). Nothing visible is lost, and it keeps the Bundle small enough that the cap rarely fires.Verification
TransactionTooLargeException.development.Summary by CodeRabbit