Allow JavaScript binding when browser wrapper cannot be found#5229
Open
campersau wants to merge 3 commits intocefsharp:masterfrom
Open
Allow JavaScript binding when browser wrapper cannot be found#5229campersau wants to merge 3 commits intocefsharp:masterfrom
campersau wants to merge 3 commits intocefsharp:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughMoved per-browser JavaScript binding configuration out of Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (CefBrowser)
participant Subproc as CefAppUnmanagedWrapper
participant Store as _browserJavascriptBindingSettings
participant Frame as Frame (CefFrame)
Browser->>Subproc: OnBrowserCreated(browser)
Subproc->>Store: create & store JavascriptBindingSettings[browserId]
Subproc->>Subproc: _browserWrappers->TryAdd(browserId, wrapper)
Frame->>Subproc: OnContextCreated(frame, browserId)
Subproc->>Store: lookup JavascriptBindingSettings[browserId]
Subproc->>Subproc: IsJavascriptBindingApiAllowed(settings, frame)
alt allowed
Subproc->>Frame: inject CefSharp JS binding
else not allowed
Subproc->>Frame: skip injection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 352-363: IsJavascriptBindingApiAllowed currently returns true when
browserWrapper is null, bypassing per-browser enforcement; change the
null-wrapper path in IsJavascriptBindingApiAllowed to treat a missing
browserWrapper as not allowed (return false) so that JavascriptBindingApiEnabled
and JavascriptBindingApiHasAllowOrigins checks are enforced, and add unit tests
that simulate a missing browserWrapper where (1) JavascriptBindingApiEnabled is
false and (2) JavascriptBindingApiHasAllowOrigins is true to verify bindings are
not injected when the wrapper lookup fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b1d8844-dad5-4b5d-ab77-aff326ee7e6b
📒 Files selected for processing (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
|
❌ Build CefSharp 146.0.70-CI5456 failed (commit caa26a55be by @campersau) |
GoutamSarkarHyland
approved these changes
Apr 9, 2026
amaitland
requested changes
Apr 9, 2026
Member
amaitland
left a comment
amaitland
left a comment
There was a problem hiding this comment.
Thanks for the PR.
As a short term workaround this is probably ok, though I'd like to resolve this in a different way.
This avoids the issue that sometimes browsers are destroyed but then referenced again without them beeing created
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h`:
- Around line 76-83: The cleanup loop is iterating the ConcurrentDictionary
directly with OfType<JavascriptBindingSettings^> which yields nothing; update
the block that checks _browserJavascriptBindingSettings to iterate its values
(e.g., use _browserJavascriptBindingSettings->Values or iterate KeyValuePair and
use kvp.Value) and delete each JavascriptBindingSettings instance
(JavascriptBindingSettings^) before setting _browserJavascriptBindingSettings =
nullptr; keep the existing null check and ensure you delete the value objects
rather than the pairs.
In `@CefSharp.BrowserSubprocess.Core/JavascriptBindingSettings.h`:
- Around line 21-25: The subprocess constructor JavascriptBindingSettings()
currently sets JavascriptBindingApiEnabled = true which contradicts the
documented default (false) and causes unintended injection; change the
constructor to set JavascriptBindingApiEnabled = false (keep the existing
initializations for JavascriptBindingApiHasAllowOrigins and
JavascriptBindingApiAllowOrigins intact) so the subprocess default matches
CefSharp/JavascriptBinding/JavascriptBindingSettings.cs and
IsJavascriptBindingApiAllowed behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe720d1-0267-4703-bec5-1c4c33f091c1
📒 Files selected for processing (6)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.hCefSharp.BrowserSubprocess.Core/CefBrowserWrapper.hCefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxprojCefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxprojCefSharp.BrowserSubprocess.Core/JavascriptBindingSettings.h
💤 Files with no reviewable changes (1)
- CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
✅ Files skipped from review due to trivial changes (2)
- CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxproj
- CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj
|
✅ Build CefSharp 146.0.70-CI5457 completed (commit 02011e55d0 by @campersau) |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
30-54:⚠️ Potential issue | 🔴 CriticalAdd per-browser cleanup for
_browserJavascriptBindingSettingsinOnBrowserDestroyed.
_browserJavascriptBindingSettingsentries are added when browsers are created but never removed when destroyed. Currently, cleanup only occurs at process teardown (destructor). In long-lived subprocesses with multiple browser creations/destructions, this causes unbounded accumulation of stale per-browser settings. Add aTryRemovecall inOnBrowserDestroyedto clean up the entry for the destroyed browser ID, matching the pattern already used for_browserWrappers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h` around lines 30 - 54, OnBrowserDestroyed currently removes entries from _browserWrappers but does not remove the per-browser settings in _browserJavascriptBindingSettings, causing leaks; modify OnBrowserDestroyed to call TryRemove on _browserJavascriptBindingSettings using the same browser id (the key used with _browserWrappers) to remove the destroyed browser's JavascriptBindingSettings entry (mirror the existing removal pattern for _browserWrappers and ensure any associated resources are released).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h`:
- Around line 30-54: OnBrowserDestroyed currently removes entries from
_browserWrappers but does not remove the per-browser settings in
_browserJavascriptBindingSettings, causing leaks; modify OnBrowserDestroyed to
call TryRemove on _browserJavascriptBindingSettings using the same browser id
(the key used with _browserWrappers) to remove the destroyed browser's
JavascriptBindingSettings entry (mirror the existing removal pattern for
_browserWrappers and ensure any associated resources are released).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a472c299-1812-48fa-b0e4-3ae109c6e717
📒 Files selected for processing (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
|
✅ Build CefSharp 146.0.70-CI5458 completed (commit dc7a0db18f by @campersau) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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: #5228
Summary: Fixes regression to allow JavaScript bindings when browser wrapper can not be found.
Changes:
How Has This Been Tested?
Manually.
My App works again with this build: https://ci.appveyor.com/project/campersau/cefsharp/builds/53796822/artifacts
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit