CPM: on-device heuristics and reload loop prevention#7862
CPM: on-device heuristics and reload loop prevention#7862
Conversation
muodov
changed the title
muodov
force-pushed
the
max/cpm-heuristic-reload-loop
branch
from
8280ecc to
77a0f24
Compare
| selfTestFailed: Boolean, | ||
| isCosmetic: Boolean?, | ||
| consentRule: String?, | ||
| consentReloadLoop: Boolean, |
| var consentSelfTestFailed: Boolean | ||
| var consentCosmeticHide: Boolean? | ||
| var consentRule: String? | ||
| var consentReloadLoop: Boolean |
| val consentOptOutFailed: Boolean, | ||
| val consentSelfTestFailed: Boolean, | ||
| val consentRule: String?, | ||
| val consentReloadLoop: Boolean, |
| val consentOptOutFailed: Boolean, | ||
| val consentSelfTestFailed: Boolean, | ||
| val consentRule: String?, | ||
| val consentReloadLoop: Boolean, |
| AUTOCONSENT_DETECTED_BY_PATTERNS_DAILY("m_autoconsent_detected-by-patterns_daily"), | ||
| AUTOCONSENT_DETECTED_BY_BOTH_DAILY("m_autoconsent_detected-by-both_daily"), | ||
| AUTOCONSENT_DETECTED_ONLY_RULES_DAILY("m_autoconsent_detected-only-rules_daily"), | ||
| AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY("m_autoconsent_error_reload-loop_daily"), |
There was a problem hiding this comment.
Needs PixelDefinition/privacy triage
...oconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
Outdated
Show resolved
Hide resolved
| site?.consentSelfTestFailed = selfTestFailed | ||
| site?.consentCosmeticHide = isCosmetic | ||
| site?.consentRule = consentRule | ||
| site?.consentReloadLoop = consentReloadLoop |
| ) | ||
|
|
||
| private val tabStates: MutableMap<WebView, TabState> = | ||
| Collections.synchronizedMap(WeakHashMap()) |
There was a problem hiding this comment.
Why use synchronizedMap? I don’t think we use it anywhere in the code base
...sent-impl/src/test/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetectorTest.kt
Outdated
Show resolved
Hide resolved
…oop=true paths - whenUserSettingIsTrueThenDetectReloadLoopShouldNotBeCalled (FAILS) - whenCmpEndsWithTopSuffixThenDetectReloadLoopShouldNotBeCalled (FAILS) - whenReloadLoopDetectedThenAutoconsentDoneResultHasReloadLoopTrue - whenReloadLoopDetectedThenSelfTestResultHasReloadLoopTrue Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…userSetting and IGNORE_CMP_SUFFIX guards detectReloadLoop was firing unconditionally, accumulating state and potentially firing the AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY pixel even when autoconsent was disabled for the site (userSetting=true) or for ignored CMPs (-top suffix). Neither case involves an actual opt-out attempt, so no reload loop can have occurred. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
| ) | ||
|
|
||
| private val tabStates: MutableMap<WebView, TabState> = | ||
| Collections.synchronizedMap(WeakHashMap()) |
There was a problem hiding this comment.
Collections.synchronizedMap can be dropped — a plain WeakHashMap() is correct here.
synchronizedMap makes individual map operations atomic, but the compound read-modify-write sequences in this class (getOrPut, and the read-then-write on TabState fields in detectReloadLoop) are not atomic regardless. So it is neither sufficient for real concurrency nor necessary in practice.
All callers invoke the detector on the main thread: PopUpFoundMessageHandlerPlugin.process() is synchronous, and OptOutAndAutoconsentDoneMessageHandlerPlugin calls rememberLastHandledCMP/isReloadLoopDetected in the synchronous body of processAutoconsentDone (the appCoroutineScope.launch there only covers the self-test scheduling that follows). WebView JS message callbacks arrive sequentially on the main thread, so for any given WebView the call sequence is inherently ordered.
|
Review: guard ordering bug in During review I found that
In both cases no opt-out attempt is ever made, so a "reload loop" can't have occurred. The pixel firing would be a false positive, and if the site is later visited with autoconsent active, the detector's state from the ignored visit would make the first real handling look like a loop immediately. What was added (two commits): Commit 1 — tests:
Commit 2 — fix: Moved |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: New consent fields may not reset on page change
- Added consentRule and consentReloadLoop reset in resetAutoConsent() and updated test to verify with non-default values.
- ✅ Fixed: Non-atomic
getOrPuton synchronized map causes race- Wrapped all getOrPut and state mutations in synchronized(tabStates) blocks to ensure atomic compound operations.
Or push these changes by commenting:
@cursor push 0c5ac266fd
Preview (0c5ac266fd)
diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
--- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
+++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
@@ -2420,6 +2420,8 @@
site?.consentManaged = false
site?.consentOptOutFailed = false
site?.consentSelfTestFailed = false
+ site?.consentRule = null
+ site?.consentReloadLoop = false
}
override fun getSite(): Site? = site
diff --git a/app/src/test/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/test/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
--- a/app/src/test/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
+++ b/app/src/test/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
@@ -4890,18 +4890,22 @@
optOutFailed = true,
selfTestFailed = true,
isCosmetic = true,
- consentRule = null,
- consentReloadLoop = false,
+ consentRule = "someCMP",
+ consentReloadLoop = true,
)
assertTrue(testee.siteLiveData.value?.consentManaged!!)
assertTrue(testee.siteLiveData.value?.consentOptOutFailed!!)
assertTrue(testee.siteLiveData.value?.consentSelfTestFailed!!)
assertTrue(testee.siteLiveData.value?.consentCosmeticHide!!)
+ assertEquals("someCMP", testee.siteLiveData.value?.consentRule)
+ assertTrue(testee.siteLiveData.value?.consentReloadLoop!!)
testee.onWebViewRefreshed()
assertFalse(testee.siteLiveData.value?.consentManaged!!)
assertFalse(testee.siteLiveData.value?.consentOptOutFailed!!)
assertFalse(testee.siteLiveData.value?.consentSelfTestFailed!!)
assertFalse(testee.siteLiveData.value?.consentCosmeticHide!!)
+ assertNull(testee.siteLiveData.value?.consentRule)
+ assertFalse(testee.siteLiveData.value?.consentReloadLoop!!)
}
@Test
diff --git a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
--- a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
+++ b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
@@ -44,46 +44,60 @@
fun updateUrl(webView: WebView, url: String) {
val newUri = url.toUri()
- val state = tabStates.getOrPut(webView) { TabState() }
+ val state = synchronized(tabStates) {
+ tabStates.getOrPut(webView) { TabState() }
+ }
val oldUri = state.lastUrl
if (oldUri != null && !isSamePageUrl(oldUri, newUri)) {
logcat { "URL changed from $oldUri to $newUri, clearing reload loop state" }
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
+ synchronized(tabStates) {
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ }
}
- state.lastUrl = newUri
+ synchronized(tabStates) {
+ state.lastUrl = newUri
+ }
}
fun detectReloadLoop(webView: WebView, cmp: String) {
- val state = tabStates[webView] ?: return
- if (!state.reloadLoopDetected && state.lastHandledCMP == cmp) {
- logcat { "Reload loop detected: $cmp on ${state.lastUrl}" }
- state.reloadLoopDetected = true
- autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY)
+ synchronized(tabStates) {
+ val state = tabStates[webView] ?: return
+ if (!state.reloadLoopDetected && state.lastHandledCMP == cmp) {
+ logcat { "Reload loop detected: $cmp on ${state.lastUrl}" }
+ state.reloadLoopDetected = true
+ autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY)
+ }
}
}
fun rememberLastHandledCMP(webView: WebView, cmp: String, isCosmetic: Boolean) {
- val state = tabStates.getOrPut(webView) { TabState() }
- if (isCosmetic) {
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
- return
+ synchronized(tabStates) {
+ val state = tabStates.getOrPut(webView) { TabState() }
+ if (isCosmetic) {
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ return
+ }
+ if (state.lastHandledCMP != cmp) {
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ }
+ state.lastHandledCMP = cmp
}
- if (state.lastHandledCMP != cmp) {
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
- }
- state.lastHandledCMP = cmp
}
fun isReloadLoopDetected(webView: WebView): Boolean {
- return tabStates[webView]?.reloadLoopDetected == true
+ synchronized(tabStates) {
+ return tabStates[webView]?.reloadLoopDetected == true
+ }
}
fun getLastHandledCMP(webView: WebView): String? {
- return tabStates[webView]?.lastHandledCMP
+ synchronized(tabStates) {
+ return tabStates[webView]?.lastHandledCMP
+ }
}
private fun isSamePageUrl(a: Uri, b: Uri): Boolean {| isCosmetic = true, | ||
| consentRule = null, | ||
| consentReloadLoop = false, | ||
| ) |
There was a problem hiding this comment.
New consent fields may not reset on page change
Medium Severity
The test whenOnPageChangeThenAutoconsentReset passes consentRule = null and consentReloadLoop = false (default values) to onAutoconsentResultReceived, then verifies the reset after onWebViewRefreshed() — but only for the old fields. Because the new fields start at their defaults, the test can never detect if they fail to reset. A grep shows site?.consentManaged = false exists at a separate reset location (line ~2420) in BrowserTabViewModel, but no corresponding reset for consentRule or consentReloadLoop. If there's an explicit field-by-field reset path, stale consentRule and consentReloadLoop values would persist after a page refresh.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| ) | ||
|
|
||
| private val tabStates: MutableMap<WebView, TabState> = | ||
| Collections.synchronizedMap(WeakHashMap()) |
There was a problem hiding this comment.
Non-atomic getOrPut on synchronized map causes race
Low Severity
AutoconsentReloadLoopDetector is a singleton using Collections.synchronizedMap(WeakHashMap()), but getOrPut (called in updateUrl and rememberLastHandledCMP) is not atomic — it performs a separate get then put. Since InitMessageHandlerPlugin runs on the IO dispatcher while other handlers may run on a different thread, concurrent access for the same WebView key could create duplicate TabState objects, with one silently overwriting the other's state.
Please tell me if this was useful or not with a 👍 or 👎.
joshliebe
left a comment
joshliebe
left a comment
There was a problem hiding this comment.
I tested it against privacy-test-pages and seems to work as expected but this touches APIs that will require an API proposal in order to be merged.
Also needs PixelDefinition for the new pixel.
…consent/impl/AutoconsentReloadLoopDetectorTest.kt Co-authored-by: Josh Leibstein <[email protected]>
…consent/impl/AutoconsentReloadLoopDetector.kt Co-authored-by: Josh Leibstein <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Reload loop detection unreachable when autoconsent is active
- Moved detectReloadLoop call before the userSetting guard so it executes when autoconsent is active and processing popups.
- ✅ Fixed: Detector state accessed from multiple threads unsafely
- Added synchronized blocks around all TabState field access to prevent data races between IO dispatcher and JS interface threads.
Or push these changes by commenting:
@cursor push 5ed9758922
Preview (5ed9758922)
diff --git a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
--- a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
+++ b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/AutoconsentReloadLoopDetector.kt
@@ -45,45 +45,57 @@
fun updateUrl(webView: WebView, url: String) {
val newUri = url.toUri()
val state = tabStates.getOrPut(webView) { TabState() }
- val oldUri = state.lastUrl
+ synchronized(state) {
+ val oldUri = state.lastUrl
- if (oldUri != null && !isSamePageUrl(oldUri, newUri)) {
- logcat { "URL changed from $oldUri to $newUri, clearing reload loop state" }
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
+ if (oldUri != null && !isSamePageUrl(oldUri, newUri)) {
+ logcat { "URL changed from $oldUri to $newUri, clearing reload loop state" }
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ }
+ state.lastUrl = newUri
}
- state.lastUrl = newUri
}
fun detectReloadLoop(webView: WebView, cmp: String) {
val state = tabStates[webView] ?: return
- if (!state.reloadLoopDetected && state.lastHandledCMP == cmp) {
- logcat { "Reload loop detected: $cmp on ${state.lastUrl}" }
- state.reloadLoopDetected = true
- autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY)
+ synchronized(state) {
+ if (!state.reloadLoopDetected && state.lastHandledCMP == cmp) {
+ logcat { "Reload loop detected: $cmp on ${state.lastUrl}" }
+ state.reloadLoopDetected = true
+ autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_ERROR_RELOAD_LOOP_DAILY)
+ }
}
}
fun rememberLastHandledCMP(webView: WebView, cmp: String, isCosmetic: Boolean) {
val state = tabStates.getOrPut(webView) { TabState() }
- if (isCosmetic) {
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
- return
+ synchronized(state) {
+ if (isCosmetic) {
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ return
+ }
+ if (state.lastHandledCMP != cmp) {
+ state.lastHandledCMP = null
+ state.reloadLoopDetected = false
+ }
+ state.lastHandledCMP = cmp
}
- if (state.lastHandledCMP != cmp) {
- state.lastHandledCMP = null
- state.reloadLoopDetected = false
- }
- state.lastHandledCMP = cmp
}
fun isReloadLoopDetected(webView: WebView): Boolean {
- return tabStates[webView]?.reloadLoopDetected == true
+ val state = tabStates[webView] ?: return false
+ synchronized(state) {
+ return state.reloadLoopDetected
+ }
}
fun getLastHandledCMP(webView: WebView): String? {
- return tabStates[webView]?.lastHandledCMP
+ val state = tabStates[webView] ?: return null
+ synchronized(state) {
+ return state.lastHandledCMP
+ }
}
private fun isSamePageUrl(a: Uri, b: Uri): Boolean {
diff --git a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/handlers/PopUpFoundMessageHandlerPlugin.kt b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/handlers/PopUpFoundMessageHandlerPlugin.kt
--- a/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/handlers/PopUpFoundMessageHandlerPlugin.kt
+++ b/autoconsent/autoconsent-impl/src/main/java/com/duckduckgo/autoconsent/impl/handlers/PopUpFoundMessageHandlerPlugin.kt
@@ -46,10 +46,10 @@
autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_POPUP_FOUND_DAILY)
val message: PopUpFoundMessage = parseMessage(jsonString) ?: return
- if (repository.userSetting) return
if (message.cmp.endsWith(IGNORE_CMP_SUFFIX, ignoreCase = true)) return
reloadLoopDetector.detectReloadLoop(webView, message.cmp)
+ if (repository.userSetting) return
autoconsentCallback.onFirstPopUpHandled()
}
} catch (e: Exception) {| val message: PopUpFoundMessage = parseMessage(jsonString) ?: return | ||
| if (repository.userSetting) return | ||
| if (message.cmp.endsWith(IGNORE_CMP_SUFFIX, ignoreCase = true)) return | ||
| reloadLoopDetector.detectReloadLoop(webView, message.cmp) |
There was a problem hiding this comment.
Reload loop detection unreachable when autoconsent is active
High Severity
detectReloadLoop is placed after the if (repository.userSetting) return guard, making it unreachable when autoconsent is active. In InitMessageHandlerPlugin, isAutoconsentDisabled = !settingsRepository.userSetting means autoconsent sends config when userSetting = true. But in PopUpFoundMessageHandlerPlugin, the same userSetting = true triggers early return before detectReloadLoop. Since reload loops only occur when autoconsent is actively processing popups, the entire reload loop prevention feature cannot trigger in production.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| ) | ||
|
|
||
| private val tabStates: MutableMap<WebView, TabState> = | ||
| Collections.synchronizedMap(WeakHashMap()) |
There was a problem hiding this comment.
Detector state accessed from multiple threads unsafely
Medium Severity
updateUrl is called from dispatcherProvider.io() in InitMessageHandlerPlugin, while detectReloadLoop and rememberLastHandledCMP run synchronously on the @JavascriptInterface callback thread. TabState fields are plain var with no volatile or synchronized access, creating data races. Collections.synchronizedMap only protects map operations, not compound read-modify-write on TabState fields. Notably, the PR reviewer's suggestion to drop synchronizedMap would worsen this since even basic map-level thread safety would be lost.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
3 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1201844467387842/task/1212270119348248?focus=true
Description
Steps to test this PR
UI changes
Note
Medium Risk
Changes core Cookie Prompt Management (autoconsent) message handling and initialization behavior (including disabling auto-actions during suspected reload loops), which could affect consent flows on some sites. Also expands breakage reporting payloads; risk is moderate and mainly behavioral/telemetry-oriented.
Overview
Cookie Prompt Management (CPM) now tracks reload loops per tab via a new
AutoconsentReloadLoopDetector, fires a newAUTOCONSENT_ERROR_RELOAD_LOOP_DAILYpixel when detected, and disablesautoActionduring a detected loop to prevent repeated reload/opt-out attempts.Autoconsent result plumbing is extended:
AutoconsentCallback.onResultReceivedandSite/SiteMonitorcarryconsentRuleandconsentReloadLoop, and the privacy dashboard/broken-site navigation include these new fields.Broken-site reporting payloads are expanded to send
consentRuleandconsentReloadLoopas additional parameters in breakage pixels/reports, andinitconfig gains anenableHeuristicActionflag controlled by a newAutoconsentFeature.heuristicAction()remote toggle (default off).Written by Cursor Bugbot for commit 82af983. This will update automatically on new commits. Configure here.