vc=87: audit #2 fix sprint — close two vc=86 regressions + MED/LOW
Fixes from the second code audit (which adversarially refute-verified every HIGH/CRIT — 0 survived; these are the confirmed MED/LOW): - Duplicate-key crash (M-1 + M-4): the subs feed, Search, and Channel lists key their LazyColumn by video url, but the sources weren't fully deduped — a video appearing twice (collab/cross-post in two subscribed feeds, or the same videoId across two search/channel shelves) made a duplicate Compose key → IllegalArgumentException → screen crash. Subs merge + Search/Channel FIRST page now distinctBy(url); the continuation paths already did. (M-4 was a regression of the vc=86 key additions, which only deduped the append path.) - IosSafe end-of-chunk roll (M-5): the vc=86 change set chunkRemaining=0 on inner EOF expecting the NEXT read() to roll, but Media3 stops calling read() after a -1, so a LENGTH_UNSET inner still truncated to the first chunk. The roll now happens WITHIN read() (loop) with a progress guard so a no-progress chunk can't spin; folds in the zero-length-chunk EOF case too. - SearchCacheStore.clear() is now atomic (updateAndGet) so a concurrent record() can't resurrect a just-cleared entry on disk (M-3). - Gate the YtRelated autoplay branch through isAllowedYtUrl to match the universal-allowlist invariant (L-11); no-op-guard setLatestKnownVersion (L-13); drop two unused TrackSelectionParameters imports (L-3). Deferred (low-risk hygiene, none a crash): SharedPreferences write-ordering serialization (M-2), the migration's dead Http.kt sweep (L-2), a stale-comment pass. Verified: full Android compileDebugKotlin green.
This commit is contained in:
parent
791975ca4a
commit
1fe6c12f1d
10 changed files with 129 additions and 54 deletions
|
|
@ -90,7 +90,11 @@ class SearchCacheStore(context: Context) {
|
|||
}
|
||||
|
||||
fun clear() {
|
||||
_entries.value = emptyList()
|
||||
// updateAndGet (not a plain `.value =`) so a concurrent record()
|
||||
// interleaving its read→build→persist can't leave disk = [entry] after
|
||||
// the user asked to clear — the same B5 race the StateFlow migration was
|
||||
// meant to close (audit #2 M-3).
|
||||
_entries.updateAndGet { emptyList() }
|
||||
sp.edit().remove(KEY).apply()
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -398,6 +398,10 @@ class SettingsStore(context: Context) {
|
|||
}
|
||||
|
||||
fun setLatestKnownVersion(vc: Long, vname: String) {
|
||||
// No-op guard to match every other setter in this file: runUpdateCheck
|
||||
// calls this on every up-to-date poll, so without it we rewrite the
|
||||
// same (0,"") to disk + re-emit the StateFlow each time (audit #2 L-13).
|
||||
if (_latestKnownVc.value == vc && _latestKnownVname.value == vname) return
|
||||
_latestKnownVc.value = vc
|
||||
_latestKnownVname.value = vname
|
||||
sp.edit()
|
||||
|
|
|
|||
|
|
@ -108,7 +108,11 @@ class ChannelViewModel : ViewModel() {
|
|||
viewCount = v.viewCount,
|
||||
uploadDateRelative = v.uploadDateRelative,
|
||||
)
|
||||
}
|
||||
}.distinctBy { it.url }
|
||||
// Dedup the FIRST page by url — the videos LazyColumn keys by
|
||||
// it.url (vc=86); a duplicate key hard-crashes Compose, which a
|
||||
// cross-shelf duplicate videoId can produce. loadMore already
|
||||
// dedups; this closes the initial-page gap (audit #2 M-4).
|
||||
if (_ui.value.loadedUrl != channelUrl) return@launch
|
||||
_ui.update {
|
||||
ChannelUiState(
|
||||
|
|
|
|||
|
|
@ -92,7 +92,6 @@ import androidx.compose.ui.unit.dp
|
|||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||
import androidx.lifecycle.viewmodel.compose.viewModel
|
||||
import androidx.media3.common.C
|
||||
import androidx.media3.common.TrackSelectionParameters
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import coil3.compose.AsyncImage
|
||||
import com.sulkta.straw.data.ChannelRef
|
||||
|
|
|
|||
|
|
@ -348,6 +348,13 @@ class SubscriptionFeedViewModel : ViewModel() {
|
|||
// memo is exact.
|
||||
val recencyMemo = HashMap<String, Long>()
|
||||
return channels.flatMap { ch -> channelCache[ch.url]?.items.orEmpty() }
|
||||
// Dedup by url BEFORE the url-keyed LazyColumn renders it: a video
|
||||
// can legitimately appear in two subscribed channels' feeds (a
|
||||
// collab, or a channel + its auto-generated "<name> - Topic"
|
||||
// mirror), and StrawHome keys the feed list by it.url — a duplicate
|
||||
// key hard-crashes the Subs tab (audit #2 M-1). distinctBy keeps
|
||||
// the first occurrence (channel order), which is fine for the sort.
|
||||
.distinctBy { it.url }
|
||||
.map { it.withEnrichment(enrichments) }
|
||||
.map { item -> item to recencyMemo.getOrPut(item.uploadDateRelative) { item.recencyScore() } }
|
||||
.sortedWith(
|
||||
|
|
|
|||
|
|
@ -352,12 +352,21 @@ class PlaybackService : MediaSessionService() {
|
|||
.firstOrNull()?.url
|
||||
}
|
||||
AutoplayMode.YtRelated -> {
|
||||
val info = uniffi.strawcore.streamInfo(currentStreamUrl)
|
||||
info.related
|
||||
.asSequence()
|
||||
.filter { it.url != currentStreamUrl }
|
||||
.filter { unwatched(it.url) }
|
||||
.firstOrNull()?.url
|
||||
// Gate currentStreamUrl through the same allowlist the
|
||||
// SameChannel/candidate paths use before it reaches the
|
||||
// extractor — it can come from a MediaItem.localConfiguration
|
||||
// .uri fallback, not always a freshly-revalidated YT url
|
||||
// (audit #2 L-11, matches the universal-gate comment above).
|
||||
if (!isAllowedYtUrl(currentStreamUrl)) {
|
||||
null
|
||||
} else {
|
||||
val info = uniffi.strawcore.streamInfo(currentStreamUrl)
|
||||
info.related
|
||||
.asSequence()
|
||||
.filter { it.url != currentStreamUrl }
|
||||
.filter { unwatched(it.url) }
|
||||
.firstOrNull()?.url
|
||||
}
|
||||
}
|
||||
}
|
||||
} catch (c: kotlinx.coroutines.CancellationException) {
|
||||
|
|
|
|||
|
|
@ -69,7 +69,6 @@ import androidx.lifecycle.viewmodel.compose.viewModel
|
|||
import androidx.media3.common.C
|
||||
import androidx.media3.common.PlaybackParameters
|
||||
import androidx.media3.common.Player
|
||||
import androidx.media3.common.TrackSelectionParameters
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.media3.ui.PlayerView
|
||||
import com.sulkta.straw.OverlayChromeColor
|
||||
|
|
|
|||
|
|
@ -246,7 +246,11 @@ class SearchViewModel : ViewModel() {
|
|||
viewCount = r.viewCount,
|
||||
uploadDateRelative = r.uploadDateRelative,
|
||||
)
|
||||
}
|
||||
}.distinctBy { it.url }
|
||||
// Dedup the FIRST page by url — the results LazyColumn keys by
|
||||
// it.url (vc=86) and Compose hard-crashes on a duplicate key,
|
||||
// which a cross-shelf duplicate videoId can produce. loadMore
|
||||
// already dedups; this closes the initial-page gap (audit #2 M-4).
|
||||
// Fence by job identity (ensureActive) — only a fresh
|
||||
// submit that called inFlight?.cancel() invalidates
|
||||
// me. Bare typing in the search bar (onQueryChange)
|
||||
|
|
|
|||
|
|
@ -45,6 +45,13 @@ class IosSafeHttpDataSource(
|
|||
/** Bytes left in the current inner-open chunk. -1 = unknown end. */
|
||||
private var chunkRemaining: Long = 0
|
||||
|
||||
/** `totalRead` at the moment the current chunk was opened. The roll loop
|
||||
* in read() uses it as a progress guard: a chunk that delivers zero new
|
||||
* bytes and then EOFs is the genuine end of stream (or a stuck server),
|
||||
* not a reason to re-open the same position — without this the roll could
|
||||
* spin forever. */
|
||||
private var chunkStartTotalRead: Long = 0
|
||||
|
||||
override fun open(dataSpec: DataSpec): Long {
|
||||
// When length is set, respect it but still cap the first inner-open to
|
||||
// chunkBytes. When length is unset, request a chunk and we'll roll
|
||||
|
|
@ -84,6 +91,7 @@ class IosSafeHttpDataSource(
|
|||
strawLogW(TAG, t) { "open failed: ${t.javaClass.simpleName}" }
|
||||
throw t
|
||||
}
|
||||
chunkStartTotalRead = totalRead
|
||||
strawLogD(TAG) { "open: inner chunkRemaining=$chunkRemaining" }
|
||||
// Report the original (potentially unbounded) length to the caller —
|
||||
// ExoPlayer cares about the overall length, not our internal chunking.
|
||||
|
|
@ -96,50 +104,64 @@ class IosSafeHttpDataSource(
|
|||
|
||||
override fun read(buffer: ByteArray, offset: Int, length: Int): Int {
|
||||
if (length == 0) return 0
|
||||
// Need a fresh chunk?
|
||||
if (chunkRemaining == 0L) {
|
||||
val spec = originalSpec ?: return C.RESULT_END_OF_INPUT
|
||||
inner.close()
|
||||
val nextPos = spec.position + totalRead
|
||||
val remainingOverall = if (spec.length == C.LENGTH_UNSET.toLong()) {
|
||||
Long.MAX_VALUE
|
||||
} else {
|
||||
spec.length - totalRead
|
||||
// Loop so a chunk-boundary roll happens WITHIN this read() call. The
|
||||
// earlier version set chunkRemaining=0 on inner EOF and `return read`
|
||||
// (== -1), expecting the NEXT read() to roll — but Media3 treats -1
|
||||
// from read() as terminal end-of-stream and never calls read() again,
|
||||
// so the deferred roll was dead code and a LENGTH_UNSET inner truncated
|
||||
// to the first chunk (audit #2 M-5).
|
||||
while (true) {
|
||||
// Need a fresh chunk?
|
||||
if (chunkRemaining == 0L) {
|
||||
val spec = originalSpec ?: return C.RESULT_END_OF_INPUT
|
||||
inner.close()
|
||||
val nextPos = spec.position + totalRead
|
||||
val remainingOverall = if (spec.length == C.LENGTH_UNSET.toLong()) {
|
||||
Long.MAX_VALUE
|
||||
} else {
|
||||
spec.length - totalRead
|
||||
}
|
||||
if (remainingOverall <= 0L) return C.RESULT_END_OF_INPUT
|
||||
val nextLen = remainingOverall.coerceAtMost(chunkBytes)
|
||||
// Same as in open() — use buildUpon().setPosition/setLength
|
||||
// rather than subrange() so the absolute position stays valid.
|
||||
val nextSpec = spec.buildUpon()
|
||||
.setPosition(nextPos)
|
||||
.setLength(nextLen)
|
||||
.build()
|
||||
chunkRemaining = inner.open(nextSpec)
|
||||
chunkStartTotalRead = totalRead
|
||||
// A freshly-opened chunk that immediately reports 0 bytes (a
|
||||
// 0-length 206, or a past-EOF range answered with an empty body)
|
||||
// is the true end — don't spin re-opening the same position
|
||||
// (audit #2 L-10).
|
||||
if (chunkRemaining == 0L) return C.RESULT_END_OF_INPUT
|
||||
}
|
||||
if (remainingOverall <= 0L) return C.RESULT_END_OF_INPUT
|
||||
val nextLen = remainingOverall.coerceAtMost(chunkBytes)
|
||||
// Same as in open() — use buildUpon().setPosition/setLength rather
|
||||
// than subrange() so the absolute position stays meaningful.
|
||||
val nextSpec = spec.buildUpon()
|
||||
.setPosition(nextPos)
|
||||
.setLength(nextLen)
|
||||
.build()
|
||||
chunkRemaining = inner.open(nextSpec)
|
||||
}
|
||||
// Cap the read against what's left in this chunk.
|
||||
val toRead = if (chunkRemaining < 0L) {
|
||||
// Inner doesn't know its end either; just read what was asked.
|
||||
length
|
||||
} else {
|
||||
length.toLong().coerceAtMost(chunkRemaining).toInt()
|
||||
}
|
||||
val read = inner.read(buffer, offset, toRead)
|
||||
if (read != C.RESULT_END_OF_INPUT) {
|
||||
totalRead += read.toLong()
|
||||
if (chunkRemaining > 0L) chunkRemaining -= read.toLong()
|
||||
// If chunkRemaining hits 0 here, the next read() call will roll
|
||||
// to the next chunk via the block at the top.
|
||||
} else if (chunkRemaining != 0L) {
|
||||
// Inner hit EOF. Force a chunk roll on the next read() so we
|
||||
// re-open at the next position. `!= 0L` (not `> 0L`) so the
|
||||
// unknown-end case (LENGTH_UNSET inner → chunkRemaining < 0)
|
||||
// ALSO rolls: with `> 0L` a negative chunkRemaining never reset
|
||||
// to 0, so the top-of-read() roll block (gated on `== 0L`) never
|
||||
// fired and we re-read the exhausted inner forever — playback
|
||||
// truncated to the first chunk.
|
||||
// Cap the read against what's left in this chunk.
|
||||
val toRead = if (chunkRemaining < 0L) {
|
||||
// Inner doesn't know its end either; just read what was asked.
|
||||
length
|
||||
} else {
|
||||
length.toLong().coerceAtMost(chunkRemaining).toInt()
|
||||
}
|
||||
val read = inner.read(buffer, offset, toRead)
|
||||
if (read != C.RESULT_END_OF_INPUT) {
|
||||
totalRead += read.toLong()
|
||||
if (chunkRemaining > 0L) chunkRemaining -= read.toLong()
|
||||
return read
|
||||
}
|
||||
// Inner hit EOF. If this chunk delivered no new bytes, it's the
|
||||
// genuine end of the stream (or a stuck server) — stop rather than
|
||||
// re-open the same position forever (the roll-loop progress guard).
|
||||
if (totalRead == chunkStartTotalRead) {
|
||||
return C.RESULT_END_OF_INPUT
|
||||
}
|
||||
// The chunk delivered some bytes then EOF'd at its boundary — roll
|
||||
// to the next chunk and retry within this read() (loop back to the
|
||||
// top, which re-opens at nextPos; the remainingOverall<=0 /
|
||||
// 0-length-chunk guards terminate it).
|
||||
chunkRemaining = 0L
|
||||
}
|
||||
return read
|
||||
}
|
||||
|
||||
override fun close() {
|
||||
|
|
@ -149,6 +171,7 @@ class IosSafeHttpDataSource(
|
|||
originalSpec = null
|
||||
totalRead = 0
|
||||
chunkRemaining = 0
|
||||
chunkStartTotalRead = 0
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue