Core - Rewrite RenderProcess to avoid using OnBrowserDestroyed for JS eval #5197
Conversation
…d of per browser - Don't store JavascriptRootObjectWrapper in the browserWrapper as CEF is calling destroyed more than it should. -
WalkthroughRefactors JavaScript root object management from per-browser collections to a centralized frame-id keyed map, injects Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)❌ Error creating Unit Test PR.
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (2)
🔇 Additional comments (4)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
375-394: Consider extracting duplicated root object creation logic.The root object lookup and creation logic here (lines 381-393) duplicates
GetJsRootObjectWrapper()(lines 304-328). While the inline approach works and handles the edge case whereOnContextCreatedwasn't called, consider callingGetJsRootObjectWrapper()to reduce duplication:JavascriptRootObjectWrapper^ rootObjectWrapper; _jsRootObjectWrappersByFrameId->TryGetValue(frameId, rootObjectWrapper); - //NOTE: In the rare case when when OnContextCreated hasn't been called we need to manually create the rootObjectWrapper - //It appears that OnContextCreated is only called for pages that have javascript on them, which makes sense - //as without javascript there is no need for a context. if (rootObjectWrapper == nullptr) { -#ifdef NETCOREAPP - rootObjectWrapper = gcnew JavascriptRootObjectWrapper(); -#else - auto browserWrapper = FindBrowserWrapper(browser->GetIdentifier()); - - // If we cannot find the browserWrapper then we'll pass in nullptr which will disable - // the use of sync bound objects (async will still work). - rootObjectWrapper = gcnew JavascriptRootObjectWrapper(browserWrapper == nullptr ? nullptr : browserWrapper->BrowserProcess); -#endif - - _jsRootObjectWrappersByFrameId->TryAdd(frameId, rootObjectWrapper); + //NOTE: In the rare case when OnContextCreated hasn't been called we need to manually create the rootObjectWrapper + rootObjectWrapper = GetJsRootObjectWrapper(browser->GetIdentifier(), frame->GetIdentifier()); }CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (2)
27-44: Make theJavascriptRootObjectWrapperdependency explicit in this header
BindObjectAsyncHandlernow stores and usesJavascriptRootObjectWrapper^(including callingBindin this header), but this file doesn’t declare or include that type. That makes compilation depend on external include order in consuming translation units.To keep the header self‑contained and avoid fragile ordering, consider explicitly including the wrapper’s header (or otherwise ensuring its declaration is available before this header is parsed), for example:
#include "include/cef_v8.h" #include "RegisterBoundObjectRegistry.h" #include "..\CefSharp.Core.Runtime\Internals\Messaging\Messages.h" #include "..\CefSharp.Core.Runtime\Internals\Serialization\Primitives.h" + #include "JavascriptRootObjectWrapper.h"If
JavascriptRootObjectWrapper.hcannot be included here due to cycles, at minimum document the required include order where this header is used.
140-152: Clarify invariants around_javascriptRootObjectWrapperin the cached‑bind fast pathThe cached‑objects fast path now hard‑depends on
_javascriptRootObjectWrapperbeing non‑null and fails the call with a JS exception when it’s null:if (Object::ReferenceEquals(_javascriptRootObjectWrapper, nullptr)) { exception = "BindObjectAsyncHandler::Execute - _javascriptRootObjectWrapper null, unable to bind objects"; return true; } JavascriptRootObjectWrapper^ rootObject = _javascriptRootObjectWrapper; rootObject->Bind(cachedObjects, context->GetGlobal());If the design guarantees that a valid
JavascriptRootObjectWrapperis always supplied when constructing this handler (which it likely should, given the new per‑frame management), it may be cleaner to enforce that at construction time (argument validation or debug assert) and treat a null here as an invariant violation (and log at ERROR level) rather than a “normal” runtime error path. That would make failures due to mis‑wiring or lifetime mismatches easier to detect during development.If null is expected in some edge cases, consider adding a brief comment explaining when that can legitimately happen.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h(2 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp(7 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h(3 hunks)CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h(0 hunks)CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp(1 hunks)CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h(1 hunks)CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h(1 hunks)
💤 Files with no reviewable changes (1)
- CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T23:58:24.668Z
Learnt from: amaitland
Repo: cefsharp/CefSharp PR: 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.BrowserSubprocess.Core/BindObjectAsyncHandler.h
🧬 Code graph analysis (4)
CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp (1)
CefSharp/Internals/JavascriptCallback.cs (1)
JavascriptCallback(38-54)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
JavascriptRootObjectWrapper(31-100)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
JavascriptRootObjectWrapper(31-100)
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (2)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
JavascriptRootObjectWrapper(31-100)CefSharp.BrowserSubprocess.Core/RegisterBoundObjectRegistry.h (1)
RegisterBoundObjectRegistry(22-62)
🔇 Additional comments (9)
CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp (1)
21-25: LGTM - Browser ID now derived from context.Deriving
BrowserIdfromcontext->GetBrowser()->GetIdentifier()at registration time is the correct approach for the frame-centric refactor. This ensures the callback captures the correct browser ID regardless of which frame's registry it was registered with.CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
58-72: LGTM - Constructor simplified to remove browserId dependency.The constructor changes correctly align with the refactored
JavascriptCallbackRegistry. The non-NETCOREAPP path still receivesIBrowserProcess^for synchronous bound object support, which is appropriate.CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h (1)
28-32: LGTM - Parameterless constructor aligns with frame-centric design.The removal of
browserIdfrom the constructor is correct. The browser ID is now derived from the V8 context at callback registration time (inRegister()), which is the appropriate location given the new frame-based architecture.CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (4)
159-159: LGTM - BindObjectAsyncHandler now receives rootObject directly.Passing
JavascriptRootObjectWrapper^instead ofCefBrowserWrapper^aligns with the frame-centric refactor. TherootObjectis obtained fromGetJsRootObjectWrapper()at line 140, which handles the lookup/creation appropriately.
223-236: LGTM - Proper cleanup on context release.The cleanup logic correctly:
- Captures a local reference to avoid race conditions
- Performs null check before proceeding
- Removes and deletes the wrapper when the frame's context is released
This ensures proper resource management per frame lifecycle.
304-328: LGTM - GetJsRootObjectWrapper correctly manages frame-based root objects.The implementation properly:
- Looks up existing wrapper by frame ID
- Creates a new wrapper if not found (lazy initialization)
- Handles the non-NETCOREAPP case where
BrowserProcessis still needed for sync bound objects- Returns
nullptrgracefully whenbrowserWrapperis unavailableThe callers appropriately handle the
nullptrcase (e.g., line 144, 607).
468-470: LGTM - Consistent usage of frame-based root object lookup.The callback request handler correctly looks up the root object by frame ID and handles the null case gracefully (lines 470-474).
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (2)
29-29: LGTM - New frame-based root object map declaration.The addition of
_jsRootObjectWrappersByFrameIdas aConcurrentDictionary<String^, JavascriptRootObjectWrapper^>^correctly supports the frame-centric architecture change.
51-52: LGTM - Proper initialization of the new map.The
_jsRootObjectWrappersByFrameIdis correctly initialized as a newConcurrentDictionaryin the constructor.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (3)
304-323: Potential race condition in lazy initialization.The
TryGetValue+TryAddpattern is not atomic. If multiple threads callGetJsRootObjectWrapperconcurrently for the sameframeId, both may create aJavascriptRootObjectWrapper, but only one will be added to the dictionary. The method would then return different instances to different callers, potentially causing inconsistent state.Consider using a thread-safe lazy initialization pattern. Apply this diff to use
GetOrAdd:JavascriptRootObjectWrapper^ CefAppUnmanagedWrapper::GetJsRootObjectWrapper(int browserId, CefString& frameId) { auto rootObjectWrappers = _jsRootObjectWrappersByFrameId; auto frameIdClr = StringUtils::ToClr(frameId); - JavascriptRootObjectWrapper^ rootObject; - if (!rootObjectWrappers->TryGetValue(frameIdClr, rootObject)) - { + auto rootObject = rootObjectWrappers->GetOrAdd(frameIdClr, gcnew Func<String^, JavascriptRootObjectWrapper^>( + [this, browserId](String^ key) -> JavascriptRootObjectWrapper^ { #ifdef NETCOREAPP - rootObject = gcnew JavascriptRootObjectWrapper(); + return gcnew JavascriptRootObjectWrapper(); #else - auto browserWrapper = FindBrowserWrapper(browserId); - - rootObject = gcnew JavascriptRootObjectWrapper(browserWrapper == nullptr ? nullptr : browserWrapper->BrowserProcess); + auto browserWrapper = FindBrowserWrapper(browserId); + return gcnew JavascriptRootObjectWrapper(browserWrapper == nullptr ? nullptr : browserWrapper->BrowserProcess); #endif - rootObjectWrappers->TryAdd(frameIdClr, rootObject); - } + } + )); return rootObject; }
304-323: Recommend comprehensive testing for this refactoring.This refactoring fundamentally changes how
JavascriptRootObjectWrapperinstances are managed (from per-browser to per-frame). Given the complexity and the multiple code paths involved (legacy binding, JS binding API, message handling, context lifecycle), comprehensive testing is essential to ensure:
- Correct behavior when frames are created/destroyed
- Thread safety under concurrent access
- Proper cleanup and no memory leaks
- Correct operation in popup scenarios
- Behavior when contexts are created without prior browser creation
The PR checklist indicates testing is "TBA". Please ensure thorough testing before merging.
140-163: Based on my verification of the code, I have confirmed the review comment's findings. Let me now generate the rewritten review comment:
Unnecessary null checking for rootObject due to implementation guarantee.
GetJsRootObjectWrapper(lines 304-323) always creates and returns aJavascriptRootObjectWrapper, never returningnullptr. Both code paths—when the wrapper is found in cache or newly created—guarantee a non-null return. However, line 144 performs a null check (rootObject != nullptr), while line 159 passesrootObjecttoBindObjectAsyncHandlerwithout any null check, and line 163 includes a ternary operator for null checking. This inconsistent null handling suggests the null check at line 144 is unnecessary defensive code. Remove it to accurately reflect the implementation guarantee.
🧹 Nitpick comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
368-371: Update comment to reflect new lazy initialization.The comment references the "rare case when OnContextCreated hasn't been called" and explains that OnContextCreated is only called for pages with JavaScript. While this is accurate, the comment could be clearer given the refactoring.
GetJsRootObjectWrappernow provides lazy initialization and is called both inOnContextCreated(line 140) and here, ensuring a rootObjectWrapper always exists when needed.Consider updating the comment to:
-//NOTE: In the rare case when when OnContextCreated hasn't been called we need to manually create the rootObjectWrapper -//It appears that OnContextCreated is only called for pages that have javascript on them, which makes sense -//as without javascript there is no need for a context. +//NOTE: GetJsRootObjectWrapper lazily creates the rootObjectWrapper if it doesn't exist. +//This handles cases where OnContextCreated hasn't been called (e.g., for pages without JavaScript). JavascriptRootObjectWrapper^ rootObjectWrapper = GetJsRootObjectWrapper(browser->GetIdentifier(), frame->GetIdentifier());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp(7 hunks)
🔇 Additional comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
315-317: _browserProcess null handling is properly implemented with defensive checks.The
_browserProcessfield is only dereferenced in theBindmethod (line 35 of JavascriptRootObjectWrapper.cpp), where an explicit null check is performed before any usage. When_browserProcessis nullptr, the code logs an ERROR message and skips that object with acontinuestatement, preventing any null dereference issues. The null-handling is correct and safe.
|
❌ Build CefSharp 142.0.150-CI5381 failed (commit ee6f9785b5 by @amaitland) |
amaitland
changed the title
|
❌ Build CefSharp 142.0.150-CI5383 failed (commit f250acca87 by @amaitland) |
|
❌ Build CefSharp 142.0.150-CI5384 failed (commit f250acca87 by @amaitland) |
…dObjectName" This reverts commit f8a81ac.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
60-60: Optionally dispose theIFramewhen readingIdentifierFor consistency with the earlier
using (var frame = browser.GetMainFrame())pattern and to dispose theIFramewrapper promptly, you could wrap this call in ausing:- var frameId = browser.GetMainFrame().Identifier; + long frameId; + using (var frame = browser.GetMainFrame()) + { + frameId = frame.Identifier; + }Not critical for the test, but slightly cleaner and consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs(1 hunks)CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (1)
CefSharp.Example/CefExample.cs (1)
CefExample(15-274)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
CefSharp.Example/CefExample.cs (1)
CefExample(15-274)
🔇 Additional comments (2)
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (1)
97-100: Explicit navigation improves test determinismLoading
BindingApiCustomObjectNameTestUrlexplicitly and assertingloadResponse.Successbefore evaluating the script makes this test independent of prior browser state and should reduce flakiness. The change looks correct and consistent with patterns used elsewhere in the suite.CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
56-69: Frame ID capture and frame-scoped assertion look correctCapturing
frameIdfrombrowser.GetMainFrame().Identifierbefore reloading, then asserting the error message starts with"Frame with Id: {frameId}", aligns the test with the new per-frame callback handling and keeps it robust viaAssert.StartsWithwhile allowing message suffix changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
60-69: Frame‑Id based assertion looks correct; consider disposing the frame instanceSwitching the assertion to use
frameIdand expecting"Frame with Id:{frameId}"aligns with the new frame‑scoped error messaging and looks logically correct. One minor improvement: the additionalbrowser.GetMainFrame()at Line 60 is not disposed, whereas earlier in the test you use ausingblock for the frame. For consistency and to avoid holding onto the wrapper longer than needed, you could grab the identifier via a shortusing:- var callbackExecuteCancelAfterV8ContextTask = callbackExecuteCancelAfterV8ContextCallback.ExecuteAsync(); - var frameId = browser.GetMainFrame().Identifier; + var callbackExecuteCancelAfterV8ContextTask = callbackExecuteCancelAfterV8ContextCallback.ExecuteAsync(); + int frameId; + using (var frame = browser.GetMainFrame()) + { + frameId = frame.Identifier; + }This keeps the behavior the same while matching the existing disposal pattern in the test.
|
❌ Build CefSharp 142.0.150-CI5390 failed (commit a5352544a6 by @amaitland) |
|
✅ Build CefSharp 142.0.150-CI5391 completed (commit a5352544a6 by @amaitland) |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Fixes:
Related to #4606
Summary:
On
OnBrowserDestroyedis no longer reliable, gets called when it shouldn'tForum thread https://www.magpcss.org/ceforum/viewtopic.php?f=6&t=20428
Changes:
How Has This Been Tested?
TBA
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Behavior Changes
Chore
Tests
✏️ Tip: You can customize this high-level summary in your review settings.