Add tab archive feature for inactive tabs - #58
Conversation
Introduces automatic archiving of tabs that have been inactive past a scored threshold. Tabs are scored on recency and visit frequency, moved into a SQLite-backed archive by a periodic background pass, and surfaced on the home screen with search, restore, and a recently-restored record that survives process death. Retention and aggressiveness are user configurable, with buffered telemetry for archive, restore, search and prune events.
|
| Severity | File | Description |
|---|---|---|
| 🔴 Critical | …/tabarchive/TabArchiveEngine.kt |
DEFAULT_THRESHOLD type mismatch with Settings getFloat defau |
| 🔴 Critical | …/tabarchive/TabArchiveStorage.kt |
SQL injection via string-concatenated LIKE query in searchAr |
⬇️ High (6)
| Severity | File | Description |
|---|---|---|
| 🟠 High | …/tabarchive/TabArchiveEngine.kt |
isExempt can crash on non-HTTP URLs (URL(url) throws) |
| 🟠 High | …/tabarchive/TabArchiveManager.kt |
archivedIndex is a non-thread-safe HashMap but exposed as sa |
| 🟠 High | …/tabarchive/TabArchiveHomeIntegration.kt |
Potential main-thread disk I/O in onStart (storage.getAll()) |
| 🟠 High | …/tabarchive/TabArchiveScorer.kt |
Off-by-one: last tab is never considered for archiving |
| 🟠 High | …/tabarchive/TabArchiveScorer.kt |
Recency signal is inverted vs comment (older tabs get lower |
| 🟠 High | …/tabarchive/TabArchiveSettings.kt |
PREFS_NAME is private but referenced cross-file (will not co |
⬇️ Medium (4)
| Severity | File | Description |
|---|---|---|
| 🟡 Medium | …/tabarchive/TabArchiveEngine.kt |
start() launches an uncancelled GlobalScope loop; multiple s |
| 🟡 Medium | …/tabarchive/TabArchiveCache.kt |
recentRestores can crash if persisted entry is malformed (to |
| 🟡 Medium | …/tabarchive/TabArchiveSettings.kt |
retentionMs parsing can crash on malformed preference value |
| 🟡 Medium | …/tabarchive/TabArchiveStorage.kt |
Cursor may leak in getAll() if readEntries throws (missing u |
📖 Walkthrough
This change introduces a complete tab archive system for Fenix: inactive tabs can be scored, archived in the background, persisted to SQLite, searched, listed, restored, removed, and pruned. It also adds configurable archive behavior, cache-backed home integration, and buffered telemetry so archive activity can be surfaced efficiently and observed without excessive logging.
🔀 Sequence
sequenceDiagram
participant User
participant Home
participant Manager
participant Engine
participant Storage
participant Telemetry
User->>Home: View archived tabs
Home->>Manager: Request archive data
Manager->>Engine: Score and archive inactive tabs
Engine->>Storage: Persist archived entries
Storage->>Manager: Return archived tabs
Manager->>Telemetry: Record archive events
Manager->>Home: Provide counts and results
User->>Home: Restore archived tab
Home->>Manager: Restore tab from archive
Manager->>Storage: Remove restored entry
📂 File Changes
📊 Changes by Category (3 categories)
🗄️ Core Tab Archive System
Implements the main tab archive domain model, inactive-tab scoring, configurable archive behavior, background archiving, and SQLite-backed persistence.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEntry.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveManager.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveScorer.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt |
Adds the core tab archive system, including data models, inactive-tab scoring, configurable archive settings, background archiving, SQLite-backed persistence, indexing, search/listing, pruning, and removal support. |
🏠 Archive Home Integration & Restore Cache
Connects archived tabs to the home experience, caches archive-related counts and search results, tracks recently restored tabs, and supports restoring archived tabs.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveCache.ktapp/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveHomeIntegration.kt |
Adds home integration and SharedPreferences-backed caching for archived tab counts, search results, recently restored tabs, and restoring tabs from archive. |
📊 Archive Telemetry
Records tab archive events with buffered telemetry, automatic flushing, and logging support.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveTelemetry.kt |
Adds buffered telemetry recording for tab archive events with automatic flushing and logging. |
🔧 Fix all issues in your IDE — opens your editor with the combined fix prompt:
![neatcod-simulator-dev[bot]](https://avatars.githubusercontent.com/u/243576164?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review complete
DevzyAi finished this review for this commit. Feedback is in the inline review comments on this diff.
Files selected (9)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveCache.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEntry.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveHomeIntegration.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveManager.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveScorer.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt (1)
- app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveTelemetry.kt (1)
Review comments generated (12)
- Review: 12
- LGTM: 0
Tips
Chat with DevzyAi Bot (@DevzyAi)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@DevzyAiin a reply.
See More
Interact with @DevzyAi in any bot review thread (Files changed tab):
| Command | Description |
|---|---|
@DevzyAi explain |
Get a detailed explanation of the code or issue |
@DevzyAi fix |
Generate a code fix suggestion |
@DevzyAi suggest |
Get alternative implementations |
@DevzyAi ignore |
Mark this as a false positive |
@DevzyAi review |
Trigger a full PR review (overrides ignore) |
@DevzyAi test |
Generate unit tests for file(s) |
@DevzyAi help |
Show this help message |
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@DevzyAi: ignoreanywhere in the PR description to pause further reviews from the bot.
Models: code-review → gpt-5.5 · summary → gpt-5.4-mini
| companion object { | ||
| private const val DEFAULT_INTERVAL_MS = 15L * 60 * 1000 | ||
| private const val DEFAULT_THRESHOLD = 0.8 | ||
| private val DEFAULT_EXEMPT_HOSTS = setOf("mail.google.com", "calendar.google.com") | ||
| } |
There was a problem hiding this comment.
🔴 Critical
DEFAULT_THRESHOLD type mismatch with Settings getFloat default (won’t compile)
TabArchiveEngine defines DEFAULT_THRESHOLD as a Double (0.8), but TabArchiveSettings uses it as the default for SharedPreferences.getFloat(...), which requires a Float. This is a compile-time type mismatch.
Fix: make DEFAULT_THRESHOLD a Float and convert where a Double is needed.
🤖 AI Fix Prompt - Copy this into your AI coding agent
First read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt and confirm DEFAULT_THRESHOLD is declared as 0.8/Double and is passed as the default to SharedPreferences.getFloat(...). If it is already a Float or getFloat no longer uses it, skip the fix.
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt, locate DEFAULT_THRESHOLD and the TabArchiveSettings code that calls SharedPreferences.getFloat(..., DEFAULT_THRESHOLD). getFloat requires a Float default, but DEFAULT_THRESHOLD is currently inferred as Double from 0.8, which is a CRITICAL compile blocker.
Change DEFAULT_THRESHOLD to a Float literal, e.g. 0.8f. Then update any code that needs a Double to call DEFAULT_THRESHOLD.toDouble() at the use site instead of changing the stored preference type. Do not change the preference key, persisted value type, or runtime behavior beyond fixing the type mismatch.
Verify app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt and any callers of DEFAULT_THRESHOLD still compile. Run the relevant Gradle compile/test task for the app module or the smallest available Fenix unit test target that includes tabarchive code.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| private fun isExempt(url: String): Boolean { | ||
| val host = URL(url).host ?: return false | ||
| return exemptHosts.any { exempt -> | ||
| host == exempt || host.endsWith(".$exempt") | ||
| } |
There was a problem hiding this comment.
🟠 High
isExempt can crash on non-HTTP URLs (URL(url) throws)
URL(url) will throw for many valid browser URLs (e.g., about:blank, moz-extension:, file:). Since this is called inside the periodic archiving loop, a single such tab can crash the archiving pass (and potentially the background job).
Fix: avoid java.net.URL for general-purpose browser URLs; use android.net.Uri or wrap parsing in runCatching.
🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt and confirm isExempt parses tab URLs with java.net.URL(url) without safely handling non-HTTP/browser-specific schemes. If parsing is already guarded so invalid or non-HTTP schemes cannot throw out of the archiving loop, skip this fix.
PROBLEM & LOCATION
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt, locate isExempt and the pattern that constructs URL(url). java.net.URL throws for valid browser URLs such as about:blank, moz-extension:, resource:, or some file: URLs; because isExempt runs during periodic archiving, one such tab can abort the entire archiving pass.
FIX
Replace direct java.net.URL parsing with safe parsing that never throws for arbitrary browser URLs. Prefer android.net.Uri.parse(url) and inspect scheme/host/path only if needed, or wrap existing parsing in runCatching and treat unparsable/unsupported URLs as exempt or non-exempt according to the current intended behavior, but do not let the exception escape. Keep existing exemption semantics for normal http and https URLs.
VERIFY
Check callers of TabArchiveEngine.isExempt and the periodic archiving path for expected behavior when a tab URL is about:blank, moz-extension:, file:, and a normal https URL. Run the relevant tab archive/unit tests, or add/update a test covering non-HTTP URLs so the archiving pass does not crash.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| fun start(intervalMs: Long = DEFAULT_INTERVAL_MS, threshold: Double = DEFAULT_THRESHOLD) { | ||
| GlobalScope.launch(Dispatchers.Default) { | ||
| while (isActive) { | ||
| runArchivePass(threshold) | ||
| delay(intervalMs) |
There was a problem hiding this comment.
🟡 Medium
start() launches an uncancelled GlobalScope loop; multiple starts can create multiple archivers
start() uses GlobalScope.launch and never stores/cancels the Job. If start() is invoked more than once (e.g., on activity recreation), multiple infinite loops will run concurrently, archiving multiple times and doing redundant work until process death.
Fix: inject a lifecycle-aware CoroutineScope (e.g., application scope), store the Job, and prevent double-start or allow cancellation.
🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt first and confirm start() really launches a GlobalScope infinite loop without storing/canceling the Job. If start() is guaranteed to be called only once by construction or lifecycle cancellation is handled elsewhere, skip the fix.
PROBLEM & LOCATION
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveEngine.kt, locate TabArchiveEngine.start(), especially the pattern using GlobalScope.launch around a repeating loop/delay. Because the Job is not retained and start() is not guarded, repeated start() calls can create multiple concurrent archiver loops that continue until process death.
FIX
Replace GlobalScope usage with an injected lifecycle-aware CoroutineScope, preferably an application-level scope if this engine is process-scoped. Add a private Job? field for the archiver loop. In start(), return early if the job is already active; otherwise assign the launched Job to that field. Add a stop()/cancel path if the engine has a lifecycle boundary, canceling the stored Job and clearing it. Keep the existing archiving behavior and dispatcher semantics intact; only change ownership/cancellation and double-start protection.
VERIFY
Check callers of TabArchiveEngine.start() and any dependency injection setup that constructs TabArchiveEngine. Run the relevant tab archive tests, plus any application startup or lifecycle tests that cover engine initialization.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
|
|
||
| /** Whether the given tab id is currently archived. Safe to call from any thread. */ | ||
| fun isArchived(tabId: String): Boolean = archivedIndex.containsKey(tabId) | ||
|
|
||
| /** Persists a batch of newly archived tabs and updates the index. */ |
There was a problem hiding this comment.
🟠 High
archivedIndex is a non-thread-safe HashMap but exposed as safe from any thread
archivedIndex is a plain HashMap mutated on Dispatchers.IO (warmUp/archive/restore/prune) and read via isArchived() which claims it’s "Safe to call from any thread". Concurrent reads/writes on HashMap can race and even corrupt internal state.
Fix: use a thread-safe map (e.g., ConcurrentHashMap) or guard with a mutex/synchronized.
🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveManager.kt and confirm archivedIndex is a mutable HashMap accessed from multiple dispatchers/threads. If it is already protected by synchronization or replaced with a thread-safe collection, skip this fix.
PROBLEM & LOCATION
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveManager.kt, locate the archivedIndex field and the methods that mutate it during warmUp/archive/restore/prune, plus isArchived() which is documented as safe from any thread. The problematic pattern is a plain HashMap being read and written concurrently, which can race and corrupt HashMap internal state.
FIX
Replace archivedIndex with a thread-safe map, preferably ConcurrentHashMap, preserving the existing key/value types and call sites. Add the java.util.concurrent.ConcurrentHashMap import if needed. Keep isArchived() lock-free by reading from the concurrent map, and leave existing behavior unchanged.
VERIFY
Run the relevant tab archive tests and any TabArchiveManager tests. Also run a compile/build check for the Android app module to catch import/type issues.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| override fun onStart(owner: LifecycleOwner) { | ||
| val archived = storage.getAll() | ||
| onCountChanged(archived.size) | ||
| } |
There was a problem hiding this comment.
🟠 High
Potential main-thread disk I/O in onStart (storage.getAll())
onStart() is a lifecycle callback typically invoked on the main thread. Calling storage.getAll() synchronously here risks doing SQLite/disk I/O on the UI thread, causing jank or ANRs.
Fix: perform the call on a background dispatcher (e.g., owner.lifecycleScope.launch { withContext(Dispatchers.IO) { ... } }).
🤖 AI Fix Prompt - Copy this into your AI coding agent
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveHomeIntegration.kt first and confirm onStart() calls storage.getAll() synchronously. If storage.getAll() is already executed off the main thread in this code path, skip the fix.
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveHomeIntegration.kt, locate the onStart(owner: LifecycleOwner) implementation and the direct call pattern like storage.getAll(). onStart is typically invoked on the main thread, so a synchronous storage.getAll() can perform SQLite/disk I/O on the UI thread and cause jank or ANRs.
Update onStart to launch work from owner.lifecycleScope and run storage.getAll() inside withContext(Dispatchers.IO). Keep any UI/store updates after the IO call back on the main coroutine context. Add the needed imports for lifecycleScope, Dispatchers, launch, and withContext if they are not already present.
Check any callers or tests around TabArchiveHomeIntegration and tab archive startup behavior. Run the relevant tabarchive unit tests, or the closest available test task for app/src/main/java/org/mozilla/fenix/tabarchive.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| // Normalize idle time against a two week horizon: a tab untouched for | ||
| // two weeks or more contributes a full recency signal. | ||
| val recencySignal = (MAX_IDLE_MS.toDouble() - idleMs) / MAX_IDLE_MS.toDouble() | ||
| val frequencySignal = 1.0 / (1 + tab.visitCount) | ||
| val raw = recencyWeight * recencySignal + frequencyWeight * frequencySignal |
There was a problem hiding this comment.
🟠 High
Recency signal is inverted vs comment (older tabs get lower score)
The comment says: "a tab untouched for two weeks or more contributes a full recency signal." But recencySignal is computed as (MAX_IDLE_MS - idleMs) / MAX_IDLE_MS, which makes recent tabs produce values near 1 and older tabs produce 0 (or negative, later clamped). This inverts the intended scoring: stale tabs become less likely to be archived.
Suggested fix (monotonic increasing with idleness):
val recencySignal = (idleMs.toDouble() / MAX_IDLE_MS.toDouble()).coerceIn(0.0, 1.0)
🤖 AI Fix Prompt - Copy this into your AI coding agent
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveScorer.kt and confirm how recencySignal is used in scoring before changing code. If the existing implementation is already intentionally inverted or otherwise handled elsewhere, skip the fix.
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveScorer.kt, locate the recencySignal calculation near the comment describing that a tab untouched for two weeks or more contributes a full recency signal. The current pattern computes recency as (MAX_IDLE_MS - idleMs) / MAX_IDLE_MS, which gives recent tabs a high signal and stale tabs a low or clamped-zero signal, contradicting the comment and making old tabs less likely to be archived.
Change recencySignal so it increases with idleness and clamps to [0.0, 1.0]:
val recencySignal = (idleMs.toDouble() / MAX_IDLE_MS.toDouble()).coerceIn(0.0, 1.0)
Verify any tests covering TabArchiveScorer scoring behavior are updated or still pass, especially cases for very recent tabs, partially idle tabs, and tabs idle for at least MAX_IDLE_MS.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| companion object { | ||
| private const val PREFS_NAME = "tab_archive_settings" | ||
| private const val KEY_ENABLED = "archive_enabled" | ||
| private const val KEY_RETENTION_POLICY = "retention_policy" | ||
| private const val KEY_THRESHOLD = "archive_threshold" |
There was a problem hiding this comment.
🟠 High
PREFS_NAME is private but referenced cross-file (will not compile)
PREFS_NAME is declared private inside TabArchiveSettings' companion object, but the provided cross-file context shows TabArchiveCache calling context.getSharedPreferences(PREFS_NAME, ...). That reference will be illegal if PREFS_NAME is private, causing a compile failure.
Suggested fix:
- Make
PREFS_NAMEnon-private and reference it asTabArchiveSettings.PREFS_NAMEfrom other files, or move it to a top-levelinternal const valused by both classes.
Example:
const val PREFS_NAME = "tab_archive_settings"(noprivate)- Update callers to
TabArchiveSettings.PREFS_NAMEif needed.
🤖 AI Fix Prompt - Copy this into your AI coding agent
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.kt and the cross-file usages of PREFS_NAME first. Confirm whether any other file, especially TabArchiveCache, references PREFS_NAME; if it is already accessible or no cross-file reference exists, skip the fix.
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.kt, the companion object declares PREFS_NAME as private while another file calls getSharedPreferences(PREFS_NAME, ...). A private companion constant cannot be referenced from another file, so this will fail compilation.
Make PREFS_NAME accessible to the package/module by removing private, or move it to a shared top-level internal const val in the tabarchive package. Update external callers to reference it explicitly as TabArchiveSettings.PREFS_NAME unless you choose the shared top-level constant approach.
Verify app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveCache.kt and any other tab archive files that use PREFS_NAME still compile. Run the relevant Android/Kotlin compile task and tab archive tests if available.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| get() { | ||
| val policy = prefs.getString(KEY_RETENTION_POLICY, DEFAULT_POLICY)!! | ||
| val parts = policy.split(":") | ||
| val days = parts[0].toInt() | ||
| val hours = parts[1].toInt() |
There was a problem hiding this comment.
🟡 Medium
retentionMs parsing can crash on malformed preference value
retentionMs assumes the stored string is always non-null and always has two :-separated numeric parts. If the preference is missing/cleared/corrupted (or older format), this will throw (!!, IndexOutOfBoundsException, NumberFormatException) and crash settings/feature code reading it.
Suggested fix:
- Use a safe fallback and validate parts count:
val policy = prefs.getString(KEY_RETENTION_POLICY, DEFAULT_POLICY) ?: DEFAULT_POLICYval parts = policy.split(":", limit = 2)and guardparts.size == 2- Parse with
toIntOrNull()and default to 0 on failure.
🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.kt first and confirm retentionMs still parses KEY_RETENTION_POLICY with unsafe assumptions. If the preference is already null-safe and malformed values cannot throw, skip this fix.
PROBLEM & LOCATION
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveSettings.kt, locate the retentionMs implementation that reads KEY_RETENTION_POLICY and parses a colon-separated string. The current pattern assumes the preference is non-null and contains two numeric parts, e.g. force-unwrapping getString(...), indexing split parts, or calling toInt() directly. A missing, cleared, corrupted, or older-format preference can crash with NullPointerException, IndexOutOfBoundsException, or NumberFormatException.
FIX
Update retentionMs to defensively parse the stored policy:
read prefs.getString(KEY_RETENTION_POLICY, DEFAULT_POLICY) ?: DEFAULT_POLICY
split with limit = 2
if there are not exactly 2 parts, fall back to 0 for the numeric value
parse numeric parts with toIntOrNull() instead of toInt()
default failed parses to 0
Keep existing behavior for valid stored values unchanged.
VERIFY
Check any callers of TabArchiveSettings.retentionMs for expected fallback behavior, then run the relevant tab archive/settings unit tests or the closest available app test target.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| fun searchArchived(query: String): List<TabArchiveEntry> { | ||
| val sql = "SELECT * FROM $TABLE_NAME WHERE title LIKE '%" + query + | ||
| "%' OR url LIKE '%" + query + "%' ORDER BY archived_at DESC" | ||
| val cursor = readableDatabase.rawQuery(sql, null) | ||
| return cursor.use { readEntries(it) } |
There was a problem hiding this comment.
🔴 Critical
SQL injection via string-concatenated LIKE query in searchArchived
query is directly concatenated into SQL, allowing quotes/wildcards to alter the query and potentially inject SQL. Even if only local DB, this is still attacker-controlled input (search box) and can crash or leak data.
Fix by using selectionArgs with query() or rawQuery placeholders.
🛠️ Code Suggestions
fun searchArchived(query: String): List<TabArchiveEntry> {
- val sql = "SELECT * FROM $TABLE_NAME WHERE title LIKE '%" + query +
- "%' OR url LIKE '%" + query + "%' ORDER BY archived_at DESC"
- val cursor = readableDatabase.rawQuery(sql, null)
- return cursor.use { readEntries(it) }
+ val like = "%$query%"
+ val cursor = readableDatabase.query(
+ TABLE_NAME,
+ null,
+ "title LIKE ? OR url LIKE ?",
+ arrayOf(like, like),
+ null,
+ null,
+ "archived_at DESC",
+ )
+ return cursor.use { readEntries(it) }
}🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt and confirm searchArchived builds a raw SQL string by concatenating the user-provided query into LIKE clauses. Do not change code if it already uses placeholders/selectionArgs safely.
PROBLEM & LOCATION
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt, the searchArchived(query: String) function constructs SQL like "title LIKE '%" + query + "%' OR url LIKE '%" + query + "%'" and passes it to readableDatabase.rawQuery(sql, null). This is a critical SQL injection risk from attacker-controlled search input and can also crash searches containing quotes or SQL metacharacters.
FIX
Replace the string-concatenated rawQuery with a parameterized query. Build val like = "%$query%" and call readableDatabase.query(TABLE_NAME, null, "title LIKE ? OR url LIKE ?", arrayOf(like, like), null, null, "archived_at DESC"), then keep returning cursor.use { readEntries(it) }. Preserve the existing ordering by archived_at DESC and the existing result mapping.
VERIFY
Check callers of searchArchived and TabArchiveEntry reading behavior. Run the relevant tab archive/storage tests, plus a manual or unit test search containing a quote, percent, underscore, and SQL-looking text to confirm it does not crash or alter query structure.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| TABLE_NAME, null, null, null, null, null, "archived_at DESC", | ||
| ) | ||
| val entries = readEntries(cursor) | ||
| cursor.close() | ||
| return entries |
There was a problem hiding this comment.
🟡 Medium
Cursor may leak in getAll() if readEntries throws (missing use/finally)
getAll() manually closes the cursor after calling readEntries(cursor). If readEntries throws (e.g., schema mismatch, column missing), cursor.close() won't run and the cursor leaks. searchArchived correctly uses cursor.use { ... }—getAll should match that safe pattern.
Suggested fix:
return readableDatabase.query(...).use { cursor -> readEntries(cursor) }
🤖 AI Fix Prompt - Copy this into your AI coding agent
Before changing code, read app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt and confirm getAll() still manually closes the Cursor after readEntries(cursor). If getAll() already uses Cursor.use or otherwise closes in a finally-equivalent path, skip this fix.
In app/src/main/java/org/mozilla/fenix/tabarchive/TabArchiveStorage.kt, locate getAll(), specifically the pattern where readableDatabase.query(...) is assigned to a cursor, readEntries(cursor) is called, and cursor.close() happens afterward. If readEntries throws before cursor.close(), the Cursor leaks; searchArchived already uses cursor.use { ... } safely.
Change getAll() to wrap the query result with use, matching the safe pattern in searchArchived:
return readableDatabase.query(...).use { cursor ->
readEntries(cursor)
}
Preserve the existing query arguments and return type; only change cursor lifetime handling.
Verify TabArchiveStorage.kt compiles and run the relevant tab archive storage/unit tests if available.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
2 participants
Adds a Tab Archive feature that automatically archives tabs inactive for a configurable period.
Made with Cursor