Dirt/pm 30319/phish cache freeze #18157
Open
+66
−12
Conversation
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
Remove automatic cache update triggering that caused UI freezes when switching to accounts with phishing detection access. Root cause: The update$ observable used startWith(undefined) which triggered an immediate cache refresh whenever a new subscription was created. On account switch, phishingDetectionSettingsService.on$ emits true, creating a new subscription and triggering a full ~800K entry fetch that blocks the UI thread. Fix: - Remove startWith(undefined) to prevent auto-triggering on subscription - Add MIN_UPDATE_INTERVAL (5 min) constant for cache freshness checks - Add _updateInProgress flag to prevent concurrent updates - Add filter() to skip updates when one is already in progress - Add cache freshness check (skip if updated within 5 minutes) - Add finalize() to reliably reset _updateInProgress flag (per ADR) - Replace share() with shareReplay() to prevent duplicate work - Add triggerUpdateIfNeeded() public method for explicit update requests The scheduled 24-hour update interval is unaffected - it still calls _triggerUpdate$.next() via the task scheduler.
Update PhishingDetectionService to explicitly trigger cache updates when phishing detection becomes active for an account, using a non-blocking pattern. Changes: - Add call to phishingDataService.triggerUpdateIfNeeded() when on$ emits true - Use of(null).pipe(delay(0)) to defer update to next event loop tick - This prevents the update from blocking the account switch UI flow The delay(0) pattern is preferred over setTimeout per codebase conventions (RxJS over native JS). The subscription auto-completes since of() emits once and completes, so no manual cleanup is needed. Combined with the previous commit's safeguards (cache freshness check, concurrent update prevention), this ensures: 1. Account switch completes immediately (non-blocking trigger) 2. Cache updates only run when actually needed (< 5 min freshness) 3. Concurrent updates are prevented (_updateInProgress flag) Fixes: PM-30319
Move phishingDataService.update$ to a separate subscription outside the merge() stream to prevent blocking the service worker during critical initialization and account switch flows. Background: The service worker is single-threaded. When the phishing cache update runs, it downloads a 25MB file and parses 800K entries using .split(), which is CPU-intensive synchronous work. During this parsing, the service worker cannot respond to popup requests, causing the extension UI to appear frozen when the user clicks the extension icon. Previously, update$ was included in the merge() alongside UI event handlers (onTabUpdated$, onContinueCommand$, onCancelCommand$). When on$ emitted true (user has phishing access), the merge subscription was created as part of the same synchronous flow, coupling the heavy cache work with the UI event setup. Changes: - Create separate updateSub subscription at initialization - Remove update$ from merge() - now only contains UI event streams - Keep delay(0) trigger for triggerUpdateIfNeeded() How delay(0) works: JavaScript's event loop must complete all synchronous code before processing async callbacks. delay(0) schedules the trigger for the next event loop tick, meaning: 1. initialize() completes and returns 2. Service worker is 'free' to handle other tasks 3. Next tick: triggerUpdateIfNeeded() fires 4. Cache update runs in background The cache parsing will still block the thread when it eventually runs, but this is now decoupled from the critical initialization path. The window where blocking can affect user interaction is minimized. PM-30319
Contributor
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18157 +/- ##
==========================================
- Coverage 42.26% 42.25% -0.02%
==========================================
Files 3599 3599
Lines 104516 104532 +16
Branches 15776 15778 +2
==========================================
- Hits 44171 44167 -4
- Misses 58465 58485 +20
Partials 1880 1880 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-30319
📔 Objective
Problem: When switching to an account with phishing detection access, the browser extension UI freezes. The phishing cache update runs synchronously, blocking the service worker thread and preventing the popup from responding.
Solution: Decouple the cache update from the critical UI path by moving it to a background subscription and deferring triggers to the next event loop tick.
Changes
phishing-data.service.tsstartWith(undefined)— prevents auto-triggering on subscriptionMIN_UPDATE_INTERVAL(5 min) — skips updates if cache is fresh_updateInProgressflag withfilter()— prevents concurrent updatesfinalize()— reliably resets flag on success/error/completionshare()withshareReplay()— prevents duplicate subscriptions from triggering multiple fetchestriggerUpdateIfNeeded()method — explicit trigger for consumersphishing-detection.service.tsupdate$subscription outside themerge()— runs independently, doesn't block UI event handlersdelay(0)trigger — deferstriggerUpdateIfNeeded()to the next event loop tick, allowinginitialize()to complete firstphishing-detection.service.spec.tsupdate$: EMPTYmock to fix test compatibilityWhy This Works
The service worker is single-threaded. Previously, the cache update ran in the same flow as UI event setup.
Now:
initialize()completes immediately (in clients/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts)⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes