FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Javascript Binding - Store JavascriptObjects per CefBrowser by amaitland · Pull Request #4475 · cefsharp/CefSharp · GitHub

/ CefSharp Public

Javascript Binding - Store JavascriptObjects per CefBrowser - #4475

Open
amaitland wants to merge 6 commits into
masterfrom
jsb/perbrowsercache
ConversationCommits (6)ChecksFiles changed
Open

Javascript Binding - Store JavascriptObjects per CefBrowser#4475
amaitland wants to merge 6 commits into
masterfrom
jsb/perbrowsercache

Conversation

amaitland commented Apr 18, 2023 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Fixes: #2306

Summary:

  • Historically JSB objects are cached per process, for shared render processes this can be problematic
    This optionally allows for a new Per Browser cache.

Use the following command line arg to opt into this behaviour.

--jsb-cache-perbrowser

// CefSettings
settings.CefCommandLineArgs.Add("jsb-cache-perbrowser");

Changes:

  • Add cache abstraction to allow for legacy and new per browser configuration
  • Add new CefSharp specific command line arg to optionally enable new code path
  • Settings haven't been moved to per Browser as that requires additional code changes to pass in extraInfo to the Popups

How Has This Been Tested?

  • New tests added

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

Summary by CodeRabbit

  • New Features

    • Added configurable JavaScript object caching with per-browser and legacy (process-wide) modes and a new command-line option to enable per-browser caching.

  • Bug Fixes

    • Centralized cache management for JS-bound objects and improved cleanup when browsers are destroyed to reduce cross-browser leakage.

  • Tests

    • Added tests covering legacy and per-browser cache behavior, isolation, update/replace semantics, and cache clearing.

  • Documentation

    • Public API signatures and interfaces updated to expose the new caching options.

amaitland linked an issue Apr 18, 2023 that may be closed by this pull request
Javascript Binding - Store JavascriptObjects/Settings per CefBrowser #2306
Open

AppVeyorBot commented Apr 18, 2023

Copy link
Copy Markdown

✅ Build CefSharp 112.2.90-CI4758 completed (commit 91bd2be36a by @amaitland)

campersau reviewed Apr 18, 2023
View reviewed changes
Comment thread CefSharp/Internals/LegacyJavaScriptObjectCache.cs Outdated
Comment thread CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs Outdated

AppVeyorBot commented Apr 21, 2023

Copy link
Copy Markdown

✅ Build CefSharp 112.2.100-CI4779 completed (commit 04fe8b2185 by @amaitland)

amaitland force-pushed the jsb/perbrowsercache branch from f18c0ff to 8eb2376 Compare August 5, 2023 20:06

AppVeyorBot commented Aug 5, 2023

Copy link
Copy Markdown

✅ Build CefSharp 115.3.110-CI4829 completed (commit c19986019f by @amaitland)

amaitland mentioned this pull request Jan 15, 2025
Closed

amaitland commented Jan 25, 2025

Copy link
Copy Markdown
Member Author

This is required for #5001

I originally added a command line arg, I think this should be plan b as it is hard to test. If we can have it configured per ChromiumWebBrowser instance that would make life a lot easier to test.

amaitland force-pushed the jsb/perbrowsercache branch from 8eb2376 to fcc0cdb Compare January 31, 2025 20:50

AppVeyorBot commented Jan 31, 2025

Copy link
Copy Markdown

✅ Build CefSharp 132.3.10-CI5157 completed (commit 3c5f7865ac by @amaitland)

amaitland commented Feb 1, 2025

Copy link
Copy Markdown
Member Author

I originally added a command line arg, I think this should be plan b as it is hard to test. If we can have it configured per ChromiumWebBrowser instance that would make life a lot easier to test.

This would require a totally different approach.

amaitland force-pushed the jsb/perbrowsercache branch from fcc0cdb to 1fed9c9 Compare May 10, 2025 23:14

coderabbitai Bot commented May 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

Introduces an IJavaScriptObjectCache abstraction with two implementations (legacy process-wide and per-browser), updates render-process code to use the cache, adds a command-line flag to select per-browser behavior, and includes unit tests for both cache implementations.

Changes

Cohort / File(s) Change Summary
Cache abstraction & implementations
CefSharp/Internals/IJavaScriptObjectCache.cs, CefSharp/Internals/LegacyJavaScriptObjectCache.cs, CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs		 Added IJavaScriptObjectCache interface and two implementations: LegacyJavaScriptObjectCache (process-wide, ClearCache no-op) and PerBrowserJavaScriptObjectCache (per-browser dictionaries, supports ClearCache).
Argument flag
CefSharp/Internals/CefSharpArguments.cs		 Added constant PerBrowserJavaScriptObjectCache = "--jsb-cache-perbrowser".
Render-process wrapper & wiring
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h, CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp, CefSharp.BrowserSubprocess.Core/SubProcess.h		 Replaced direct dictionary usage with an IJavaScriptObjectCache member, added jsbCachePerBrowser constructor flag and conditional instantiation of the appropriate cache, updated code paths to call InsertOrUpdate, GetCacheValues, GetCache, and ClearCache.
Handler signatures
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h, CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h		 Changed member and constructor parameter types from concrete Dictionary<String^, JavascriptObject^>^ to IDictionary<String^, JavascriptObject^>^.
Tests
CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs, CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs		 Added unit tests covering insertion/updating, retrieval, clearing behavior, per-browser isolation, and edge cases for both cache implementations.

Sequence Diagram(s)

sequenceDiagram
    participant SubProcess
    participant CefAppUnmanagedWrapper
    participant IJavaScriptObjectCache
    participant Browser

    SubProcess->>CefAppUnmanagedWrapper: new(jsbCachePerBrowser, ...)
    CefAppUnmanagedWrapper->>IJavaScriptObjectCache: instantiate (Legacy or PerBrowser)
    Note right of IJavaScriptObjectCache: Cache lives in render process
    Browser->>CefAppUnmanagedWrapper: OnContextCreated(browserId)
    CefAppUnmanagedWrapper->>IJavaScriptObjectCache: GetCacheValues(browserId)
    IJavaScriptObjectCache-->>CefAppUnmanagedWrapper: collection<JavascriptObject>
    CefAppUnmanagedWrapper->>Browser: bind objects (create V8 bindings)
    Browser->>IJavaScriptObjectCache: InsertOrUpdate(browserId, jsObjects)
    Browser->>IJavaScriptObjectCache: ClearCache(browserId) (on destroy)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Suggested labels

breaking-change

Poem

"I stitched caches with hops and care,
Per-browser pockets everywhere.
Tests on my paws, flags in my clutch,
No more shared crumbs — a tidy touch.
🐇"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.71% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly and concisely summarizes the primary change, indicating that Javascript objects are now stored on a per-browser basis, which matches the pull request’s main objective of introducing per-browser caching.
Linked Issues Check ✅ Passed The changes fully implement the objectives of issue #2306 by introducing an IJavaScriptObjectCache abstraction, supplying both legacy and per-browser cache implementations, integrating a command-line flag to select per-browser caching, and replacing process-wide dictionaries with browser-scoped caches in the binding handlers.
Out of Scope Changes Check ✅ Passed All changes are focused on introducing and wiring up the per-browser caching abstraction for Javascript binding objects and related tests, with no unrelated or out-of-scope modifications present.
Description Check ✅ Passed The pull request description adheres to the repository’s template by including the Fixes reference, a clear summary of the change, a detailed list of modifications, testing information, types of changes, and a populated checklist, providing sufficient context for reviewers.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jsb/perbrowsercache

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

amaitland commented May 10, 2025

Copy link
Copy Markdown
Member Author

@coderabbitai review

coderabbitai Bot commented May 10, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

amaitland marked this pull request as ready for review May 10, 2025 23:53
coderabbitai Bot reviewed May 10, 2025
View reviewed changes

coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actionable comments posted: 2

🧹 Nitpick comments (5)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)

1-68: Well-implemented per-browser JavaScript object cache

This implementation correctly manages separate JavaScript object caches for each browser instance, addressing the issue with shared render processes. The code handles edge cases appropriately, such as creating new caches when needed and returning empty collections when a cache doesn't exist.

One minor consideration: GetCache will always create a new cache dictionary if one doesn't exist for the specified browser ID. This could potentially create unnecessary empty caches if the caller just wants to check for existence. However, this aligns with the interface contract and is a reasonable implementation decision.

CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (2)

109-110: ClearCache result is silently ignored – verify intention

_javascriptObjectCache->ClearCache(browser->GetIdentifier());

For the legacy cache implementation this is a no-op by design, while for the per-browser cache it frees memory.
A reader might assume the call always does something, so please:

  1. Add a short explanatory comment (// no-op for Legacy cache) to prevent future confusion.
  2. Optionally gate the call behind an if (_jsbCachePerBrowser) flag to save an unnecessary virtual dispatch.

628-629: Potential duplication of cached objects

InsertOrUpdate is invoked both here and in OnBrowserCreated. If the browser process re-sends the same objects (e.g. after a navigation) the cache might now hold duplicates unless the implementation performs a replace-by-name.

Confirm that:

  1. InsertOrUpdate overwrites existing entries that share the same key.
  2. Unit tests cover this message path (currently only constructor-time insertion is tested).
CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs (1)

102-116: Test encodes legacy “no-op” behaviour – document rationale

ClearCacheShouldDoNothing intentionally expects the cache to remain populated.
Add a comment explaining that the legacy cache is process-wide and intentionally retains objects to avoid accidental refactors flipping the expectation.

CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (1)

90-105: Missing negative check after overwrite

After overwriting the cache you assert only that the new value is present:

Assert.Equal(2, cachedObjects.First().Value);

To fully validate overwrite semantics, also assert that the previous value (1) no longer exists:

Assert.DoesNotContain(cachedObjects, o => o.Value == 1);

This prevents false positives where both objects remain in the list.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8fe8810 and 83b6747.

📒 Files selected for processing (11)
  • CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (5 hunks)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/SubProcess.h (1 hunks)
  • CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs (1 hunks)
  • CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (1 hunks)
  • CefSharp/Internals/CefSharpArguments.cs (1 hunks)
  • CefSharp/Internals/IJavaScriptObjectCache.cs (1 hunks)
  • CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1 hunks)
  • CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1 hunks)
🧰 Additional context used 🧬 Code Graph Analysis (7) CefSharp/Internals/CefSharpArguments.cs (1)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)
  • PerBrowserJavaScriptObjectCache (14-68)
CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h (3)
CefSharp/Internals/IJavaScriptObjectCache.cs (1)
  • IDictionary (25-25)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1)
  • IDictionary (40-43)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (2)
  • IDictionary (48-53)
  • IDictionary (55-67)
CefSharp/Internals/IJavaScriptObjectCache.cs (2)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (2)
  • ClearCache (19-22)
  • InsertOrUpdate (25-31)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (2)
  • ClearCache (20-23)
  • InsertOrUpdate (26-34)
CefSharp.BrowserSubprocess.Core/SubProcess.h (1)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)
  • PerBrowserJavaScriptObjectCache (14-68)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (2)
CefSharp/Internals/IJavaScriptObjectCache.cs (4)
  • ClearCache (18-18)
  • InsertOrUpdate (40-40)
  • ICollection (33-33)
  • IDictionary (25-25)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (4)
  • ClearCache (19-22)
  • InsertOrUpdate (25-31)
  • ICollection (34-37)
  • IDictionary (40-43)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (2)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)
  • PerBrowserJavaScriptObjectCache (14-68)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1)
  • LegacyJavaScriptObjectCache (13-44)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (2)
CefSharp/Internals/IJavaScriptObjectCache.cs (4)
  • ClearCache (18-18)
  • InsertOrUpdate (40-40)
  • ICollection (33-33)
  • IDictionary (25-25)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (5)
  • ClearCache (20-23)
  • InsertOrUpdate (26-34)
  • ICollection (37-45)
  • IDictionary (48-53)
  • IDictionary (55-67)
🔇 Additional comments (11)
CefSharp/Internals/CefSharpArguments.cs (1)

13-13: LGTM: Command-line argument for per-browser JavaScript object caching added.

The new constant follows the existing naming conventions and pattern in the class, providing a clear toggle to enable per-browser JavaScript object caching.

CefSharp.BrowserSubprocess.Core/SubProcess.h (2)

36-36: LGTM: Command-line flag parser for per-browser JavaScript object caching.

The implementation correctly uses the CommandLineArgsParser::HasArgument method to check for the new command-line argument, consistent with how other arguments are parsed in this class.

39-39: LGTM: Updated constructor call with new parameter.

The CefAppUnmanagedWrapper constructor is now called with the new jsbCachePerBrowser parameter, allowing the appropriate cache implementation to be selected at runtime.

CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h (1)

30-30: LGTM: Updated to use interface instead of concrete implementation.

Good change to use the more abstract IDictionary interface instead of the concrete Dictionary class. This modification supports the new cache abstraction by allowing different cache implementations to be injected, promoting loose coupling and better maintainability.

Also applies to: 33-33

CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (1)

28-28: LGTM: Updated to use interface instead of concrete implementation.

Good change to use the more abstract IDictionary interface instead of the concrete Dictionary class. This enables the class to work with different cache implementations (either legacy process-wide cache or per-browser cache) through dependency injection.

Also applies to: 32-32

CefSharp/Internals/IJavaScriptObjectCache.cs (1)

1-42: Well-designed interface for JavaScript object caching

This interface provides a clean and well-documented contract for managing JavaScript binding objects in the render process, with support for both per-process (legacy) and per-browser caching strategies. The method signatures are clear, focused, and appropriate for the operations being performed.

CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (2)

38-38: LGTM: Good abstraction for JavaScript object caching

The replacement of the concrete dictionary with an interface type provides better abstraction and flexibility.

45-64: LGTM: Proper initialization based on caching strategy

The constructor now correctly initializes the appropriate cache implementation based on the jsbCachePerBrowser flag. This allows for easy switching between caching strategies without modifying the rest of the code.

CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1)

1-44: LGTM: Legacy cache implementation maintains backward compatibility

This implementation correctly maintains the legacy behavior of caching JavaScript objects per process rather than per browser. The ClearCache method is appropriately implemented as a no-op, and the other methods ignore the browser ID parameter as expected.

CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)

133-139: Gracefully handle an empty or null value collection

GetCacheValues is assumed to return a non-null collection. If the implementation ever changes to return nullptr when the browserId is not present, the subsequent ->Count dereference will crash.

-auto values = _javascriptObjectCache->GetCacheValues(browser->GetIdentifier());
-if (values->Count > 0 && rootObject != nullptr)
+auto values = _javascriptObjectCache->GetCacheValues(browser->GetIdentifier());
+if (values != nullptr && values->Count > 0 && rootObject != nullptr)

Adding the extra guard costs nothing and improves robustness.

CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs (1)

15-26: Equality semantics might mask duplicate objects

Assert.Contains(javascriptObjects[0], cachedValues);

Contains relies on reference equality unless JavascriptObject overrides Equals/GetHashCode.
If equality is not overridden, a scenario where two distinct JavascriptObject instances share the same Name but both remain in the cache would not be detected by these tests.

Please either:

  1. Override equality on JavascriptObject to be value-based (preferred), or
  2. Assert against the cache dictionary keys instead of values.

This will make the test guard the contract more strictly.

Comment thread CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
Comment thread CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp

AppVeyorBot commented May 11, 2025

Copy link
Copy Markdown

✅ Build CefSharp 136.1.40-CI5239 completed (commit 344ec7fb5f by @amaitland)

amaitland force-pushed the jsb/perbrowsercache branch from 83b6747 to 1d19085 Compare May 16, 2025 22:22

AppVeyorBot commented May 16, 2025

Copy link
Copy Markdown

✅ Build CefSharp 136.1.40-CI5258 completed (commit df9b49b25a by @amaitland)

amaitland force-pushed the jsb/perbrowsercache branch from 1d19085 to 687ef7c Compare June 20, 2025 23:11

AppVeyorBot commented Jun 21, 2025

Copy link
Copy Markdown

✅ Build CefSharp 137.0.100-CI5283 completed (commit 55b8f61430 by @amaitland)

coderabbitai Bot mentioned this pull request Jul 23, 2025
Merged
9 tasks
amaitland force-pushed the jsb/perbrowsercache branch from 687ef7c to e064cee Compare October 11, 2025 01:39
coderabbitai Bot reviewed Oct 11, 2025
View reviewed changes

coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actionable comments posted: 0

🧹 Nitpick comments (2)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)

44-45: Return a shared empty collection instead of allocating.

When no cache exists we create a new List<JavascriptObject> each call. Returning Array.Empty<JavascriptObject>() avoids repeated allocations while still satisfying the contract.

Apply this diff:

-            return new List<JavascriptObject>();
+            return Array.Empty<JavascriptObject>();
CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (1)

26-40: Test name could be more precise.

The test name suggests it verifies both add and update behavior, but it only validates insertion of new objects. While update behavior is properly tested elsewhere (lines 87-105), consider renaming to InsertOrUpdateAddsObjectsToCache for clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 687ef7c and e064cee.

📒 Files selected for processing (11)
  • CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (5 hunks)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h (1 hunks)
  • CefSharp.BrowserSubprocess.Core/SubProcess.h (1 hunks)
  • CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs (1 hunks)
  • CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (1 hunks)
  • CefSharp/Internals/CefSharpArguments.cs (1 hunks)
  • CefSharp/Internals/IJavaScriptObjectCache.cs (1 hunks)
  • CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1 hunks)
  • CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • CefSharp/Internals/LegacyJavaScriptObjectCache.cs
  • CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h
  • CefSharp/Internals/CefSharpArguments.cs
  • CefSharp.BrowserSubprocess.Core/SubProcess.h
  • CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h
🧰 Additional context used 🧠 Learnings (2) 📓 Common learnings 
Learnt from: amaitland
PR: cefsharp/CefSharp#4475
File: CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp:147-154
Timestamp: 2025-05-10T23:58:24.668Z
Learning: The `GetCache` method of the `IJavaScriptObjectCache` implementation should never return null in its current implementation. Both the legacy and per-browser cache implementations guarantee a non-null return value, with the per-browser implementation returning an empty dictionary when no cache exists for a given browser ID.
📚 Learning: 2025-05-10T23:58:24.668Z 
Learnt from: amaitland
PR: cefsharp/CefSharp#4475
File: CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp:147-154
Timestamp: 2025-05-10T23:58:24.668Z
Learning: The `GetCache` method of the `IJavaScriptObjectCache` implementation should never return null in its current implementation. Both the legacy and per-browser cache implementations guarantee a non-null return value, with the per-browser implementation returning an empty dictionary when no cache exists for a given browser ID.

Applied to files:

  • CefSharp/Internals/IJavaScriptObjectCache.cs
  • CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs
  • CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs
  • CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
🧬 Code graph analysis (5) CefSharp/Internals/IJavaScriptObjectCache.cs (3)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
  • CefSharp (17-98)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (2)
  • ClearCache (19-22)
  • InsertOrUpdate (25-31)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (2)
  • ClearCache (20-23)
  • InsertOrUpdate (26-34)
CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs (4)
CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (8)
  • Fact (12-24)
  • Fact (26-40)
  • Fact (42-50)
  • Fact (52-66)
  • Fact (68-85)
  • Fact (87-105)
  • Fact (107-115)
  • Fact (117-126)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (3)
  • LegacyJavaScriptObjectCache (13-44)
  • InsertOrUpdate (25-31)
  • ClearCache (19-22)
CefSharp/Internals/IJavaScriptObjectCache.cs (2)
  • InsertOrUpdate (40-40)
  • ClearCache (18-18)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (2)
  • InsertOrUpdate (26-34)
  • ClearCache (20-23)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (3)
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h (1)
  • CefBrowserWrapper (27-75)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)
  • PerBrowserJavaScriptObjectCache (14-68)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (1)
  • LegacyJavaScriptObjectCache (13-44)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (3)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
  • CefSharp (17-98)
CefSharp/Internals/IJavaScriptObjectCache.cs (4)
  • ClearCache (18-18)
  • InsertOrUpdate (40-40)
  • ICollection (33-33)
  • IDictionary (25-25)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (4)
  • ClearCache (19-22)
  • InsertOrUpdate (25-31)
  • ICollection (34-37)
  • IDictionary (40-43)
CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (3)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (3)
  • PerBrowserJavaScriptObjectCache (14-68)
  • InsertOrUpdate (26-34)
  • ClearCache (20-23)
CefSharp/Internals/IJavaScriptObjectCache.cs (2)
  • InsertOrUpdate (40-40)
  • ClearCache (18-18)
CefSharp/Internals/LegacyJavaScriptObjectCache.cs (2)
  • InsertOrUpdate (25-31)
  • ClearCache (19-22)
🔇 Additional comments (7)
CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs (7)

12-24: LGTM!

The test correctly verifies that ClearCache removes the cache for a specific browser ID.

42-50: LGTM!

Good edge case coverage for non-existent cache scenarios.

52-66: LGTM!

The test properly verifies GetCache returns the correct dictionary with expected content.

68-85: Excellent test for cache isolation!

This test properly validates that the per-browser cache maintains isolation between different browser instances, which is a critical requirement for this feature.

87-105: LGTM!

The test correctly verifies that InsertOrUpdate overwrites existing cache entries. The use of .First().Value at line 104 is safe due to the Assert.Single at line 103 ensuring exactly one element exists.

107-115: LGTM!

Good edge case coverage. The test correctly verifies that GetCache returns an empty dictionary (not null) for non-existent browser IDs, which aligns with the implementation guarantee.

Based on learnings.

117-126: LGTM!

Good defensive test ensuring empty object lists are handled gracefully.

AppVeyorBot commented Oct 11, 2025

Copy link
Copy Markdown

❌ Build CefSharp 140.1.140-CI5360 failed (commit 31c7b3a6c8 by @amaitland)

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

coderabbitai[bot] coderabbitai[bot] left review comments
+1 more reviewer
campersau campersau left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Javascript Binding - Store JavascriptObjects/Settings per CefBrowser

3 participants

Back | FazBrowse Home | New Git URL