Skip to content

fix: timing issue DCHECK crash in DesktopCapturer on macOS#50960

Merged
ckerr merged 1 commit into
mainfrom
fix/macos-dcheck-desktop-capturer-is-ready
Apr 14, 2026
Merged

fix: timing issue DCHECK crash in DesktopCapturer on macOS#50960
ckerr merged 1 commit into
mainfrom
fix/macos-dcheck-desktop-capturer-is-ready

Conversation

@ckerr
Copy link
Copy Markdown
Member

@ckerr ckerr commented Apr 11, 2026

Description of Change

Take @MarshallOfSound's suggestion in fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch to clean up the desktop capturer:

This [patch] 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.

I came across this while tracking down a recurring crash in macOS CI (example here) and found that refactoring DesktopCapturer to Sam's suggestion should also fix this crash.

  • Replace the one-shot Update() callback model with the continuous StartUpdating() observer model for NativeDesktopMediaList.

    This 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().

  • 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.


I asked Claude to outline this refactor's logic flow. Previously it was a one-shot subscription, which didn't fit Chromium's model very well. The new workflow is like this:

JS: getSources({types: ['window', 'screen']})
  │
  ▼
StartHandling()
  ├─ creates window_capturer_, screen_capturer_
  ├─ creates ListObserver for each (separate from DesktopCapturer)
  ├─ pending_lists_ = 2
  │
  ▼
window_capturer_->StartUpdating(observer)
  │  refresh_callback_ = ScheduleNextRefresh   ← always re-set!
  │  Refresh(true) → worker enumerates → UpdateSourcesList()
  │    │  OnSourceAdded(0) → has_sources_ = true, MaybeNotifyReady()
  │    │  OnSourceAdded(1) → MaybeNotifyReady() (notified_=false still, but...)
  │    │  ...all sources added...
  │    ▼
  │  OnRefreshComplete() → ScheduleNextRefresh()
  │    │  refresh_callback_ = ScheduleNextRefresh  ← repopulated immediately
  │    │  posts next Refresh() after delay
  │    ▼
  │  thumbnails arrive one-by-one → OnSourceThumbnailChanged(i)
  │    │  → MaybeNotifyReady() → IsReady()? all thumbnails non-null?
  │    │    not yet → returns
  │    │  ...last thumbnail arrives...
  │    │  → IsReady() = true, notified_ = false → notified_ = true
  │    │  → PostTask(OnListReady)    ← POSTED, not called synchronously
  │    ▼
  │  [next message loop iteration]
  │  OnListReady(window_list)
  │    ├─ CollectSourcesFrom() — all sources present, all thumbnails populated
  │    ├─ --pending_lists_ → 1 (not zero yet, wait for screen)
  │
  ▼
screen_capturer_->StartUpdating(observer)
  │  ...same flow...
  │  → PostTask(OnListReady)
  │
  ▼
OnListReady(screen_list)
  ├─ CollectSourcesFrom() — populates display_ids
  ├─ --pending_lists_ → 0
  ├─ HandleSuccess() — safe: called from posted task, nothing on stack
  │   ├─ emits _onfinished to JS
  │   ├─ destroys observers, then capturers (clean, no re-entrancy)
  │   └─ Unpin()

Checklist

Release Notes

Notes: Fixed DesktopCapturer crash on macOS.

@ckerr ckerr requested a review from a team as a code owner April 11, 2026 20:07
@ckerr ckerr added semver/patch backwards-compatible bug fixes target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. target/42-x-y PR should also be added to the "42-x-y" branch. labels Apr 11, 2026
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Apr 11, 2026
@ckerr ckerr marked this pull request as draft April 11, 2026 20:07
Copy link
Copy Markdown
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Woooo delete the patch 🥳 That said I think we should hold off on backporting this initially, this area is fiddly and likely to reveal timing issues when tested in Real Apps ™️

@ckerr ckerr added no-backport and removed target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. target/42-x-y PR should also be added to the "42-x-y" branch. labels Apr 11, 2026
@ckerr ckerr force-pushed the fix/macos-dcheck-desktop-capturer-is-ready branch 2 times, most recently from 2d15f42 to a7271bc Compare April 12, 2026 12:35
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
@ckerr ckerr force-pushed the fix/macos-dcheck-desktop-capturer-is-ready branch from a7271bc to 5931cd8 Compare April 12, 2026 12:39
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Apr 12, 2026
@ckerr ckerr marked this pull request as ready for review April 13, 2026 13:00
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Apr 13, 2026
@ckerr ckerr changed the title refactor: use StartUpdating() in desktopCapturer fix: timing issue DCHECK crash in DesktopCapturer on macOS Apr 13, 2026
Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Nice! This is a clean solution to the issue. Code changes LGTM, and I manually validated that it works on my macOS machine.

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Apr 14, 2026
@ckerr ckerr merged commit dad4ab6 into main Apr 14, 2026
166 of 173 checks passed
@ckerr ckerr deleted the fix/macos-dcheck-desktop-capturer-is-ready branch April 14, 2026 14:03
@release-clerk
Copy link
Copy Markdown

release-clerk Bot commented Apr 14, 2026

Release Notes Persisted

Fixed DesktopCapturer crash on macOS.

@trop
Copy link
Copy Markdown
Contributor

trop Bot commented May 5, 2026

@jkleinsc has manually backported this PR to "42-x-y", please check out #51506

@trop trop Bot added the in-flight/42-x-y label May 5, 2026
ckerr added a commit that referenced this pull request May 6, 2026
* 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
(cherry picked from commit dad4ab6)

* fix: do not block indefinitely on thumbnails in desktopCapturer (#51128)

* fix: do not block indefinitely on thumbnails in desktopCapturer

fixes dad4ab6 regression

* fix: build error

* fixup! fix: do not block indefinitely on thumbnails in desktopCapturer

chore: remove unnecessary code

* Update shell/browser/api/electron_api_desktop_capturer.cc

Co-authored-by: Niklas Wenzel <dev@nikwen.de>

---------

Co-authored-by: Niklas Wenzel <dev@nikwen.de>
(cherry picked from commit a1d28e6)

* fix: dangling `raw_ptr` regression in `DesktopCapturer` (#51158)

fix: dangling raw_ptr in desktopCapturer
(cherry picked from commit bef68b6)

* fix: do not pass a `DesktopMediaList* to DesktopCapturer::OnListReady()` (#51399)

refactor: do not pass a DesktopMediaList* to DesktopCapturer::OnListReady()

The list pointer was being used as a proxy for its type, so just pass
the type instead. This solves a lifecycle issue occurring in CI where
the callack can outlive the DesktopMediaList.

Sample error log:

[48471:0428/193441.269750:FATAL:base/allocator/partition_alloc_support.cc:798] Detected dangling raw_ptr in unretained with id=0x0000013c02e14378:
 Task trace:
 0   Electron Framework  0x000000012283a0ba electron::api::DesktopCapturer::ListObserver::MaybeNotifyReady() + 170
 1   Electron Framework  0x0000000133246dc5 NativeDesktopMediaList::Worker::OnRecurrentCaptureResult(webrtc::DesktopCapturer::Result, std::__Cr::unique_ptr<webrtc::DesktopFrame, std::__Cr::default_delete<webrtc::DesktopFrame>>, long) + 357
 2   Electron Framework  0x000000013328dbcf (anonymous namespace)::ScreenshotManagerCapturer::OnRecurrentCaptureTimer() + 1343
 Stack trace:
 0   Electron Framework  0x000000012ade42f2 base::debug::CollectStackTrace(base::span<void const*, 18446744073709551615ul, void const**>) + 18
 1   Electron Framework  0x000000012add00e1 base::debug::StackTrace::StackTrace(unsigned long) + 225
 2   Electron Framework  0x000000012ade978a base::allocator::UnretainedDanglingRawPtrDetectedCrash(unsigned long) + 90
 3   Electron Framework  0x000000012ae437f7 base::internal::RawPtrBackupRefImpl<true>::ReportIfDanglingInternal(unsigned long) + 391

(cherry picked from commit f8d0412)

---------

Co-authored-by: Charles Kerr <charles@charleskerr.com>
Co-authored-by: Niklas Wenzel <dev@nikwen.de>
@trop trop Bot added merged/42-x-y PR was merged to the "42-x-y" branch. and removed in-flight/42-x-y labels May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged/42-x-y PR was merged to the "42-x-y" branch. no-backport platform/macOS semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants