Fix BrowserBookmark '100% CPU' issue #3826
Conversation
# Fix BrowserBookmark plugin locking threads at 100% CPU
The following files were modified to implement this fix:
1. /Flow.Launcher.Plugin.BrowserBookmark.csproj
- packages SkiaSharp and Svg.Skia added to output WEBP
2. /Helper/FaviconHelper.cs
- new method, TryConvertToWebp, added to take any image data and encode to WEBP
3. /FirefoxBookmarkLoader.cs
- LoadFaviconsFromDb to use the new helper which ensures only safe WEBP files are used by the UI, which resolves the CPU-locking. It was GZIPped SVGs that were causing the thread / CPU lock.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough""" WalkthroughThe changes update the favicon extraction process for Firefox bookmarks by enhancing image data handling, introducing WebP conversion, and updating dependency packages. The code now attempts to convert favicon data to WebP, saves it with a unique filename, and removes previous SVG-specific logic. New dependencies for SVG and image processing are added. Changes
Sequence Diagram(s)sequenceDiagram
participant FirefoxBookmarkLoader
participant Database
participant FaviconHelper
participant FileSystem
FirefoxBookmarkLoader->>Database: Query for bookmark favicons (icon_id, data)
loop For each favicon result
FirefoxBookmarkLoader->>FaviconHelper: TryConvertToWebp(data)
alt Conversion succeeds
FaviconHelper-->>FirefoxBookmarkLoader: WebP image bytes
FirefoxBookmarkLoader->>FileSystem: Save WebP file (domain + icon_id)
FileSystem-->>FirefoxBookmarkLoader: File path
FirefoxBookmarkLoader->>Bookmark: Update FaviconPath
else Conversion fails
FaviconHelper-->>FirefoxBookmarkLoader: null
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (1)
70-123: Well-implemented WebP conversion with proper resource management.The method correctly handles both SVG and bitmap formats with appropriate fallback logic. The WebP quality of 65 provides a good balance between file size and quality for favicons.
Consider making the WebP quality configurable in the future to allow users to adjust the trade-off between file size and quality:
-public static byte[] TryConvertToWebp(byte[] data) +public static byte[] TryConvertToWebp(byte[] data, int quality = 65)And update line 110:
-using (var webp = image.Encode(SKEncodedImageFormat.Webp, 65)) +using (var webp = image.Encode(SKEncodedImageFormat.Webp, quality))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (5)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2442
File: Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj:4-4
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In .NET 8, SourceLink is included by default, eliminating the need for the `Microsoft.SourceLink.GitHub` package.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2442
File: Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj:4-4
Timestamp: 2024-08-31T20:42:08.482Z
Learning: In .NET 8, SourceLink is included by default, eliminating the need for the `Microsoft.SourceLink.GitHub` package.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (4)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🧬 Code Graph Analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (3)
FaviconHelper(8-124)TryConvertToWebp(70-123)SaveBitmapData(58-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
100-101: LGTM! Package dependencies align with PR objectives.The addition of
Svg.SkiaandSkiaSharppackages is appropriate for implementing the WebP conversion functionality to fix the CPU usage issue.Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
166-187: Excellent implementation of gzip decompression and WebP conversion.The code properly detects gzip-compressed data using magic bytes and handles decompression correctly. The WebP conversion integration with unique file naming prevents collisions and the ConcurrentDictionary ensures thread-safe duplicate prevention.
|
Nevermind I saw the previous PR. |
|
@taooceros - yes, something to do with SVG loading - not sure if it was GZIPped or malformed SVGs causing it. Basically, Firefox stores favicons as the website supplies them, whereas Chrome/ium converts to bitmap so we can rely on it. So, for each bad SVG a thread would get locked up trying to understand it. Are we waiting for review on this PR? I will rework the original PR once FL2.0 drops. It turned in to a total refactor and caused some head scratching which I want to work through. As I said on the original PR, this will solve / improve many things over the original plugin - e.g. intelligent refresh of the bookmarks / icons which won't rely on restarting the app. |
|
🥷 Code experts: Jack251970 Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (1)
77-94: Consider improving exception handling and adding dimension validation.The SVG processing logic is correct, but there are opportunities for improvement:
- The empty catch block could hide useful debugging information for malformed SVGs
- No validation for SVG dimensions that could be zero or negative
Consider this improvement:
try { using (var ms = new MemoryStream(data)) { var svg = new SKSvg(); if (svg.Load(ms) != null && svg.Picture != null) { - bitmap = new SKBitmap((int)svg.Picture.CullRect.Width, (int)svg.Picture.CullRect.Height); + var width = Math.Max(1, (int)svg.Picture.CullRect.Width); + var height = Math.Max(1, (int)svg.Picture.CullRect.Height); + bitmap = new SKBitmap(width, height); using (var canvas = new SKCanvas(bitmap)) { canvas.Clear(SKColors.Transparent); canvas.DrawPicture(svg.Picture); canvas.Flush(); } } } } -catch { /* Not an SVG */ } +catch (Exception ex) +{ + // Log SVG processing failures for debugging, but continue with bitmap fallback + Main.Context.API.LogDebug(ClassName, $"SVG processing failed: {ex.Message}"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (5)
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
PR: #3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Yusyuriv
PR: #3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in JsonRPCPluginSettings.cs), path validation is enabled by default in OpenFileDialog and FolderBrowserDialog, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs
[warning] 3-3:
Skia is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (4)
3-4: LGTM! Necessary imports for WebP conversion functionality.The new SkiaSharp and Svg.Skia imports are required for the WebP conversion logic and SVG processing capabilities.
72-73: LGTM! Proper null and empty data validation.The early return for null or empty data prevents unnecessary processing and potential exceptions.
96-123: LGTM! Robust fallback and encoding logic addresses the CPU issue.The implementation effectively handles the core problem:
- Bitmap fallback: Properly handles non-SVG images using SkiaSharp's universal decoder
- WebP encoding: Quality setting of 65 provides good balance between size and quality
- Resource management: Proper disposal in finally block prevents memory leaks
- Graceful failure: Returns null instead of throwing exceptions
This approach should resolve the 100% CPU issue by converting problematic SVG data to safe WebP format before UI consumption.
70-123: Excellent solution for the CPU locking issue.This implementation effectively addresses the core problem described in the PR objectives:
Problem solved: Malformed or GZIP-compressed SVGs from Firefox favicons were causing threads to lock at 100% CPU usage.
Solution benefits:
- Safe conversion: All favicon data is converted to WebP format before UI consumption
- Robust handling: Gracefully processes both SVG and bitmap data with appropriate fallbacks
- Performance: WebP format reduces memory usage and eliminates SVG parsing overhead in the UI
- Stability: Exception handling prevents crashes while maintaining functionality
This approach aligns perfectly with the interim fix strategy mentioned in the PR discussion, providing immediate stability improvements before the planned 2.0 refactor.
Fix BrowserBookmark plugin locking threads at 100% CPU
The following files were modified to implement this fix:
/Flow.Launcher.Plugin.BrowserBookmark.csproj
/Helper/FaviconHelper.cs
/FirefoxBookmarkLoader.cs
Fixes #3748