From dad4ab658a9b16a94c0bd7d2ccf86b73067793cf Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Tue, 14 Apr 2026 09:03:18 -0500 Subject: [PATCH] fix: timing issue DCHECK crash in DesktopCapturer on macOS (#50960) refactor: use StartUpdating in desktopCapturer Replace the one-shot Update() callback model with the continuous StartUpdating() observer model for NativeDesktopMediaList. Fixes a macOS DCHECK(can_refresh()) crash in UpdateSourceThumbnail(), where ScreenCaptureKit's recurrent thumbnail capturer would post UpdateSourceThumbnail callbacks after the one-shot refresh_callback_ had been consumed. Now, can_refresh() is always true because refresh_callback_ is repopulated via ScheduleNextRefresh(). Each capturer (window, screen) gets its own ListObserver that tracks readiness via OnSourceAdded and OnSourceThumbnailChanged events. Once a list has both sources and thumbnails (or thumbnails aren't requested), its data is snapshotted and the capturer checks if all requested types are ready before resolving to JS. Also remove the "skip_next_refresh_" Chromium patch, which was a workaround for the timing mismatch between the one-shot Update() model and ScreenCaptureKit's asynchronous thumbnail delivery. refactor: simplify state logic in DesktopCapturer --- patches/chromium/.patches | 1 - ...first_2_no-op_refreshes_in_thumb_cap.patch | 45 --- .../api/electron_api_desktop_capturer.cc | 292 +++++++++--------- .../api/electron_api_desktop_capturer.h | 55 +--- 4 files changed, 157 insertions(+), 236 deletions(-) delete mode 100644 patches/chromium/fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 43b50a5dba..5ab2d36b49 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -103,7 +103,6 @@ chore_remove_check_is_test_on_script_injection_tracker.patch fix_restore_original_resize_performance_on_macos.patch feat_allow_code_cache_in_custom_schemes.patch build_run_reclient_cfg_generator_after_chrome.patch -fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch refactor_expose_file_system_access_blocklist.patch feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch feat_enable_passing_exit_code_on_service_process_crash.patch diff --git a/patches/chromium/fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch b/patches/chromium/fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch deleted file mode 100644 index 7a4f24a8ab..0000000000 --- a/patches/chromium/fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch +++ /dev/null @@ -1,45 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Samuel Attard -Date: Tue, 13 Feb 2024 17:40:15 -0800 -Subject: fix: add support for skipping first 2 no-op refreshes in thumb cap - -Fixes a bug in the SCK thumbnail capturer, will be reported upstream for a hopefully -less hacky fix. - -The first refresh is "no windows yet, no thumbnails". -The second refresh is "we have windows, we queued the thumbnail requests" -The third refresh (the one we want) is "we have windows, and have thumbnail requests" - -This really isn't ideal at all, we need to refactor desktopCapturer (read completely re-implement it) -to use StartUpdating and handle the events instead of using the "get the list once" method. - -diff --git a/chrome/browser/media/webrtc/desktop_media_list.h b/chrome/browser/media/webrtc/desktop_media_list.h -index 09895ccfa99999e6e0ea24a3190d3f429ee40344..b1189cebeabe49971c0d0d4d013e6fe26e7df5a5 100644 ---- a/chrome/browser/media/webrtc/desktop_media_list.h -+++ b/chrome/browser/media/webrtc/desktop_media_list.h -@@ -149,6 +149,8 @@ class DesktopMediaList { - // source lists that need to be displayed independently from when the - // DesktopMediaList gains focus. - virtual void ShowDelegatedList() = 0; -+ -+ int skip_next_refresh_ = 0; - }; - - #endif // CHROME_BROWSER_MEDIA_WEBRTC_DESKTOP_MEDIA_LIST_H_ -diff --git a/chrome/browser/media/webrtc/desktop_media_list_base.cc b/chrome/browser/media/webrtc/desktop_media_list_base.cc -index f95b2230135dbcd6b19a31215d4f10be3481148c..9e1eb8423969e75d5ece0056690f651c1bb901cd 100644 ---- a/chrome/browser/media/webrtc/desktop_media_list_base.cc -+++ b/chrome/browser/media/webrtc/desktop_media_list_base.cc -@@ -232,7 +232,11 @@ uint32_t DesktopMediaListBase::GetImageHash(const gfx::Image& image) { - void DesktopMediaListBase::OnRefreshComplete() { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - DCHECK(refresh_callback_); -- std::move(refresh_callback_).Run(); -+ if (skip_next_refresh_ > 0) { -+ skip_next_refresh_--; -+ } else { -+ std::move(refresh_callback_).Run(); -+ } - } - - void DesktopMediaListBase::ScheduleNextRefresh() { diff --git a/shell/browser/api/electron_api_desktop_capturer.cc b/shell/browser/api/electron_api_desktop_capturer.cc index 509ff092d7..91748b2af0 100644 --- a/shell/browser/api/electron_api_desktop_capturer.cc +++ b/shell/browser/api/electron_api_desktop_capturer.cc @@ -9,13 +9,15 @@ #include #include "base/containers/flat_map.h" +#include "base/memory/raw_ptr.h" #include "base/strings/utf_string_conversions.h" -#include "base/threading/thread_restrictions.h" +#include "base/task/sequenced_task_runner.h" #include "chrome/browser/media/webrtc/desktop_capturer_wrapper.h" #include "chrome/browser/media/webrtc/desktop_media_list.h" +#include "chrome/browser/media/webrtc/desktop_media_list_observer.h" +#include "chrome/browser/media/webrtc/native_desktop_media_list.h" #include "chrome/browser/media/webrtc/thumbnail_capturer_mac.h" #include "chrome/browser/media/webrtc/window_icon_util.h" -#include "content/public/browser/browser_thread.h" #include "content/public/browser/desktop_capture.h" #include "gin/object_template_builder.h" #include "shell/browser/javascript_environment.h" @@ -223,48 +225,79 @@ namespace electron::api { gin::DeprecatedWrapperInfo DesktopCapturer::kWrapperInfo = { gin::kEmbedderNativeGin}; -DesktopCapturer::DesktopCapturer(v8::Isolate* isolate) {} +// Observer that forwards DesktopMediaListObserver events back to +// the owning DesktopCapturer, tagging them with the list type so +// the capturer knows which list fired. +class DesktopCapturer::ListObserver : public DesktopMediaListObserver { + public: + ListObserver(DesktopCapturer* capturer, + DesktopMediaList* list, + bool need_thumbnails) + : capturer_{capturer}, list_{list}, need_thumbnails_{need_thumbnails} {} + ~ListObserver() override = default; + + [[nodiscard]] bool IsReady() const { + if (!has_sources_) + return false; + + if (!need_thumbnails_) + return true; + + // Every source needs a non-empty thumbnail. + // Thumbnails finish one-at-a-time via tasks posted by the worker thread, + // so wait until every thumbnail arrives. + for (int i = 0; i < list_->GetSourceCount(); ++i) { + if (list_->GetSource(i).thumbnail.isNull()) + return false; + } + return true; + } + + private: + // Post OnListReady to the message loop when sources & thumbnails are done. + void MaybeNotifyReady() { + if (!IsReady() || notified_) + return; + notified_ = true; + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, + base::BindOnce(&DesktopCapturer::OnListReady, + capturer_->weak_ptr_factory_.GetWeakPtr(), list_.get())); + } + + // DesktopMediaListObserver: + void OnSourceAdded(int index) override { + has_sources_ = true; + MaybeNotifyReady(); + } + void OnSourceRemoved(int index) override {} + void OnSourceMoved(int old_index, int new_index) override {} + void OnSourceNameChanged(int index) override {} + void OnSourceThumbnailChanged(int index) override { MaybeNotifyReady(); } + void OnSourcePreviewChanged(size_t index) override {} + void OnDelegatedSourceListSelection() override { + // For delegated source lists (e.g. PipeWire), the selection event means + // the user picked a source. Treat as ready once we also have a thumbnail. + has_sources_ = true; + MaybeNotifyReady(); + } + void OnDelegatedSourceListDismissed() override { + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, base::BindOnce(&DesktopCapturer::HandleFailure, + capturer_->weak_ptr_factory_.GetWeakPtr())); + } + + raw_ptr capturer_; + raw_ptr list_; + bool need_thumbnails_ = false; + bool has_sources_ = false; + bool notified_ = false; +}; + +DesktopCapturer::DesktopCapturer() = default; DesktopCapturer::~DesktopCapturer() = default; -DesktopCapturer::DesktopListListener::DesktopListListener( - OnceCallback update_callback, - OnceCallback failure_callback, - bool skip_thumbnails) - : update_callback_(std::move(update_callback)), - failure_callback_(std::move(failure_callback)), - have_thumbnail_(skip_thumbnails) {} - -DesktopCapturer::DesktopListListener::~DesktopListListener() = default; - -void DesktopCapturer::DesktopListListener::OnDelegatedSourceListSelection() { - if (have_thumbnail_) { - content::GetUIThreadTaskRunner({})->PostTask(FROM_HERE, - std::move(update_callback_)); - } else { - have_selection_ = true; - } -} - -void DesktopCapturer::DesktopListListener::OnSourceThumbnailChanged(int index) { - if (have_selection_) { - // This is called every time a thumbnail is refreshed. Reset variable to - // ensure that the callback is not run again. - have_selection_ = false; - - // PipeWire returns a single source, so index is not relevant. - content::GetUIThreadTaskRunner({})->PostTask(FROM_HERE, - std::move(update_callback_)); - } else { - have_thumbnail_ = true; - } -} - -void DesktopCapturer::DesktopListListener::OnDelegatedSourceListDismissed() { - content::GetUIThreadTaskRunner({})->PostTask(FROM_HERE, - std::move(failure_callback_)); -} - void DesktopCapturer::StartHandling(bool capture_window, bool capture_screen, const gfx::Size& thumbnail_size, @@ -282,41 +315,8 @@ void DesktopCapturer::StartHandling(bool capture_window, // clear any existing captured sources. captured_sources_.clear(); - - if (capture_window && capture_screen) { - // Some capturers like PipeWire support a single capturer for both screens - // and windows. Use it if possible, treating both as window capture - std::unique_ptr desktop_capturer = - webrtc::DesktopCapturer::CreateGenericCapturer( - content::desktop_capture::CreateDesktopCaptureOptions()); - auto capturer = desktop_capturer ? std::make_unique( - std::move(desktop_capturer)) - : nullptr; - if (capturer && capturer->GetDelegatedSourceListController()) { - capture_screen_ = false; - capture_window_ = capture_window; - window_capturer_ = std::make_unique( - DesktopMediaList::Type::kWindow, std::move(capturer), true, true); - window_capturer_->SetThumbnailSize(thumbnail_size); - - OnceCallback update_callback = base::BindOnce( - &DesktopCapturer::UpdateSourcesList, weak_ptr_factory_.GetWeakPtr(), - window_capturer_.get()); - OnceCallback failure_callback = base::BindOnce( - &DesktopCapturer::HandleFailure, weak_ptr_factory_.GetWeakPtr()); - - window_listener_ = std::make_unique( - std::move(update_callback), std::move(failure_callback), - thumbnail_size.IsEmpty()); - window_capturer_->StartUpdating(window_listener_.get()); - - return; - } - } - - // Start listening for captured sources. - capture_window_ = capture_window; - capture_screen_ = capture_screen; + finished_ = false; + pending_lists_ = 0; #if BUILDFLAG(IS_MAC) if (!ui::TryPromptUserForScreenCapture()) { @@ -325,77 +325,72 @@ void DesktopCapturer::StartHandling(bool capture_window, } #endif - { - // Initialize the source list. - // Apply the new thumbnail size and restart capture. - if (capture_window) { - auto capturer = MakeWindowCapturer(); - if (capturer) { - window_capturer_ = std::make_unique( - DesktopMediaList::Type::kWindow, std::move(capturer), true, true); - window_capturer_->SetThumbnailSize(thumbnail_size); -#if BUILDFLAG(IS_MAC) - window_capturer_->skip_next_refresh_ = - ShouldUseThumbnailCapturerMac(DesktopMediaList::Type::kWindow) - ? thumbnail_size.IsEmpty() ? 1 : 2 - : 0; -#endif + const bool need_thumbnails = !thumbnail_size.IsEmpty(); + bool need_screen = capture_screen; - OnceCallback update_callback = base::BindOnce( - &DesktopCapturer::UpdateSourcesList, weak_ptr_factory_.GetWeakPtr(), - window_capturer_.get()); - - if (window_capturer_->IsSourceListDelegated()) { - OnceCallback failure_callback = base::BindOnce( - &DesktopCapturer::HandleFailure, weak_ptr_factory_.GetWeakPtr()); - window_listener_ = std::make_unique( - std::move(update_callback), std::move(failure_callback), - thumbnail_size.IsEmpty()); - window_capturer_->StartUpdating(window_listener_.get()); - } else { - window_capturer_->Update(std::move(update_callback), - /* refresh_thumbnails = */ true); - } + if (capture_window) { + // Some capturers like PipeWire support a single capturer for both screens + // and windows. Use it if possible, treating both as window capture. + std::unique_ptr capturer; + bool use_generic = false; + if (capture_screen) { + auto desktop_capturer = webrtc::DesktopCapturer::CreateGenericCapturer( + content::desktop_capture::CreateDesktopCaptureOptions()); + auto wrapper = desktop_capturer + ? std::make_unique( + std::move(desktop_capturer)) + : nullptr; + if (wrapper && wrapper->GetDelegatedSourceListController()) { + capturer = std::move(wrapper); + use_generic = true; + // Generic capturer handles both types as window capture. + need_screen = false; } } + if (!capturer) + capturer = MakeWindowCapturer(); - if (capture_screen) { - auto capturer = MakeScreenCapturer(); - if (capturer) { - screen_capturer_ = std::make_unique( - DesktopMediaList::Type::kScreen, std::move(capturer)); - screen_capturer_->SetThumbnailSize(thumbnail_size); -#if BUILDFLAG(IS_MAC) - screen_capturer_->skip_next_refresh_ = - ShouldUseThumbnailCapturerMac(DesktopMediaList::Type::kScreen) - ? thumbnail_size.IsEmpty() ? 1 : 2 - : 0; -#endif + if (capturer) { + window_capturer_ = std::make_unique( + DesktopMediaList::Type::kWindow, std::move(capturer), + /* add_current_process_windows = */ true, + /* auto_show_delegated_source_list = */ use_generic); + window_capturer_->SetThumbnailSize(thumbnail_size); + window_observer_ = std::make_unique( + this, window_capturer_.get(), need_thumbnails); + window_capturer_->StartUpdating(window_observer_.get()); + pending_lists_++; + } + } - OnceCallback update_callback = base::BindOnce( - &DesktopCapturer::UpdateSourcesList, weak_ptr_factory_.GetWeakPtr(), - screen_capturer_.get()); - - if (screen_capturer_->IsSourceListDelegated()) { - OnceCallback failure_callback = base::BindOnce( - &DesktopCapturer::HandleFailure, weak_ptr_factory_.GetWeakPtr()); - screen_listener_ = std::make_unique( - std::move(update_callback), std::move(failure_callback), - thumbnail_size.IsEmpty()); - screen_capturer_->StartUpdating(screen_listener_.get()); - } else { - screen_capturer_->Update(std::move(update_callback), - /* refresh_thumbnails = */ true); - } - } + if (need_screen) { + auto capturer = MakeScreenCapturer(); + if (capturer) { + screen_capturer_ = std::make_unique( + DesktopMediaList::Type::kScreen, std::move(capturer)); + screen_capturer_->SetThumbnailSize(thumbnail_size); + screen_observer_ = std::make_unique( + this, screen_capturer_.get(), need_thumbnails); + screen_capturer_->StartUpdating(screen_observer_.get()); + pending_lists_++; } } } -void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) { - if (capture_window_ && - list->GetMediaListType() == DesktopMediaList::Type::kWindow) { - capture_window_ = false; +void DesktopCapturer::OnListReady(DesktopMediaList* list) { + if (finished_) + return; + + CollectSourcesFrom(list); + + if (--pending_lists_ == 0) + HandleSuccess(); +} + +void DesktopCapturer::CollectSourcesFrom(DesktopMediaList* list) { + const auto type = list->GetMediaListType(); + + if (type == DesktopMediaList::Type::kWindow) { std::vector window_sources; window_sources.reserve(list->GetSourceCount()); for (int i = 0; i < list->GetSourceCount(); i++) { @@ -406,9 +401,7 @@ void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) { std::back_inserter(captured_sources_)); } - if (capture_screen_ && - list->GetMediaListType() == DesktopMediaList::Type::kScreen) { - capture_screen_ = false; + if (type == DesktopMediaList::Type::kScreen) { std::vector screen_sources; screen_sources.reserve(list->GetSourceCount()); for (int i = 0; i < list->GetSourceCount(); i++) { @@ -477,16 +470,19 @@ void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) { std::move(screen_sources.begin(), screen_sources.end(), std::back_inserter(captured_sources_)); } - - if (!capture_window_ && !capture_screen_) - HandleSuccess(); } void DesktopCapturer::HandleSuccess() { + if (finished_) + return; + finished_ = true; + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); v8::HandleScope scope(isolate); gin_helper::CallMethod(this, "_onfinished", captured_sources_); + window_observer_.reset(); + screen_observer_.reset(); screen_capturer_.reset(); window_capturer_.reset(); @@ -494,11 +490,17 @@ void DesktopCapturer::HandleSuccess() { } void DesktopCapturer::HandleFailure() { + if (finished_) + return; + finished_ = true; + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); v8::HandleScope scope(isolate); gin_helper::CallMethod(this, "_onerror", "Failed to get sources."); + window_observer_.reset(); + screen_observer_.reset(); screen_capturer_.reset(); window_capturer_.reset(); @@ -508,7 +510,7 @@ void DesktopCapturer::HandleFailure() { // static gin_helper::Handle DesktopCapturer::Create( v8::Isolate* isolate) { - auto handle = gin_helper::CreateHandle(isolate, new DesktopCapturer(isolate)); + auto handle = gin_helper::CreateHandle(isolate, new DesktopCapturer()); // Keep reference alive until capturing has finished. handle->Pin(isolate); diff --git a/shell/browser/api/electron_api_desktop_capturer.h b/shell/browser/api/electron_api_desktop_capturer.h index 459c97425d..c36a41f8bd 100644 --- a/shell/browser/api/electron_api_desktop_capturer.h +++ b/shell/browser/api/electron_api_desktop_capturer.h @@ -9,8 +9,7 @@ #include #include -#include "chrome/browser/media/webrtc/desktop_media_list_observer.h" -#include "chrome/browser/media/webrtc/native_desktop_media_list.h" +#include "chrome/browser/media/webrtc/desktop_media_list.h" #include "shell/common/gin_helper/pinnable.h" #include "shell/common/gin_helper/wrappable.h" @@ -23,8 +22,7 @@ namespace electron::api { class DesktopCapturer final : public gin_helper::DeprecatedWrappable, - public gin_helper::Pinnable, - private DesktopMediaListObserver { + public gin_helper::Pinnable { public: struct Source { DesktopMediaList::Source media_list_source; @@ -55,57 +53,24 @@ class DesktopCapturer final DesktopCapturer& operator=(const DesktopCapturer&) = delete; protected: - explicit DesktopCapturer(v8::Isolate* isolate); + DesktopCapturer(); ~DesktopCapturer() override; private: - // DesktopMediaListObserver: - void OnSourceAdded(int index) override {} - void OnSourceRemoved(int index) override {} - void OnSourceMoved(int old_index, int new_index) override {} - void OnSourceNameChanged(int index) override {} - void OnSourceThumbnailChanged(int index) override {} - void OnSourcePreviewChanged(size_t index) override {} - void OnDelegatedSourceListSelection() override {} - void OnDelegatedSourceListDismissed() override {} + class ListObserver; - using OnceCallback = base::OnceClosure; - - class DesktopListListener : public DesktopMediaListObserver { - public: - DesktopListListener(OnceCallback update_callback, - OnceCallback failure_callback, - bool skip_thumbnails); - ~DesktopListListener() override; - - protected: - void OnSourceAdded(int index) override {} - void OnSourceRemoved(int index) override {} - void OnSourceMoved(int old_index, int new_index) override {} - void OnSourceNameChanged(int index) override {} - void OnSourceThumbnailChanged(int index) override; - void OnSourcePreviewChanged(size_t index) override {} - void OnDelegatedSourceListSelection() override; - void OnDelegatedSourceListDismissed() override; - - private: - OnceCallback update_callback_; - OnceCallback failure_callback_; - bool have_selection_ = false; - bool have_thumbnail_ = false; - }; - - void UpdateSourcesList(DesktopMediaList* list); + void OnListReady(DesktopMediaList* list); + void CollectSourcesFrom(DesktopMediaList* list); void HandleFailure(); void HandleSuccess(); - std::unique_ptr window_listener_; - std::unique_ptr screen_listener_; + std::unique_ptr window_observer_; + std::unique_ptr screen_observer_; std::unique_ptr window_capturer_; std::unique_ptr screen_capturer_; std::vector captured_sources_; - bool capture_window_ = false; - bool capture_screen_ = false; + int pending_lists_ = 0; + bool finished_ = false; bool fetch_window_icons_ = false; #if BUILDFLAG(IS_WIN) bool using_directx_capturer_ = false;