fix: prevent crashes from stale intents after process death#546
fix: prevent crashes from stale intents after process death#546jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPluviaMain now clears intermediate routes when navigating to XServer and uses a guarded popBackStack-with-fallback when returning to Home. XServerScreen wraps container retrieval in try/catch, logs errors, and early-exits to navigation if container is null. SettingsGroupDebug adds readTail (256 KB cap) and uses it for conditional crash/wine log dialogs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Pluvia as PluviaMain (NavHost)
participant Nav as NavController
participant XScreen as XServerScreen
participant Container as ContainerUtils
User->>Pluvia: Tap "Launch XServer"
Pluvia->>Nav: navigate(XServer) with popUpTo(clear intermediate)
Nav->>XScreen: show XServer route
XScreen->>Container: getContainer(context, appId)
alt container retrieved
XScreen->>XScreen: proceed with startup
else exception or null
XScreen->>Pluvia: trigger back-navigation
Pluvia->>Nav: navigate(Home) with clearBackStack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
These change appear to improve/fix random crashing I was getting when (re-)starting the app via Intents (e.g. from ES-DE), or when re-opening my clamshell device after it had been shut (during which time Android apparently killed off the process). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1003-1006: When checking currentRoute ==
PluviaScreen.XServer.route and attempting to pop back to Home, capture the
boolean result of navController.popBackStack(PluviaScreen.Home.route, inclusive
= false); if it returns false (Home not in back stack) then explicitly navigate
to Home (e.g., navController.navigate(PluviaScreen.Home.route) with appropriate
options) as a fallback so the UI does not remain on XServer. Update the block
around currentRoute/PluviaScreen.XServer.route to perform the pop, test the
result, and call navController.navigate to PluviaScreen.Home.route when
popBackStack fails.
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 275-295: The readTail function risks OOM because
InputStream.skip() can skip fewer bytes than requested; replace the stream-based
tail logic in readTail with a RandomAccessFile-based approach: open the file
with RandomAccessFile, call seek(max(0, file.length() - MAX_LOG_DISPLAY_BYTES)),
read the remaining bytes via a buffered reader, drop the partial first line as
currently done, and prepend the "... (sizeKB, showing last XKB) ..." header;
keep the small-file path (file.readText()) and the exception handling intact and
reference MAX_LOG_DISPLAY_BYTES and readTail when making the change.
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
jeremybernstein
force-pushed
the
jb/intents_crash_pr
branch
from
cff5ae7 to
b0e3ca4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt (1)
281-293: Cachefile.length()to avoid four separatestatsyscalls and a subtle OOM edge case
file.length()is called four times (lines 281, 285, 286, 292). Because the file is actively written to (it's a live log), each call may return a different value. In the worst case, if the file grows significantly between lines 285 and 286,ByteArray(...)can allocate more thanMAX_LOG_DISPLAY_BYTES. SinceOutOfMemoryErroris anError(not anException), the catch block on line 295 won't intercept it, defeating the OOM guard.♻️ Proposed refactor — capture length once
} else { - java.io.RandomAccessFile(file, "r").use { raf -> - raf.seek(maxOf(0L, file.length() - MAX_LOG_DISPLAY_BYTES)) - val bytes = ByteArray((file.length() - raf.filePointer).toInt()) - raf.readFully(bytes) - val text = bytes.toString(Charsets.UTF_8) - // drop partial first line - val idx = text.indexOf('\n') - val trimmed = if (idx >= 0) text.substring(idx + 1) else text - "... (${file.length() / 1024}KB, showing last ${MAX_LOG_DISPLAY_BYTES / 1024}KB) ...\n$trimmed" - } + val length = file.length() + val seekPos = maxOf(0L, length - MAX_LOG_DISPLAY_BYTES) + java.io.RandomAccessFile(file, "r").use { raf -> + raf.seek(seekPos) + val bytes = ByteArray((length - seekPos).toInt()) + raf.readFully(bytes) + val text = bytes.toString(Charsets.UTF_8) + // drop partial first line + val idx = text.indexOf('\n') + val trimmed = if (idx >= 0) text.substring(idx + 1) else text + "... (${length / 1024}KB, showing last ${MAX_LOG_DISPLAY_BYTES / 1024}KB) ...\n$trimmed" + } }Also update line 281 to reuse the cached value:
- if (file.length() <= MAX_LOG_DISPLAY_BYTES) { + val length = file.length() + if (length <= MAX_LOG_DISPLAY_BYTES) { file.readText() } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt` around lines 281 - 293, Cache the file length once into a local val (e.g. val len = file.length()) and use that cached len everywhere instead of calling file.length() multiple times; specifically, in SettingsGroupDebug.kt replace the repeated file.length() calls with the cached len, compute start = maxOf(0L, len - MAX_LOG_DISPLAY_BYTES) before raf.seek(start), and compute the byte array size as val size = (len - start).toInt() so ByteArray(size) cannot grow beyond the intended window and avoids the OOM/race between syscalls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 281-293: Cache the file length once into a local val (e.g. val len
= file.length()) and use that cached len everywhere instead of calling
file.length() multiple times; specifically, in SettingsGroupDebug.kt replace the
repeated file.length() calls with the cached len, compute start = maxOf(0L, len
- MAX_LOG_DISPLAY_BYTES) before raf.seek(start), and compute the byte array size
as val size = (len - start).toInt() so ByteArray(size) cannot grow beyond the
intended window and avoids the OOM/race between syscalls.
b0e3ca4 to
9a569a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 117-125: The call to readTail(latestCrashFile) is performing
synchronous I/O on the main/composition thread and is re-run on every
recomposition; change the code to compute the file contents off the main thread
and cache the result keyed by latestCrashFile (e.g., use produceState or
remember with a coroutine/dispatcher) and then pass that cached/async-loaded
string into CrashLogDialog (also used for the Wine log block), keeping existing
handlers like onSave/saveResultContract and onDismissRequest unchanged.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 275-300: The readTail function correctly replaces
InputStream.skip() with RandomAccessFile(file, "r") using seek() and
readFully(); leave the implementation as-is but ensure MAX_LOG_DISPLAY_BYTES
remains <= Int.MAX_VALUE so the ByteArray((len - start).toInt()) cast is safe,
retain the try/catch around readTail to handle EOFException or other IO errors,
and keep the logic in readTail (including the partial-line drop and the summary
prefix) in the RandomAccessFile use within readTail.
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt
Show resolved
Hide resolved
jeremybernstein
force-pushed
the
jb/intents_crash_pr
branch
from
9a569a8 to
7ef4328
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt (1)
283-310: LGTM — pastInputStream.skip()OOM risk fully addressed.
RandomAccessFile.seek()+readFully()gives deterministic tail reads;readFullythrowsEOFException(caught by the outercatch) if the file is truncated mid-read, so there's no silent data loss. Thecheck()on line 288 being outside thetry/catchis intentional and correct: a developer accidentally raisingMAX_LOG_DISPLAY_BYTESpastInt.MAX_VALUEshould surface as a hard crash, not silently degrade to a "Failed to read file" message.One optional consistency nit:
file.readText()on line 293 (small-file path) already defaults toCharsets.UTF_8in Kotlin's stdlib, so there is no actual charset inconsistency withbytes.toString(Charsets.UTF_8)on line 300 — but making the encoding explicit (file.readText(Charsets.UTF_8)) would self-document the intent.📝 Explicit charset for self-documentation
- file.readText() + file.readText(Charsets.UTF_8)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt` around lines 283 - 310, Update readTail to explicitly use UTF-8 for the small-file path: when the fast path uses file.readText(), call file.readText(Charsets.UTF_8) so its charset matches the RandomAccessFile branch (which uses Charsets.UTF_8); keep the existing check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) and exception handling unchanged (symbols: readTail, MAX_LOG_DISPLAY_BYTES, file.readText).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 283-310: Update readTail to explicitly use UTF-8 for the
small-file path: when the fast path uses file.readText(), call
file.readText(Charsets.UTF_8) so its charset matches the RandomAccessFile branch
(which uses Charsets.UTF_8); keep the existing check(MAX_LOG_DISPLAY_BYTES <=
Int.MAX_VALUE) and exception handling unchanged (symbols: readTail,
MAX_LOG_DISPLAY_BYTES, file.readText).
https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.io/read-text.html But ok, to make the intent 100% clear, I'll do it. For you, CR. |
jeremybernstein
force-pushed
the
jb/intents_crash_pr
branch
from
7ef4328 to
025008c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 287-310: readTail currently returns null when file is null or
doesn't exist which causes callers (crashText/wineText) to display "Loading..."
forever; change readTail so that when file == null or !file.exists() it returns
a distinct informative string (e.g., "File not found" possibly including
file?.name) instead of null, and in the catch block return "Failed to read file:
${e.message ?: e.javaClass.simpleName}" so the error message never becomes
"null"; update references to readTail (used for latestCrashFile handling)
accordingly so callers can distinguish missing files from a real loading state.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 156-167: The Crash log dialog can stay stuck showing "Loading..."
because produceState's value can become null if readTail(latestWineLogFile)
returns null; update the produceState block used with
showWineLogDialog/latestWineLogFile so that the state never resolves to null
(e.g., use a non-null initialValue and assign value =
withContext(Dispatchers.IO) { readTail(latestWineLogFile) } ?: "File not found"
or an appropriate fallback), and ensure the CrashLogDialog call uses that
non-null fallback (fileText should not be the literal "Loading..." once the IO
read completes), keeping references: produceState, readTail, latestWineLogFile,
showWineLogDialog, CrashLogDialog, and saveWineLogContract.
| private fun readTail(file: File?): String? { | ||
| check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) { "MAX_LOG_DISPLAY_BYTES exceeds Int.MAX_VALUE" } | ||
| if (file == null || !file.exists()) return null | ||
| return try { | ||
| val len = file.length() | ||
| if (len <= MAX_LOG_DISPLAY_BYTES) { | ||
| file.readText(Charsets.UTF_8) | ||
| } else { | ||
| val start = maxOf(0L, len - MAX_LOG_DISPLAY_BYTES) | ||
| java.io.RandomAccessFile(file, "r").use { raf -> | ||
| raf.seek(start) | ||
| val bytes = ByteArray((len - start).toInt()) | ||
| raf.readFully(bytes) | ||
| val text = bytes.toString(Charsets.UTF_8) | ||
| // drop partial first line | ||
| val idx = text.indexOf('\n') | ||
| val trimmed = if (idx >= 0) text.substring(idx + 1) else text | ||
| "... (${len / 1024}KB, showing last ${MAX_LOG_DISPLAY_BYTES / 1024}KB) ...\n$trimmed" | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| "Failed to read file: ${e.message}" | ||
| } | ||
| } |
There was a problem hiding this comment.
readTail returns null for "file not found", making callers show "Loading..." indefinitely.
Line 289 returns null for both file == null and !file.exists(). Both callers use crashText ?: "Loading..." / wineText ?: "Loading...", so if latestCrashFile existed when LaunchedEffect ran but was deleted before the user opens the dialog, readTail silently returns null and the dialog shows "Loading..." with no way to distinguish it from a genuine in-progress load.
🛠️ Proposed fix — return a distinct string for the "file not found" case
- if (file == null || !file.exists()) return null
+ if (file == null) return null
+ if (!file.exists()) return "File no longer available."Separate optional nit: e.message is nullable on the JVM; for certain exceptions the error string becomes "Failed to read file: null". Consider e.message ?: e.javaClass.simpleName as a fallback.
✨ Proposed improvement
- "Failed to read file: ${e.message}"
+ "Failed to read file: ${e.message ?: e.javaClass.simpleName}"📝 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.
| private fun readTail(file: File?): String? { | |
| check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) { "MAX_LOG_DISPLAY_BYTES exceeds Int.MAX_VALUE" } | |
| if (file == null || !file.exists()) return null | |
| return try { | |
| val len = file.length() | |
| if (len <= MAX_LOG_DISPLAY_BYTES) { | |
| file.readText(Charsets.UTF_8) | |
| } else { | |
| val start = maxOf(0L, len - MAX_LOG_DISPLAY_BYTES) | |
| java.io.RandomAccessFile(file, "r").use { raf -> | |
| raf.seek(start) | |
| val bytes = ByteArray((len - start).toInt()) | |
| raf.readFully(bytes) | |
| val text = bytes.toString(Charsets.UTF_8) | |
| // drop partial first line | |
| val idx = text.indexOf('\n') | |
| val trimmed = if (idx >= 0) text.substring(idx + 1) else text | |
| "... (${len / 1024}KB, showing last ${MAX_LOG_DISPLAY_BYTES / 1024}KB) ...\n$trimmed" | |
| } | |
| } | |
| } catch (e: Exception) { | |
| "Failed to read file: ${e.message}" | |
| } | |
| } | |
| private fun readTail(file: File?): String? { | |
| check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) { "MAX_LOG_DISPLAY_BYTES exceeds Int.MAX_VALUE" } | |
| if (file == null) return null | |
| if (!file.exists()) return "File no longer available." | |
| return try { | |
| val len = file.length() | |
| if (len <= MAX_LOG_DISPLAY_BYTES) { | |
| file.readText(Charsets.UTF_8) | |
| } else { | |
| val start = maxOf(0L, len - MAX_LOG_DISPLAY_BYTES) | |
| java.io.RandomAccessFile(file, "r").use { raf -> | |
| raf.seek(start) | |
| val bytes = ByteArray((len - start).toInt()) | |
| raf.readFully(bytes) | |
| val text = bytes.toString(Charsets.UTF_8) | |
| // drop partial first line | |
| val idx = text.indexOf('\n') | |
| val trimmed = if (idx >= 0) text.substring(idx + 1) else text | |
| "... (${len / 1024}KB, showing last ${MAX_LOG_DISPLAY_BYTES / 1024}KB) ...\n$trimmed" | |
| } | |
| } | |
| } catch (e: Exception) { | |
| "Failed to read file: ${e.message ?: e.javaClass.simpleName}" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`
around lines 287 - 310, readTail currently returns null when file is null or
doesn't exist which causes callers (crashText/wineText) to display "Loading..."
forever; change readTail so that when file == null or !file.exists() it returns
a distinct informative string (e.g., "File not found" possibly including
file?.name) instead of null, and in the catch block return "Failed to read file:
${e.message ?: e.javaClass.simpleName}" so the error message never becomes
"null"; update references to readTail (used for latestCrashFile handling)
accordingly so callers can distinguish missing files from a real loading state.
025008c to
fca4c28
Compare
| try { | ||
| ContainerUtils.getContainer(context, appId) | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Container unavailable for $appId, likely process death") | ||
| null | ||
| } | ||
| } | ||
|
|
||
| if (container == null) { | ||
| LaunchedEffect(Unit) { navigateBack() } | ||
| return |
There was a problem hiding this comment.
What's this? I don't think we should make this change. It just closes the game if the container doesn't exist? Isn't that what happens anyway if the container doesn't exist?
There was a problem hiding this comment.
The difference is crash vs graceful exit. Without the guard, if the container is gone after process death (Intents launch is the typical launch case), getContainer() throws and the app crashes. With the guard, it catches the exception and navigates back to Home cleanly. So same end result (game closes), but without a crash dialog for the user.
There was a problem hiding this comment.
Changes in this file also seem unnecessary? Please confirm
There was a problem hiding this comment.
sorry, I put two distinct issues into a single PR, I'm going to close/reopen 2 cleaner PRs
There was a problem hiding this comment.
#605 is the logs OOM PR. This PR is cleaned up and isolated to the container death scenario.
jeremybernstein
force-pushed
the
jb/intents_crash_pr
branch
from
47413d4 to
438ff20
Compare
jeremybernstein
changed the title
|
@jeremybernstein lgtm but there's a conflict |
|
dang, rebased yesterday. I'll take care of it. |
|
ah probably conflicts with #609 which is in a similar domain |
jeremybernstein
force-pushed
the
jb/intents_crash_pr
branch
from
438ff20 to
a5be0b9
Compare
|
@utkarshdalal this will merge cleanly now. I am still seeing |
2 participants
Summary
getContainer()in XServerScreen — if container is gone after process death, navigate back instead of crashingTest plan
Fixes #619