Skip to content

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

Merged
ckerr merged 4 commits into
mainfrom
fix/test-regression-f35122b-mediaDevice
Apr 19, 2026
Merged

fix: do not block indefinitely on thumbnails in desktopCapturer#51128
ckerr merged 4 commits into
mainfrom
fix/test-regression-f35122b-mediaDevice

Conversation

@ckerr
Copy link
Copy Markdown
Member

@ckerr ckerr commented Apr 17, 2026

Description of Change

Fixes #51125.
Fixes #51124.

Fixes a desktopCapturer regression introduced in dad4ab6. Only affects main.

Checklist

Release Notes

Notes: Fixed desktopCapturer.getSources() hanging on macOS.

@ckerr ckerr added semver/patch backwards-compatible bug fixes no-backport labels Apr 17, 2026
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Apr 17, 2026
@ckerr ckerr marked this pull request as draft April 17, 2026 15:18
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Apr 17, 2026

The Fiddle gist works for me on this patch.

e test -g desktopCapturer almost works for me:

ok 1 desktopCapturer should return a non-empty array of sources
ok 2 desktopCapturer throws an error for invalid options
ok 3 desktopCapturer does not throw an error when called more than once (regression)
ok 4 desktopCapturer responds to subsequent calls of different options
ok 5 desktopCapturer returns an empty display_id for window sources
not ok 6 desktopCapturer returns display_ids matching the Screen API
  expected '3' to equal '1'
  AssertionError: expected '3' to equal '1'
      at Context.<anonymous> (electron/spec/api-desktop-capturer-spec.ts:65:36)
not ok 7 desktopCapturer enabling thumbnail should return non-empty images
  expected false to be true
  AssertionError: expected false to be true
      at Context.<anonymous> (electron/spec/api-desktop-capturer-spec.ts:84:61)
ok 8 desktopCapturer disabling thumbnail should return empty images
ok 9 desktopCapturer getMediaSourceId should match DesktopCapturerSource.id
ok 10 desktopCapturer getSources should not incorrectly duplicate window_id
ok 11 desktopCapturer does not affect window resizable state
ok 12 desktopCapturer moveAbove should move the window at the requested place
ok 13 screen module screen.getAllDisplays returns displays with IDs matching desktopCapturer source display IDs

The first failure is due to having external screens connected. That's unrelated.

The second failure also happens for me on Electron 41.2.0. It's my password manager that won't let Electron record its window.

Comment thread shell/browser/api/electron_api_desktop_capturer.cc Outdated
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Apr 17, 2026

Hm, we're still getting test failures in CI:

Logs

Failure in test: "desktopCapturer should return a non-empty array of sources"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (1/3)...
Failure in test: "desktopCapturer should return a non-empty array of sources"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (2/3)...
Failure in test: "desktopCapturer should return a non-empty array of sources"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (3/3)...
not ok 105 desktopCapturer should return a non-empty array of sources
  Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
      at listOnTimeout (node:internal/timers:605:17)
      at processTimers (node:internal/timers:541:7)
ok 106 desktopCapturer throws an error for invalid options
Failure in test: "desktopCapturer does not throw an error when called more than once (regression)"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (1/3)...
Failure in test: "desktopCapturer does not throw an error when called more than once (regression)"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (2/3)...
Failure in test: "desktopCapturer does not throw an error when called more than once (regression)"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (3/3)...
not ok 107 desktopCapturer does not throw an error when called more than once (regression)
  Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
      at listOnTimeout (node:internal/timers:605:17)
      at processTimers (node:internal/timers:541:7)
Failure in test: "desktopCapturer responds to subsequent calls of different options"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (1/3)...
Failure in test: "desktopCapturer responds to subsequent calls of different options"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (2/3)...
Failure in test: "desktopCapturer responds to subsequent calls of different options"
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
    at createTimeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/errors.js:498:15)
    at Test.Runnable._timeoutError (/Users/runner/work/electron/electron/src/electron/spec/node_modules/mocha/lib/runnable.js:429:10)
Retrying test (3/3)...
not ok 108 desktopCapturer responds to subsequent calls of different options
  Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/electron/electron/src/electron/spec/api-desktop-capturer-spec.ts)
      at listOnTimeout (node:internal/timers:605:17)
      at processTimers (node:internal/timers:541:7)
ok 109 desktopCapturer returns an empty display_id for window sources
ok 110 desktopCapturer returns display_ids matching the Screen API
ok 111 desktopCapturer enabling thumbnail should return non-empty images
ok 112 desktopCapturer disabling thumbnail should return empty images
ok 113 desktopCapturer getMediaSourceId should match DesktopCapturerSource.id
ok 114 desktopCapturer getSources should not incorrectly duplicate window_id
ok 115 desktopCapturer does not affect window resizable state
ok 116 desktopCapturer moveAbove should move the window at the requested place

Comment thread shell/browser/api/electron_api_desktop_capturer.cc Outdated
@ckerr ckerr force-pushed the fix/test-regression-f35122b-mediaDevice branch from 05b54bb to b70aaf6 Compare April 17, 2026 17:41
Comment thread shell/browser/api/electron_api_desktop_capturer.cc Outdated
Comment thread shell/browser/api/electron_api_desktop_capturer.cc
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Apr 17, 2026

Local test results on the most recent commit:

ok 1 desktopCapturer should return a non-empty array of sources
ok 2 desktopCapturer throws an error for invalid options
ok 3 desktopCapturer does not throw an error when called more than once (regression)
ok 4 desktopCapturer responds to subsequent calls of different options
ok 5 desktopCapturer returns an empty display_id for window sources
not ok 6 desktopCapturer returns display_ids matching the Screen API
  expected '3' to equal '1'
  AssertionError: expected '3' to equal '1'
      at Context.<anonymous> (electron/spec/api-desktop-capturer-spec.ts:65:36)
not ok 7 desktopCapturer enabling thumbnail should return non-empty images
  expected false to be true
  AssertionError: expected false to be true
      at Context.<anonymous> (electron/spec/api-desktop-capturer-spec.ts:84:61)
ok 8 desktopCapturer disabling thumbnail should return empty images
ok 9 desktopCapturer getMediaSourceId should match DesktopCapturerSource.id
ok 10 desktopCapturer getSources should not incorrectly duplicate window_id
ok 11 desktopCapturer does not affect window resizable state
ok 12 desktopCapturer moveAbove should move the window at the requested place
ok 13 screen module screen.getAllDisplays returns displays with IDs matching desktopCapturer source display IDs
# tests 13
# pass 11
# fail 2

I do notice that when I have my password manager open, tests take a lot longer to run now, given the 3-second timeout. (Maybe that's something we have to accept.)

@nikwen
Copy link
Copy Markdown
Member

nikwen commented Apr 17, 2026

The mas tests ran through successfully!

ok 105 desktopCapturer should return a non-empty array of sources
ok 106 desktopCapturer throws an error for invalid options
ok 107 desktopCapturer does not throw an error when called more than once (regression)
ok 108 desktopCapturer responds to subsequent calls of different options
ok 109 desktopCapturer returns an empty display_id for window sources
ok 110 desktopCapturer returns display_ids matching the Screen API
ok 111 desktopCapturer enabling thumbnail should return non-empty images
ok 112 desktopCapturer disabling thumbnail should return empty images
ok 113 desktopCapturer getMediaSourceId should match DesktopCapturerSource.id
ok 114 desktopCapturer getSources should not incorrectly duplicate window_id
ok 115 desktopCapturer does not affect window resizable state
ok 116 desktopCapturer moveAbove should move the window at the requested place
ok 117 desktopCapturer fetchWindowIcons should find the test window in the list of captured sources
ok 118 desktopCapturer fetchWindowIcons should return a non-null appIcon for the captured window
ok 119 desktopCapturer fetchWindowIcons should return an appIcon that is not an empty image
ok 120 desktopCapturer fetchWindowIcons should return an appIcon that encodes to a valid PNG data URL
ok 121 desktopCapturer fetchWindowIcons should return an appIcon with dimensions greater than 0x0 pixels

Copy link
Copy Markdown
Member

@nikwen nikwen left a comment

Choose a reason for hiding this comment

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

Just reviewed the code. Looks good to me! 👍

Comment thread shell/browser/api/electron_api_desktop_capturer.cc Outdated
@ckerr ckerr marked this pull request as ready for review April 17, 2026 21:46
Co-authored-by: Niklas Wenzel <dev@nikwen.de>
@ckerr
Copy link
Copy Markdown
Member Author

ckerr commented Apr 18, 2026

@nikwen thanks for all the feedback, it was a great help!

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Apr 18, 2026
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Apr 18, 2026

@ckerr Thanks for working on it! If you could merge it, I'd appreciate that.

@ckerr ckerr merged commit a1d28e6 into main Apr 19, 2026
69 checks passed
@release-clerk
Copy link
Copy Markdown

release-clerk Bot commented Apr 19, 2026

Release Notes Persisted

Fixed desktopCapturer.getSources() hanging on macOS.

@ckerr ckerr deleted the fix/test-regression-f35122b-mediaDevice branch April 19, 2026 18:48
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-backport semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

desktopCapturer.getSources() hangs indefinitely on macOS desktopCapturer tests are flaky on macos-x64

2 participants