⚡ perf: Limit Firefox Bookmark Import size to prevent memory exhaustion#140
⚡ perf: Limit Firefox Bookmark Import size to prevent memory exhaustion#140Lucenx9 wants to merge 12 commits into
Conversation
Adds a `MAX_FIREFOX_BOOKMARKS` constant and a `LIMIT` clause to the SQLite query that reads Firefox bookmarks. This prevents memory issues and application thread stalls when reading from a malicious or bloated Firefox profile containing millions of entries, matching the existing soft cap used for Chromium profiles. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
More reviews will be available in 31 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR limits Firefox bookmarks to a maximum count and ensures they are returned in a consistent order by bookmark ID. The constant ChangesFirefox bookmarks limit and ordering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e7f9a47af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE b.type = 1 AND (p.url LIKE 'http://%' OR p.url LIKE 'https://%')", | ||
| )?; | ||
| WHERE b.type = 1 AND (p.url LIKE 'http://%' OR p.url LIKE 'https://%') | ||
| LIMIT {}", |
There was a problem hiding this comment.
When a Firefox profile has more than MAX_FIREFOX_BOOKMARKS matching bookmarks, this new LIMIT decides which bookmarks are imported, but the query has no ORDER BY. SQLite does not guarantee row order without an explicit ordering, so the capped import can silently keep an arbitrary subset that may change with indexes/query plans instead of a stable prefix such as bookmark id or date-added order.
Useful? React with 👍 / 👎.
Adds a `MAX_FIREFOX_BOOKMARKS` constant and a `LIMIT` clause to the SQLite query that reads Firefox bookmarks. This prevents memory issues and application thread stalls when reading from a malicious or bloated Firefox profile containing millions of entries, matching the existing soft cap used for Chromium profiles. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com>
Adds a `MAX_FIREFOX_BOOKMARKS` constant and a `LIMIT` clause to the SQLite query that reads Firefox bookmarks. This prevents memory issues and application thread stalls when reading from a malicious or bloated Firefox profile containing millions of entries, matching the existing soft cap used for Chromium profiles. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com>
…kmarks-17048942308476909152
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/forktty-import/src/history.rs (1)
197-230: ⚡ Quick winAdd a direct truncation assertion for the Firefox bookmark cap.
This test validates ordering, but not that results are actually capped at
MAX_FIREFOX_BOOKMARKS. Add one test insertingMAX_FIREFOX_BOOKMARKS + 1bookmark rows and assert returned length equalsMAX_FIREFOX_BOOKMARKS.Proposed test shape
+ #[test] + fn firefox_bookmarks_respect_max_limit() { + let dir = tempfile::tempdir().unwrap(); + let db = dir.path().join("places.sqlite"); + let conn = rusqlite::Connection::open(&db).unwrap(); + conn.execute_batch( + "CREATE TABLE moz_places (id INTEGER PRIMARY KEY, url TEXT, title TEXT); + CREATE TABLE moz_bookmarks (id INTEGER PRIMARY KEY, fk INTEGER, title TEXT, type INTEGER);", + ).unwrap(); + + let tx = conn.unchecked_transaction().unwrap(); + { + let mut p = tx.prepare("INSERT INTO moz_places (id,url,title) VALUES (?1, ?2, ?3)").unwrap(); + let mut b = tx.prepare("INSERT INTO moz_bookmarks (id,fk,title,type) VALUES (?1, ?2, ?3, 1)").unwrap(); + for i in 1..=(MAX_FIREFOX_BOOKMARKS as i64 + 1) { + let url = format!("https://{i}.test/"); + p.execute(rusqlite::params![i, url, "t"]).unwrap(); + b.execute(rusqlite::params![i, i, "bm"]).unwrap(); + } + } + tx.commit().unwrap(); + drop(conn); + + let bms = read_firefox_bookmarks(&db).unwrap(); + assert_eq!(bms.len(), MAX_FIREFOX_BOOKMARKS); + }Based on learnings: "Write Rust tests for browser command types, profile metadata, and history/bookmark stores in browser feature implementation".
🤖 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 `@crates/forktty-import/src/history.rs` around lines 197 - 230, Add a new unit test that specifically asserts truncation at MAX_FIREFOX_BOOKMARKS: create a temp DB, insert MAX_FIREFOX_BOOKMARKS + 1 rows into moz_places and moz_bookmarks (using the same pattern as firefox_bookmarks_are_ordered_by_bookmark_id_before_limit), call read_firefox_bookmarks(&db).unwrap(), and assert that the returned Vec length equals MAX_FIREFOX_BOOKMARKS; reference the existing helper read_firefox_bookmarks and the constant MAX_FIREFOX_BOOKMARKS to locate where to cap the results.
🤖 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.
Nitpick comments:
In `@crates/forktty-import/src/history.rs`:
- Around line 197-230: Add a new unit test that specifically asserts truncation
at MAX_FIREFOX_BOOKMARKS: create a temp DB, insert MAX_FIREFOX_BOOKMARKS + 1
rows into moz_places and moz_bookmarks (using the same pattern as
firefox_bookmarks_are_ordered_by_bookmark_id_before_limit), call
read_firefox_bookmarks(&db).unwrap(), and assert that the returned Vec length
equals MAX_FIREFOX_BOOKMARKS; reference the existing helper
read_firefox_bookmarks and the constant MAX_FIREFOX_BOOKMARKS to locate where to
cap the results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 512f4e6d-93d2-44cc-a81c-a1d115c1ba80
📒 Files selected for processing (1)
crates/forktty-import/src/history.rs
Adds a `MAX_FIREFOX_BOOKMARKS` constant and a `LIMIT` clause to the SQLite query that reads Firefox bookmarks. This prevents memory issues and application thread stalls when reading from a malicious or bloated Firefox profile containing millions of entries, matching the existing soft cap used for Chromium profiles. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com>
💡 What: Added a hard cap to the SQLite query reading Firefox bookmarks, capping the result set at
MAX_FIREFOX_BOOKMARKS(100,000) using aLIMITclause.🎯 Why: A user's Firefox
places.sqlitecan grow extremely large or be bloated by automated tools. Loading millions of rows into memory at once can exhaust memory resources or block the application thread during import. This brings Firefox behavior in line with the existing Chromium JSON parser cap of 100,000 nodes.📊 Measured Improvement:
This prevents memory footprint bloat and ensures the import function execution time is bounded and won't hang the application on excessively large databases.
PR created automatically by Jules for task 17048942308476909152 started by @Lucenx9
Performance: Firefox Bookmarks Import Size Limited to 100,000
User-visible behavior: Firefox bookmarks import now caps at 100,000 entries (matching the existing Chromium bookmark limit). Users with Firefox profiles containing more than 100,000 bookmarks will only import the first 100,000.
Core Rust impact:
MAX_FIREFOX_BOOKMARKSconstant (100,000) and modified theread_firefox_bookmarksSQLite query to includeORDER BY b.id ASCwith aLIMITclauseplaces.sqlitefilesTests:
firefox_bookmarks_are_ordered_by_bookmark_id_before_limit()unit test to verify bookmarks are returned in ascending bookmark-id order before the limit is appliedSecurity: Mitigates potential denial-of-service scenarios from maliciously oversized or bloated Firefox profiles that could exhaust system resources during import.