fix: timing issue DCHECK crash in DesktopCapturer on macOS#50960
Merged
Conversation
Member
MarshallOfSound
left a comment
There was a problem hiding this comment.
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 ™️
2d15f42 to
a7271bc
Compare
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
a7271bc to
5931cd8
Compare
StartUpdating() in desktopCapturer
dsanders11
approved these changes
Apr 14, 2026
Member
dsanders11
left a comment
There was a problem hiding this comment.
Nice! This is a clean solution to the issue. Code changes LGTM, and I manually validated that it works on my macOS machine.
|
Release Notes Persisted
|
This was referenced Apr 14, 2026
This was referenced Apr 19, 2026
Contributor
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
Take @MarshallOfSound's suggestion in
fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patchto clean up the desktop capturer: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 inUpdateSourceThumbnail(), 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 becauserefresh_callback_is repopulated viaScheduleNextRefresh().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:
Checklist
npm testpassesRelease Notes
Notes: Fixed DesktopCapturer crash on macOS.