Skip to content

Fix FileBasedFaviconPersister FileAlreadyExistsException#8030

Merged
joshliebe merged 1 commit intodevelopfrom
feature/josh/fix-favicon-crash
Mar 20, 2026
Merged

Fix FileBasedFaviconPersister FileAlreadyExistsException#8030
joshliebe merged 1 commit intodevelopfrom
feature/josh/fix-favicon-crash

Conversation

@joshliebe
Copy link
Contributor

@joshliebe joshliebe commented Mar 19, 2026

Task/Issue URL: https://app.asana.com/1/137249556945/project/1200204095367872/task/1213736584422142?focus=true

Description

  • Catches FileAlreadyExistsException if copyTo throws

Steps to test this PR

  • Verify that favicons work correctly

Note

Low Risk
Low risk: wraps a single file copy operation to avoid crashing when copy fails (e.g., existing destination/race), with no behavioral changes elsewhere.

Overview
Prevents crashes when persisting favicons by wrapping FileBasedFaviconPersister.copyToDirectory’s file.copyTo(...) call in runCatching, effectively swallowing FileAlreadyExistsException/IO failures during the copy.

Written by Cursor Bugbot for commit 2789592. This will update automatically on new commits. Configure here.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Silent exception swallowing masks all copy failures
    • Replaced broad runCatching with explicit exception handling that logs copy failures and only catches Exception instead of all Throwable.

Create PR

Or push these changes by commenting:

@cursor push a8b051c7d9
Preview (a8b051c7d9)
diff --git a/app/src/main/java/com/duckduckgo/app/browser/favicon/FaviconPersister.kt b/app/src/main/java/com/duckduckgo/app/browser/favicon/FaviconPersister.kt
--- a/app/src/main/java/com/duckduckgo/app/browser/favicon/FaviconPersister.kt
+++ b/app/src/main/java/com/duckduckgo/app/browser/favicon/FaviconPersister.kt
@@ -27,6 +27,7 @@
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
+import logcat.LogPriority.ERROR
 import logcat.LogPriority.INFO
 import logcat.logcat
 import java.io.File
@@ -96,7 +97,17 @@
     ) {
         withContext(dispatcherProvider.io()) {
             val persistedFile = fileForFavicon(directory, newSubfolder, newFilename)
-            runCatching { file.copyTo(persistedFile, overwrite = true) }
+            try {
+                file.copyTo(persistedFile, overwrite = true)
+            } catch (exception: FileAlreadyExistsException) {
+                logcat(INFO, exception) {
+                    "Favicon destination already exists and could not be replaced: ${persistedFile.absolutePath}"
+                }
+            } catch (exception: Exception) {
+                logcat(ERROR, exception) {
+                    "Failed to persist favicon to ${persistedFile.absolutePath}"
+                }
+            }
         }
     }

@joshliebe joshliebe merged commit e7bc5a3 into develop Mar 20, 2026
18 checks passed
@joshliebe joshliebe deleted the feature/josh/fix-favicon-crash branch March 20, 2026 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants