Javascript Binding - Store JavascriptObjects per CefBrowser #4475
Conversation
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
|
✅ Build CefSharp 112.2.90-CI4758 completed (commit 91bd2be36a by @amaitland) |
campersau
reviewed
Apr 18, 2023
|
✅ Build CefSharp 112.2.100-CI4779 completed (commit 04fe8b2185 by @amaitland) |
amaitland
force-pushed
the
jsb/perbrowsercache
branch
from
f18c0ff to
8eb2376
Compare
|
✅ Build CefSharp 115.3.110-CI4829 completed (commit c19986019f by @amaitland) |
|
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 |
amaitland
force-pushed
the
jsb/perbrowsercache
branch
from
8eb2376 to
fcc0cdb
Compare
|
✅ Build CefSharp 132.3.10-CI5157 completed (commit 3c5f7865ac by @amaitland) |
This would require a totally different approach. |
amaitland
force-pushed
the
jsb/perbrowsercache
branch
from
fcc0cdb to
1fed9c9
Compare
## Walkthrough
This change introduces a new per-browser JavaScript object cache for the render process, allowing JavaScript binding objects to be stored and managed separately for each browser instance. It adds two cache implementations, updates constructors and interfaces, and modifies logic to use the appropriate cache based on a command-line flag. Comprehensive tests for both cache types are included.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|---------------|
| CefSharp/Internals/IJavaScriptObjectCache.cs<br>CefSharp/Internals/LegacyJavaScriptObjectCache.cs<br>CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs | Introduced a new interface for JavaScript object caching and two implementations: one for legacy (process-wide) and one for per-browser caching. |
| CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp<br>CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h<br>CefSharp.BrowserSubprocess.Core/SubProcess.h | Replaced direct dictionary management with the new cache abstraction. Updated constructors to accept a cache-per-browser flag and use the appropriate cache implementation. |
| CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h<br>CefSharp.BrowserSubprocess.Core/RegisterBoundObjectHandler.h | Updated constructor and member variable types from concrete dictionaries to interface-based types for flexibility. |
| CefSharp/Internals/CefSharpArguments.cs | Added a new command-line argument constant for enabling per-browser cache. |
| CefSharp.Test/JavascriptBinding/LegacyJavaScriptObjectCacheTests.cs<br>CefSharp.Test/JavascriptBinding/PerBrowserJavaScriptObjectCacheTests.cs | Added comprehensive unit tests for both legacy and per-browser JavaScript object cache implementations. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant SubProcess
participant CefAppUnmanagedWrapper
participant IJavaScriptObjectCache
participant BrowserInstance
SubProcess->>CefAppUnmanagedWrapper: Create(jsbCachePerBrowser)
CefAppUnmanagedWrapper->>IJavaScriptObjectCache: Instantiate (per-browser or legacy)
BrowserInstance->>IJavaScriptObjectCache: InsertOrUpdate(browserId, jsObjects)
BrowserInstance->>IJavaScriptObjectCache: GetCacheValues(browserId)
BrowserInstance->>IJavaScriptObjectCache: ClearCache(browserId)Assessment against linked issues
Poem
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
CefSharp/Internals/PerBrowserJavaScriptObjectCache.cs (1)
1-68: Well-implemented per-browser JavaScript object cacheThis 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:
GetCachewill 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:ClearCacheresult 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:
- Add a short explanatory comment (
// no-op for Legacy cache) to prevent future confusion.- Optionally gate the call behind an
if (_jsbCachePerBrowser)flag to save an unnecessary virtual dispatch.
628-629: Potential duplication of cached objects
InsertOrUpdateis invoked both here and inOnBrowserCreated. 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:
InsertOrUpdateoverwrites existing entries that share the same key.- 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
ClearCacheShouldDoNothingintentionally 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 overwriteAfter 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
📒 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::HasArgumentmethod 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
CefAppUnmanagedWrapperconstructor is now called with the newjsbCachePerBrowserparameter, 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
IDictionaryinterface instead of the concreteDictionaryclass. 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
IDictionaryinterface instead of the concreteDictionaryclass. 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 cachingThis 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 cachingThe replacement of the concrete dictionary with an interface type provides better abstraction and flexibility.
45-64: LGTM: Proper initialization based on caching strategyThe constructor now correctly initializes the appropriate cache implementation based on the
jsbCachePerBrowserflag. 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 compatibilityThis implementation correctly maintains the legacy behavior of caching JavaScript objects per process rather than per browser. The
ClearCachemethod 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
GetCacheValuesis assumed to return a non-null collection. If the implementation ever changes to returnnullptrwhen the browserId is not present, the subsequent->Countdereference 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);
Containsrelies on reference equality unlessJavascriptObjectoverridesEquals/GetHashCode.
If equality is not overridden, a scenario where two distinctJavascriptObjectinstances share the sameNamebut both remain in the cache would not be detected by these tests.Please either:
- Override equality on
JavascriptObjectto be value-based (preferred), or- Assert against the cache dictionary keys instead of values.
This will make the test guard the contract more strictly.
|
✅ Build CefSharp 136.1.40-CI5239 completed (commit 344ec7fb5f by @amaitland) |
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
amaitland
force-pushed
the
jsb/perbrowsercache
branch
from
83b6747 to
1d19085
Compare
|
✅ Build CefSharp 136.1.40-CI5258 completed (commit df9b49b25a by @amaitland) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #2306
Summary:
This optionally allows for a new Per Browser cache.
Use the following command line arg to opt into this behaviour.
Changes:
CefSharpspecific command line arg to optionally enable new code pathHow Has This Been Tested?
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation