Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
910e698 to
5927434
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5927434. Configure here.
Two lists were rebuilt in place on a worker thread while readers read them on the main thread, and both repositories handed out the live instance. RealUnprotectedTemporaryRepository did clear() then one add() per row, so a reader could observe any prefix. With ~300 exception domains that is a millisecond-wide window, and it opens on every privacy config persist. RealContentScopeScripts reads the list on every navigation and serializes it into the injected script, so a truncated read meant content-scope features running on sites meant to be exempt. Both repositories now build the list aside and publish it in a single assignment to a @volatile field, following RealTrackerAllowlistRepository in the same module. UnprotectedTemporaryRepository.exceptions widens from CopyOnWriteArrayList<FeatureException> to List<FeatureException>: no -api change, and existing test stubs still compile. RealUserAllowListRepository used a single addAll() so it could never tear, and is here for the reason below. Because a published list is immutable, the optimized path in RealContentScopeScripts holds the references rather than copying into two CopyOnWriteArrayLists, comparing !== then !=. Identity settles the unchanged case; equals still stops a reload that changed nothing from reassembling the script. The legacy path keeps its copies and now clears the optimized baselines, because it rewrites the shared JSON fields while keeping its own: without that, a mid-session flag flip followed by an input returning to its pre-flip value left the optimized path seeing no change while the shared JSON held what legacy last wrote. Tests cover a reload never exposing a partial list, a previously returned list being unaffected by a reload, a failed reload retaining the previous list, a new instance with equal contents not reassembling the script, and both flag-flip revert cases. Each was verified by mutation to fail against the previous implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both getScript() paths write the same JSON caches but keep separate change-detection baselines, so resetting the optimized baselines from inside getLegacyScript() left a baseline that no longer described what the cache held. Reachable three ways: an input emptied while on legacy (reset value collides with the real one), an input returning to its pre-flip value while on legacy, and the same collision on the empty plugin config. Detect the flip in getScript() and drop the baselines together with the JSON caches they describe, landing on the state a fresh instance has. getLegacyScript() is back to its pre-flip-handling form, and in a session where the flag never moves the reset never runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
80bb528 to
605346c
Compare
|
|
||
| override val exceptions get() = snapshot | ||
|
|
||
| init { |
There was a problem hiding this comment.
While we're here, can we move this from init to onCreate from MainProcessLifecycleObserver?
There was a problem hiding this comment.
@CrisBarreiro I think it's bigger than it looks, and it's better to do it as a follow-up as part of the modularization AoI.
privacy-config-store currently has no di and no lifecycle dependencies. Implementing MainProcessLifecycleObserver means adding :di + lifecycle-runtime to a pure persistence module. Other classes in the same module (ContentBlocking, Gpc, Https, Drm, etc) use the identical init pattern, so doing just this one makes the module inconsistent.


Task/Issue URL: https://app.asana.com/1/137249556945/project/1200905986587319/task/1217019532582642?focus=true
Tech Design URL (if applicable):
API Proposals URL(s) (if applicable):
Description
Two privacy-config lists were rebuilt in place on a worker thread while readers read them on the main thread, and both repositories handed out the live instance.
RealUnprotectedTemporaryRepositorydidclear()then oneadd()per row, so a reader could observe any prefix — a millisecond-wide window on every config persist, with ~300 exception domains.RealContentScopeScriptsreads that list on every navigation and serializes it into the injected script, so a truncated read meant content-scope features running on sites meant to be exempt.Both repositories now build the list aside and publish it in a single assignment to a
@Volatilefield, followingRealTrackerAllowlistRepositoryin the same module.UnprotectedTemporaryRepository.exceptionswidens fromCopyOnWriteArrayList<FeatureException>toList<FeatureException>; no-apichange, and existing test stubs are unaffected.RealUserAllowListRepositoryused a singleaddAll()so it could never tear, and is included for the second reason below.Because a published list is now immutable, the optimized path in
RealContentScopeScriptsdrops its two defensiveCopyOnWriteArrayListcopies and holds the references instead, comparing!==then!=: identity settles the unchanged case in O(1), and equals still stops a reload that changed nothing from reassembling the script. The legacy path keeps its copies and now clears the optimized baselines, so a mid-session flag flip followed by an input returning to its pre-flip value cannot leave the shared JSON stale.Steps to test this PR
With
optimizeContentScopeInjectionON (default on internal)unprotectedTemporarychange takes effect.NO UI changes
Note
Medium Risk
Changes threading and caching for privacy exemptions and injected content-scope JS; incorrect behavior would mis-apply protections, but the change is well-tested around flag flips and list immutability.
Overview
Fixes a race where
RealUnprotectedTemporaryRepositoryandRealUserAllowListRepositoryrebuiltCopyOnWriteArrayListin place while readers (including content-scope script injection on every navigation) could see truncated exception or allowlist data.Both repositories now build aside and assign a new
Listto a@Volatilefield.UnprotectedTemporaryRepository.exceptionsis typed asList<FeatureException>instead of the liveCopyOnWriteArrayList. Failed reloads keep the previous snapshot; returned lists are not mutated after publish.In
RealContentScopeScripts, the optimized path drops defensiveCopyOnWriteArrayListcopies and uses reference + equality change detection (differsFrom) so equal content on a new list instance does not reassemble the script.resetCaches()runs whenoptimizeContentScopeInjectionflips mid-session so legacy and optimized baselines cannot leave shared JSON stale after flag toggles.Reviewed by Cursor Bugbot for commit 605346c. Bugbot is set up for automated code reviews on this repo. Configure here.