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

@@ -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

patches/chromium/fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch

@@ -1,45 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Samuel Attard <marshallofsound@electronjs.org>
-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() {

shell/browser/api/electron_api_desktop_capturer.cc

@@ -9,13 +9,15 @@
 #include <vector>
 
 #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<DesktopCapturer> capturer_;
+  raw_ptr<DesktopMediaList> 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();
-
+  finished_ = false;
-  if (capture_window && capture_screen) {
+  pending_lists_ = 0;
-    // 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<webrtc::DesktopCapturer> desktop_capturer =
-        webrtc::DesktopCapturer::CreateGenericCapturer(
-            content::desktop_capture::CreateDesktopCaptureOptions());
-    auto capturer = desktop_capturer ? std::make_unique<DesktopCapturerWrapper>(
-                                           std::move(desktop_capturer))
-                                     : nullptr;
-    if (capturer && capturer->GetDelegatedSourceListController()) {
-      capture_screen_ = false;
-      capture_window_ = capture_window;
-      window_capturer_ = std::make_unique<NativeDesktopMediaList>(
-          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<DesktopListListener>(
-          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;
 
 #if BUILDFLAG(IS_MAC)
   if (!ui::TryPromptUserForScreenCapture()) {
@@ -325,77 +325,72 @@ void DesktopCapturer::StartHandling(bool capture_window,
   }
 #endif
 
-  {
+  const bool need_thumbnails = !thumbnail_size.IsEmpty();
-    // Initialize the source list.
+  bool need_screen = capture_screen;
-    // Apply the new thumbnail size and restart capture.
-    if (capture_window) {
-      auto capturer = MakeWindowCapturer();
-      if (capturer) {
-        window_capturer_ = std::make_unique<NativeDesktopMediaList>(
-            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
 
-        OnceCallback update_callback = base::BindOnce(
+  if (capture_window) {
-            &DesktopCapturer::UpdateSourcesList, weak_ptr_factory_.GetWeakPtr(),
+    // Some capturers like PipeWire support a single capturer for both screens
-            window_capturer_.get());
+    // and windows. Use it if possible, treating both as window capture.
-
+    std::unique_ptr<ThumbnailCapturer> capturer;
-        if (window_capturer_->IsSourceListDelegated()) {
+    bool use_generic = false;
-          OnceCallback failure_callback = base::BindOnce(
+    if (capture_screen) {
-              &DesktopCapturer::HandleFailure, weak_ptr_factory_.GetWeakPtr());
+      auto desktop_capturer = webrtc::DesktopCapturer::CreateGenericCapturer(
-          window_listener_ = std::make_unique<DesktopListListener>(
+          content::desktop_capture::CreateDesktopCaptureOptions());
-              std::move(update_callback), std::move(failure_callback),
+      auto wrapper = desktop_capturer
-              thumbnail_size.IsEmpty());
+                         ? std::make_unique<DesktopCapturerWrapper>(
-          window_capturer_->StartUpdating(window_listener_.get());
+                               std::move(desktop_capturer))
-        } else {
+                         : nullptr;
-          window_capturer_->Update(std::move(update_callback),
+      if (wrapper && wrapper->GetDelegatedSourceListController()) {
-                                   /* refresh_thumbnails = */ true);
+        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) {
+    if (capturer) {
-      auto capturer = MakeScreenCapturer();
+      window_capturer_ = std::make_unique<NativeDesktopMediaList>(
-      if (capturer) {
+          DesktopMediaList::Type::kWindow, std::move(capturer),
-        screen_capturer_ = std::make_unique<NativeDesktopMediaList>(
+          /* add_current_process_windows = */ true,
-            DesktopMediaList::Type::kScreen, std::move(capturer));
+          /* auto_show_delegated_source_list = */ use_generic);
-        screen_capturer_->SetThumbnailSize(thumbnail_size);
+      window_capturer_->SetThumbnailSize(thumbnail_size);
-#if BUILDFLAG(IS_MAC)
+      window_observer_ = std::make_unique<ListObserver>(
-        screen_capturer_->skip_next_refresh_ =
+          this, window_capturer_.get(), need_thumbnails);
-            ShouldUseThumbnailCapturerMac(DesktopMediaList::Type::kScreen)
+      window_capturer_->StartUpdating(window_observer_.get());
-                ? thumbnail_size.IsEmpty() ? 1 : 2
+      pending_lists_++;
-                : 0;
+    }
-#endif
+  }
 
-        OnceCallback update_callback = base::BindOnce(
+  if (need_screen) {
-            &DesktopCapturer::UpdateSourcesList, weak_ptr_factory_.GetWeakPtr(),
+    auto capturer = MakeScreenCapturer();
-            screen_capturer_.get());
+    if (capturer) {
-
+      screen_capturer_ = std::make_unique<NativeDesktopMediaList>(
-        if (screen_capturer_->IsSourceListDelegated()) {
+          DesktopMediaList::Type::kScreen, std::move(capturer));
-          OnceCallback failure_callback = base::BindOnce(
+      screen_capturer_->SetThumbnailSize(thumbnail_size);
-              &DesktopCapturer::HandleFailure, weak_ptr_factory_.GetWeakPtr());
+      screen_observer_ = std::make_unique<ListObserver>(
-          screen_listener_ = std::make_unique<DesktopListListener>(
+          this, screen_capturer_.get(), need_thumbnails);
-              std::move(update_callback), std::move(failure_callback),
+      screen_capturer_->StartUpdating(screen_observer_.get());
-              thumbnail_size.IsEmpty());
+      pending_lists_++;
-          screen_capturer_->StartUpdating(screen_listener_.get());
-        } else {
-          screen_capturer_->Update(std::move(update_callback),
-                                   /* refresh_thumbnails = */ true);
-        }
-      }
     }
   }
 }
 
-void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) {
+void DesktopCapturer::OnListReady(DesktopMediaList* list) {
-  if (capture_window_ &&
+  if (finished_)
-      list->GetMediaListType() == DesktopMediaList::Type::kWindow) {
+    return;
-    capture_window_ = false;
+
+  CollectSourcesFrom(list);
+
+  if (--pending_lists_ == 0)
+    HandleSuccess();
+}
+
+void DesktopCapturer::CollectSourcesFrom(DesktopMediaList* list) {
+  const auto type = list->GetMediaListType();
+
+  if (type == DesktopMediaList::Type::kWindow) {
     std::vector<DesktopCapturer::Source> 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_ &&
+  if (type == DesktopMediaList::Type::kScreen) {
-      list->GetMediaListType() == DesktopMediaList::Type::kScreen) {
-    capture_screen_ = false;
     std::vector<DesktopCapturer::Source> 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> 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);

shell/browser/api/electron_api_desktop_capturer.h

@@ -9,8 +9,7 @@
 #include <string>
 #include <vector>
 
-#include "chrome/browser/media/webrtc/desktop_media_list_observer.h"
+#include "chrome/browser/media/webrtc/desktop_media_list.h"
-#include "chrome/browser/media/webrtc/native_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<DesktopCapturer>,
-      public gin_helper::Pinnable<DesktopCapturer>,
+      public gin_helper::Pinnable<DesktopCapturer> {
-      private DesktopMediaListObserver {
  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:
+  class ListObserver;
-  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 {}
 
-  using OnceCallback = base::OnceClosure;
+  void OnListReady(DesktopMediaList* list);
-
+  void CollectSourcesFrom(DesktopMediaList* list);
-  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 HandleFailure();
   void HandleSuccess();
 
-  std::unique_ptr<DesktopListListener> window_listener_;
+  std::unique_ptr<ListObserver> window_observer_;
-  std::unique_ptr<DesktopListListener> screen_listener_;
+  std::unique_ptr<ListObserver> screen_observer_;
   std::unique_ptr<DesktopMediaList> window_capturer_;
   std::unique_ptr<DesktopMediaList> screen_capturer_;
   std::vector<DesktopCapturer::Source> captured_sources_;
-  bool capture_window_ = false;
+  int pending_lists_ = 0;
-  bool capture_screen_ = false;
+  bool finished_ = false;
   bool fetch_window_icons_ = false;
 #if BUILDFLAG(IS_WIN)
   bool using_directx_capturer_ = false;