From f38ce3409c5cfc5dcadbf1e219ffaaf7e720b609 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Thu, 16 Sep 2021 11:01:55 +0300 Subject: [PATCH] Don't use NotificationStateLock for GetMainFrame (see issue #2421) This causes a race related to |notification_state_lock_| assignment when GetMainFrame is called from multiple threads. GetMainFrame doesn't trigger any notifications so it shouldn't need that lock. Instead, only use NotificationStateLock on the UI thread. --- libcef/browser/browser_info.cc | 13 +++++++------ libcef/browser/browser_info.h | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/libcef/browser/browser_info.cc b/libcef/browser/browser_info.cc index ada824d5b..952f5b66c 100644 --- a/libcef/browser/browser_info.cc +++ b/libcef/browser/browser_info.cc @@ -207,7 +207,7 @@ void CefBrowserInfo::RemoveFrame(content::RenderFrameHost* host) { } CefRefPtr CefBrowserInfo::GetMainFrame() { - NotificationStateLock lock_scope(this); + base::AutoLock lock_scope(lock_); // Early exit if called post-destruction. if (!browser_) return nullptr; @@ -451,6 +451,8 @@ void CefBrowserInfo::RemoveAllFrames( CefBrowserInfo::NotificationStateLock::NotificationStateLock( CefBrowserInfo* browser_info) : browser_info_(browser_info) { + CEF_REQUIRE_UIT(); + // Take the navigation state lock. { base::AutoLock lock_scope_(browser_info_->notification_lock_); @@ -465,6 +467,8 @@ CefBrowserInfo::NotificationStateLock::NotificationStateLock( } CefBrowserInfo::NotificationStateLock::~NotificationStateLock() { + CEF_REQUIRE_UIT(); + // Unlock in reverse order. browser_info_lock_scope_.reset(); @@ -477,11 +481,8 @@ CefBrowserInfo::NotificationStateLock::~NotificationStateLock() { if (!queue_.empty()) { DCHECK(frame_handler_); - scoped_refptr nav_lock; - if (CEF_CURRENTLY_ON_UIT()) { - // Don't navigate while inside callbacks. - nav_lock = browser_info_->CreateNavigationLock(); - } + // Don't navigate while inside callbacks. + auto nav_lock = browser_info_->CreateNavigationLock(); // Empty the queue of pending actions. Any of these actions might result in // the acquisition of a new NotificationStateLock. diff --git a/libcef/browser/browser_info.h b/libcef/browser/browser_info.h index 0a2126075..e8eb4ef33 100644 --- a/libcef/browser/browser_info.h +++ b/libcef/browser/browser_info.h @@ -192,7 +192,7 @@ class CefBrowserInfo : public base::RefCountedThreadSafe { // Used instead of |base::AutoLock(lock_)| in situations that might generate // CefFrameHandler notifications. Any notifications passed to // MaybeExecuteFrameNotification() will be queued until the lock is released, - // and then executed in order. + // and then executed in order. Only accessed on the UI thread. class NotificationStateLock final { public: explicit NotificationStateLock(CefBrowserInfo* browser_info);