CPM: on-device heuristics and reload loop prevention#7862
Merged
Conversation
muodov
changed the title
muodov
force-pushed
the
max/cpm-heuristic-reload-loop
branch
from
8280ecc to
77a0f24
Compare
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
joshliebe
reviewed
Mar 5, 2026
…oop=true paths - whenUserSettingIsTrueThenDetectReloadLoopShouldNotBeCalled (FAILS) - whenCmpEndsWithTopSuffixThenDetectReloadLoopShouldNotBeCalled (FAILS) - whenReloadLoopDetectedThenAutoconsentDoneResultHasReloadLoopTrue - whenReloadLoopDetectedThenSelfTestResultHasReloadLoopTrue Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
aitorvs
reviewed
Mar 5, 2026
Collaborator
|
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}
Contributor
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 {
joshliebe
requested changes
Mar 5, 2026
Contributor
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 <joshliebe@gmail.com>
…consent/impl/AutoconsentReloadLoopDetector.kt Co-authored-by: Josh Leibstein <joshliebe@gmail.com>
Contributor
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: 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) {
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Reload loop detection runs before guard checks
- Moved the
reloadLoopDetector.detectReloadLoop()call after both theuserSettingandIGNORE_CMP_SUFFIXearly-return guards so it only runs when autoconsent is active and the CMP is not a-topframe rule.
- Moved the
- ✅ Fixed: Unsynchronized WeakHashMap accessed from multiple threads
- Wrapped the
WeakHashMapwithCollections.synchronizedMap()to preventConcurrentModificationExceptionfrom concurrent access across the IO dispatcher and WebView JS bridge threads.
- Wrapped the
Or push these changes by commenting:
@cursor push 808a0ea7ca
Preview (808a0ea7ca)
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
@@ -24,6 +24,7 @@
import com.duckduckgo.di.scopes.AppScope
import dagger.SingleInstanceIn
import logcat.logcat
+import java.util.Collections
import java.util.WeakHashMap
import javax.inject.Inject
@@ -38,7 +39,7 @@
var reloadLoopDetected: Boolean = false,
)
- private val tabStates: MutableMap<WebView, TabState> = WeakHashMap()
+ private val tabStates: MutableMap<WebView, TabState> = Collections.synchronizedMap(WeakHashMap())
fun updateUrl(webView: WebView, url: String) {
val newUri = url.toUri()
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,13 +46,13 @@
autoconsentPixelManager.fireDailyPixel(AutoConsentPixel.AUTOCONSENT_POPUP_FOUND_DAILY)
val message: PopUpFoundMessage = parseMessage(jsonString) ?: return
- reloadLoopDetector.detectReloadLoop(webView, message.cmp)
// Opt-in flow below, if user setting is true, do nothing
if (repository.userSetting) return
// for complex multi-frame CMPs, trigger opt-in flow only on -frame rules
if (message.cmp.endsWith(IGNORE_CMP_SUFFIX, ignoreCase = true)) return
+ reloadLoopDetector.detectReloadLoop(webView, message.cmp)
autoconsentCallback.onFirstPopUpHandled()
}
} catch (e: Exception) {
Contributor
|
Privacy Review task: https://app.asana.com/0/69071770703008/1213805139878106 |
joshliebe
approved these changes
Mar 25, 2026
Contributor
joshliebe
left a comment
joshliebe
left a comment
There was a problem hiding this comment.
Tested, works as specified 👍
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
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 CPM/autoconsent flow and telemetry by adding reload-loop detection that can disable auto-actions, plus new fields propagated into broken-site reporting; mis-detection could reduce consent handling or alter reported metrics.
Overview
Adds CPM on-device heuristic support behind a new
heuristicActionremote-config toggle, and introduces a per-WebViewAutoconsentReloadLoopDetectorto detect repeated CMP handling on the same page and disableautoActionwhen a reload loop is flagged.Refactors the
AutoconsentCallback.onResultReceivedAPI to pass a structuredAutoconsentResult(includingconsentRuleandconsentReloadLoop) and propagates these new fields throughSite, the privacy dashboard CPM state, and broken-site reporting/pixels (plus new autoconsent pixel definitions for heuristic success and reload-loop errors).Written by Cursor Bugbot for commit 8f81cbb. This will update automatically on new commits. Configure here.