fix(favicon): eliminate duplicate downloads and file handle race conditions

This commit fixes three critical issues causing favicon download spam
and file handle errors:

1. Duplicate Handler Registration (internal/services/browser.go)
   - Removed duplicate RegisterFaviconURIChangedHandler from AttachWebView()
   - Handler already registered in buildPane() for all WebViews
   - IMPACT: Reduces 6 downloads to 3 for a page with 3 favicons

2. File Handle Race Conditions (internal/cache/favicon.go)
   - Added sync.Mutex to FaviconCache to serialize downloads
   - Implemented double-check locking pattern after mutex acquisition
   - IMPACT: Prevents concurrent writes to same temp files

3. Double-Close Bug (internal/cache/favicon.go:savePNG)
   - Fixed double-close via explicit close + deferred close
   - Added 'closed' flag to track file state
   - Made temp file cleanup safer with existence check
   - IMPACT: Eliminates "file already closed" errors

Root Cause Analysis:
- Pages with multiple favicons (16x16, 32x32, favicon.ico) triggered
  multiple favicon-changed signals from WebKit
- Duplicate handlers caused each signal to spawn 2 downloads
- No concurrency protection led to race conditions on temp files
- Result: 100+ log messages and file handle errors

Performance: Global mutex is acceptable as favicons are cached for
7 days and downloads are infrequent.

commit: 0e199d79e8b38fa6a8331fd7338b34c523ba73e5
author: bamen · 12mo ago · 2025-10-15 05:23 UTC
parent: 2db2e40
signature: unsigned

2 files · +32 -11

M internal/cache/favicon.go +27 -7


@@ -14,6 +14,7 @@ import (
14	14		"os"
15	15		"path/filepath"
16	16		"strings"
	17	+	"sync"
17	18		"time"
18	19
19	20		"github.com/bnema/dumber/internal/config"

@@ -22,7 +23,8 @@ import (
22	23
23	24		// FaviconCache manages favicon caching to local files
24	25		type FaviconCache struct {
25		-	cacheDir string
	26	+	cacheDir string
	27	+	downloadMutex sync.Mutex // Prevents concurrent downloads of the same favicon
26	28		}
27	29
28	30		// NewFaviconCache creates a new favicon cache

@@ -123,7 +125,13 @@ func (fc *FaviconCache) downloadFavicon(faviconURL string) error {
123	125		filename := fc.getFilename(faviconURL)
124	126		cachedPath := filepath.Join(fc.cacheDir, filename)
125	127
126		-	// Don't download if already exists and is recent
	128	+	// Lock to prevent concurrent downloads of the same favicon
	129	+	// This prevents race conditions on temp files and duplicate downloads
	130	+	fc.downloadMutex.Lock()
	131	+	defer fc.downloadMutex.Unlock()
	132	+
	133	+	// Double-check if already exists after acquiring lock
	134	+	// Another goroutine might have downloaded it while we were waiting
127	135		if info, err := os.Stat(cachedPath); err == nil {
128	136		if time.Since(info.ModTime()) < 724time.Hour {
129	137		return nil

@@ -175,8 +183,11 @@ func (fc *FaviconCache) downloadFavicon(faviconURL string) error {
175	183		}
176	184		}()
177	185		defer func() {
178		-	if err := os.Remove(tempDownloadFile); err != nil {
179		-	log.Printf("[favicon] failed to remove temp download file: %v", err)
	186	+	// Clean up temp download file if it still exists
	187	+	if _, err := os.Stat(tempDownloadFile); err == nil {
	188	+	if err := os.Remove(tempDownloadFile); err != nil {
	189	+	log.Printf("[favicon] failed to remove temp download file: %v", err)
	190	+	}
180	191		}
181	192		}()
182	193

@@ -235,9 +246,14 @@ func (fc *FaviconCache) savePNG(img image.Image, outputPath string) error {
235	246		if err != nil {
236	247		return fmt.Errorf("failed to create temp file: %w", err)
237	248		}
	249	+
	250	+	// Track if we've already closed the file
	251	+	closed := false
238	252		defer func() {
239		-	if err := file.Close(); err != nil {
240		-	log.Printf("[favicon] failed to close file: %v", err)
	253	+	if !closed {
	254	+	if err := file.Close(); err != nil {
	255	+	log.Printf("[favicon] failed to close file: %v", err)
	256	+	}
241	257		}
242	258		}()
243	259

@@ -248,7 +264,11 @@ func (fc *FaviconCache) savePNG(img image.Image, outputPath string) error {
248	264		return fmt.Errorf("failed to encode PNG: %w", err)
249	265		}
250	266
251		-	file.Close()
	267	+	// Close the file before renaming (required on Windows, good practice on all platforms)
	268	+	if err := file.Close(); err != nil {
	269	+	return fmt.Errorf("failed to close file before rename: %w", err)
	270	+	}
	271	+	closed = true
252	272
253	273		// Atomic move
254	274		if err := os.Rename(tempFile, outputPath); err != nil {

 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"time"
 	"github.com/bnema/dumber/internal/config"
 // FaviconCache manages favicon caching to local files
 type FaviconCache struct {
-	cacheDir string
+	cacheDir      string
+	downloadMutex sync.Mutex // Prevents concurrent downloads of the same favicon
 }
 // NewFaviconCache creates a new favicon cache
 	filename := fc.getFilename(faviconURL)
 	cachedPath := filepath.Join(fc.cacheDir, filename)
-	// Don't download if already exists and is recent
+	// Lock to prevent concurrent downloads of the same favicon
+	// This prevents race conditions on temp files and duplicate downloads
+	fc.downloadMutex.Lock()
+	defer fc.downloadMutex.Unlock()
+	// Double-check if already exists after acquiring lock
+	// Another goroutine might have downloaded it while we were waiting
 	if info, err := os.Stat(cachedPath); err == nil {
 		if time.Since(info.ModTime()) < 7*24*time.Hour {
 			return nil
 		}
 	}()
 	defer func() {
-		if err := os.Remove(tempDownloadFile); err != nil {
+		// Clean up temp download file if it still exists
-			log.Printf("[favicon] failed to remove temp download file: %v", err)
+		if _, err := os.Stat(tempDownloadFile); err == nil {
+			if err := os.Remove(tempDownloadFile); err != nil {
+				log.Printf("[favicon] failed to remove temp download file: %v", err)
+			}
 		}
 	}()
 	if err != nil {
 		return fmt.Errorf("failed to create temp file: %w", err)
 	}
+	// Track if we've already closed the file
+	closed := false
 	defer func() {
-		if err := file.Close(); err != nil {
+		if !closed {
-			log.Printf("[favicon] failed to close file: %v", err)
+			if err := file.Close(); err != nil {
+				log.Printf("[favicon] failed to close file: %v", err)
+			}
 		}
 	}()
 		return fmt.Errorf("failed to encode PNG: %w", err)
 	}
-	file.Close()
+	// Close the file before renaming (required on Windows, good practice on all platforms)
+	if err := file.Close(); err != nil {
+		return fmt.Errorf("failed to close file before rename: %w", err)
+	}
+	closed = true
 	// Atomic move
 	if err := os.Rename(tempFile, outputPath); err != nil {

M internal/services/browser.go +5 -4


@@ -93,6 +93,8 @@ func (s BrowserService) AttachWebView(view webkit.WebView) {
93	93		s.webView = view
94	94
95	95		// Register legacy favicon change handler (binary data - fallback)
	96	+	// This is kept for backward compatibility but is rarely used
	97	+	// Modern favicon handling uses the URI-based handler registered in buildPane()
96	98		view.RegisterFaviconChangedHandler(func(data []byte) {
97	99		if currentURL := view.GetCurrentURL(); currentURL != "" {
98	100		log.Printf("[favicon] Favicon detected for %s, data size: %d bytes", currentURL, len(data))

@@ -100,10 +102,9 @@ func (s BrowserService) AttachWebView(view webkit.WebView) {
100	102		}
101	103		})
102	104
103		-	// Register favicon URI change handler (WebKit native)
104		-	view.RegisterFaviconURIChangedHandler(func(pageURI, faviconURI string) {
105		-	s.handleFaviconURIChanged(pageURI, faviconURI)
106		-	})
	105	+	// Note: RegisterFaviconURIChangedHandler is now only called in buildPane()
	106	+	// to avoid duplicate handler registration which causes multiple downloads
	107	+	// of the same favicon and file handle race conditions
107	108		}
108	109
109	110		// LoadGUIBundle loads the unified GUI bundle from assets

 	s.webView = view
 	// Register legacy favicon change handler (binary data - fallback)
+	// This is kept for backward compatibility but is rarely used
+	// Modern favicon handling uses the URI-based handler registered in buildPane()
 	view.RegisterFaviconChangedHandler(func(data []byte) {
 		if currentURL := view.GetCurrentURL(); currentURL != "" {
 			log.Printf("[favicon] Favicon detected for %s, data size: %d bytes", currentURL, len(data))
 		}
 	})
-	// Register favicon URI change handler (WebKit native)
+	// Note: RegisterFaviconURIChangedHandler is now only called in buildPane()
-	view.RegisterFaviconURIChangedHandler(func(pageURI, faviconURI string) {
+	// to avoid duplicate handler registration which causes multiple downloads
-		s.handleFaviconURIChanged(pageURI, faviconURI)
+	// of the same favicon and file handle race conditions
-	})
 }
 // LoadGUIBundle loads the unified GUI bundle from assets