Skip to content

fix ObjectDisposedException in synchronous js binding #5260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
skillbert wants to merge 1 commit into
base: master
Choose a base branch
from
+44 −6
Open

fix ObjectDisposedException in synchronous js binding #5260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,24 @@ namespace CefSharp
return this;
};

ref struct CefBrowserWrapperUpdater
{
CefBrowserWrapper^ _capture;
CefAppUnmanagedWrapper& _wrapper;
CefBrowserWrapperUpdater(CefBrowserWrapper^ capture, CefAppUnmanagedWrapper& wrapper) :
_capture(capture),
_wrapper(wrapper) {
}

public:
CefBrowserWrapper^ update(int key, CefBrowserWrapper^ oldValue)
{
_wrapper._onBrowserDestroyed->Invoke(oldValue);
delete oldValue;
return _capture;
}
};

// CefRenderProcessHandler
void CefAppUnmanagedWrapper::OnBrowserCreated(CefRefPtr<CefBrowser> browser, CefRefPtr<CefDictionaryValue> extraInfo)
{
Expand All @@ -63,7 +81,14 @@ namespace CefSharp

//Multiple CefBrowserWrappers created when opening popups
auto browserId = browser->GetIdentifier();
_browserWrappers->TryAdd(browserId, wrapper);

// In some cases CEF recreates the browser with identical id instead of making a new one
// If this happens it will call OnBrowserCreated a second time before calling OnBrowserDestroyed for the first browser
// We need to check if the browser wrapper with given id already exists and if it does destroy it before adding the new one
// https://github.com/chromiumembedded/cef/blob/ffa86e361d9c210136b539953663526add56191e/include/cef_render_process_handler.h#L67
auto updater = gcnew CefBrowserWrapperUpdater(wrapper, *this);
auto updateFunc = gcnew Func<int, CefBrowserWrapper^, CefBrowserWrapper^>(updater, &CefBrowserWrapperUpdater::update);
_browserWrappers->AddOrUpdate(browserId, wrapper, updateFunc);

static gcroot<Func<int, JavascriptBindingSettings^>^> factory =
gcnew Func<int, JavascriptBindingSettings^>(CefAppUnmanagedWrapper::JavascriptBindingSettingsFactory);
Expand Down Expand Up @@ -136,11 +161,20 @@ namespace CefSharp

void CefAppUnmanagedWrapper::OnBrowserDestroyed(CefRefPtr<CefBrowser> browser)
{
CefBrowserWrapper^ wrapper;
if (_browserWrappers->TryRemove(browser->GetIdentifier(), 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^ 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;
}
}
}
Comment on lines +164 to 178
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


// Don't remove javascript settings because cef is unreliable in calling OnBrowserCreated/OnBrowserDestroyed consistently:
Expand Down
1 change: 1 addition & 0 deletions CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ namespace CefSharp
// This class is the native subprocess level CEF object wrapper.
private class CefAppUnmanagedWrapper : SubProcessApp, CefRenderProcessHandler
{
friend ref struct CefBrowserWrapperUpdater;
private:
gcroot<IRenderProcessHandler^> _handler;
gcroot<Action<CefBrowserWrapper^>^> _onBrowserCreated;
Expand Down
3 changes: 3 additions & 0 deletions CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ namespace CefSharp
{
this->!CefBrowserWrapper();
}
bool IsSameBrowser(CefRefPtr<CefBrowser> other) {
return this->_cefBrowser->IsSame(other);
}

property int BrowserId;
property bool IsPopup;
Expand Down