fix ObjectDisposedException in synchronous js binding#5260
Open
skillbert wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR improves browser-wrapper lifecycle management in the CEF wrapper to handle cases where the browser engine reuses identifiers. It adds browser identity comparison, an updater callback mechanism, and guarded creation/destruction logic to clean up stale wrappers and prevent mismatched references. ChangesBrowser wrapper lifecycle improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 164-178: The removal can delete a different wrapper due to the
race between TryGetValue and TryRemove; change the logic to perform TryRemove
first and then verify the removed wrapper matches the expected browser before
invoking and deleting it. Specifically, use
_browserWrappers->TryRemove(browserId, wrapper) and then check wrapper !=
nullptr && wrapper->IsSameBrowser(browser) (or compare to currentValue if you
retained it) before calling _onBrowserDestroyed->Invoke(wrapper) and delete
wrapper; ensure you only delete the wrapper that you verified matches the
incoming CefRefPtr by using IsSameBrowser on the actually-removed wrapper.
🪄 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: 6446284d-0a3b-481f-887e-5847315259aa
📒 Files selected for processing (3)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.hCefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
Comment on lines
+164
to
178
| int browserId = browser->GetIdentifier(); | ||
| CefBrowserWrapper^ currentValue; | ||
| if (_browserWrappers->TryGetValue(browserId, currentValue)) { | ||
| // Check if another wrapper hasn't already taken our place | ||
| if (currentValue->IsSameBrowser(browser)) { | ||
| CefBrowserWrapper^ wrapper; | ||
| // This is technically a race condition since the entry for browser id might have changed between TryGetValue | ||
| // and TryRemove. No clear way to fix this with ConcurrentDictionary | ||
| if (_browserWrappers->TryRemove(browserId, wrapper)) | ||
| { | ||
| _onBrowserDestroyed->Invoke(wrapper); | ||
| delete wrapper; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition can delete the wrong wrapper.
The code checks currentValue->IsSameBrowser(browser) but then TryRemove outputs to a different variable wrapper. If the dictionary entry changes between TryGetValue and TryRemove (as acknowledged in the comment), the deleted wrapper could differ from the checked currentValue.
While CEF render process callbacks are typically serialized on the render thread (making true races unlikely), the current structure doesn't guarantee correctness. Consider using a pattern that atomically checks and removes, or at minimum, re-verify identity after removal:
Suggested defensive check
- CefBrowserWrapper^ wrapper;
- // This is technically a race condition since the entry for browser id might have changed between TryGetValue
- // and TryRemove. No clear way to fix this with ConcurrentDictionary
- if (_browserWrappers->TryRemove(browserId, wrapper))
+ CefBrowserWrapper^ removedWrapper;
+ if (_browserWrappers->TryRemove(browserId, removedWrapper))
{
- _onBrowserDestroyed->Invoke(wrapper);
- delete wrapper;
+ // Re-verify we removed the wrapper we intended to remove
+ if (removedWrapper->IsSameBrowser(browser))
+ {
+ _onBrowserDestroyed->Invoke(removedWrapper);
+ delete removedWrapper;
+ }
+ else
+ {
+ // Raced with AddOrUpdate; put the new wrapper back
+ _browserWrappers->TryAdd(browserId, removedWrapper);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int browserId = browser->GetIdentifier(); | |
| CefBrowserWrapper^ currentValue; | |
| if (_browserWrappers->TryGetValue(browserId, currentValue)) { | |
| // Check if another wrapper hasn't already taken our place | |
| if (currentValue->IsSameBrowser(browser)) { | |
| CefBrowserWrapper^ wrapper; | |
| // This is technically a race condition since the entry for browser id might have changed between TryGetValue | |
| // and TryRemove. No clear way to fix this with ConcurrentDictionary | |
| if (_browserWrappers->TryRemove(browserId, wrapper)) | |
| { | |
| _onBrowserDestroyed->Invoke(wrapper); | |
| delete wrapper; | |
| } | |
| } | |
| } | |
| int browserId = browser->GetIdentifier(); | |
| CefBrowserWrapper^ currentValue; | |
| if (_browserWrappers->TryGetValue(browserId, currentValue)) { | |
| // Check if another wrapper hasn't already taken our place | |
| if (currentValue->IsSameBrowser(browser)) { | |
| CefBrowserWrapper^ removedWrapper; | |
| if (_browserWrappers->TryRemove(browserId, removedWrapper)) | |
| { | |
| // Re-verify we removed the wrapper we intended to remove | |
| if (removedWrapper->IsSameBrowser(browser)) | |
| { | |
| _onBrowserDestroyed->Invoke(removedWrapper); | |
| delete removedWrapper; | |
| } | |
| else | |
| { | |
| // Raced with AddOrUpdate; put the new wrapper back | |
| _browserWrappers->TryAdd(browserId, removedWrapper); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp` around lines 164
- 178, The removal can delete a different wrapper due to the race between
TryGetValue and TryRemove; change the logic to perform TryRemove first and then
verify the removed wrapper matches the expected browser before invoking and
deleting it. Specifically, use _browserWrappers->TryRemove(browserId, wrapper)
and then check wrapper != nullptr && wrapper->IsSameBrowser(browser) (or compare
to currentValue if you retained it) before calling
_onBrowserDestroyed->Invoke(wrapper) and delete wrapper; ensure you only delete
the wrapper that you verified matches the incoming CefRefPtr by using
IsSameBrowser on the actually-removed wrapper.
|
✅ Build CefSharp 148.0.90-CI5509 completed (commit dda2249daf by @skillbert) |
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
2 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:
#5232
Summary:
CEF 145 introduced several issues with synchronous js bindings. This PR addresses an issue with the WCF channel used for synchronous js bindings being broken after reloading the page.
It seems like the CEF update made it so that
CefBrowserids can still be reused when the underlying CefBrowser has to be recreated (A code comment states that this happens on cross-origin navigation, but it seems to happen at any navigation). When CEF recreates theCefBrowserlike this it fires theOnBrowserCreatedevent before firing the correspondingOnBrowserDestroyedof the old browser with same id.Changes:
OnBrowserCreatedif the given id already exists in _browserWrappersHow Has This Been Tested?
I didn't see a way to enable WCF in the unit tests, so it has been tested manually.
CefSharp.WinForms.ExampleScreenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes