From 84117f2d1be1ce54513290d835c4dcb79da750d4 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 dbade2dd8..b0ba616f1 100644 --- a/libcef/browser/browser_info.cc +++ b/libcef/browser/browser_info.cc @@ -234,7 +234,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_ || is_closing_) return nullptr; @@ -504,6 +504,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_); @@ -518,6 +520,8 @@ CefBrowserInfo::NotificationStateLock::NotificationStateLock( } CefBrowserInfo::NotificationStateLock::~NotificationStateLock() { + CEF_REQUIRE_UIT(); + // Unlock in reverse order. browser_info_lock_scope_.reset(); @@ -530,11 +534,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 fe61b823f..11796ad0b 100644 --- a/libcef/browser/browser_info.h +++ b/libcef/browser/browser_info.h @@ -204,7 +204,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);