Add DesktopCaptureOption enumerate_current_process_windows to avoid hang
Enumerating windows owned by the current process on Windows has some
complications due to the GetWindowText*() APIs potentially causing a
deadlock. The APIs will send messages to the window's message loop, and
if the message loop is waiting on this operation we will enter a
deadlock.
I previously put in a mitigation for this [1] which brought the
incidence rate down by an order of magnitude, but we are still seeing
this issue fairly frequently.
So, I've added DesktopCaptureOption enumerate_current_process_windows
which allows consumers to avoid this issue completely by ignoring
these potentially problematic windows.
By default the flag is set to true which equates with the current
behavior, consumers can set the flag to false to get the new behavior.
I've also updated all the capturers that enumerate windows on Windows
to respect the option.
[1] https://webrtc-review.googlesource.com/c/src/+/195365
Bug: chromium:1152841
Change-Id: I0e0d868957d6fbe1e607a440b3a909d005c93ccf
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219380
Commit-Queue: Austin Orion <auorion@microsoft.com>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#34191}
diff --git a/modules/desktop_capture/cropping_window_capturer_win.cc b/modules/desktop_capture/cropping_window_capturer_win.cc
index de36adb..c52ca13 100644
--- a/modules/desktop_capture/cropping_window_capturer_win.cc
+++ b/modules/desktop_capture/cropping_window_capturer_win.cc
@@ -130,6 +130,8 @@
public:
explicit CroppingWindowCapturerWin(const DesktopCaptureOptions& options)
: CroppingWindowCapturer(options),
+ enumerate_current_process_windows_(
+ options.enumerate_current_process_windows()),
full_screen_window_detector_(options.full_screen_window_detector()) {}
void CaptureFrame() override;
@@ -148,6 +150,8 @@
WindowCaptureHelperWin window_capture_helper_;
+ bool enumerate_current_process_windows_;
+
rtc::scoped_refptr<FullScreenWindowDetector> full_screen_window_detector_;
};
@@ -164,7 +168,12 @@
// it uses responsiveness check which could lead to performance
// issues.
SourceList result;
- if (!webrtc::GetWindowList(GetWindowListFlags::kNone, &result))
+ int window_list_flags =
+ enumerate_current_process_windows_
+ ? GetWindowListFlags::kNone
+ : GetWindowListFlags::kIgnoreCurrentProcessWindows;
+
+ if (!webrtc::GetWindowList(window_list_flags, &result))
return false;
// Filter out windows not visible on current desktop
diff --git a/modules/desktop_capture/desktop_capture_options.h b/modules/desktop_capture/desktop_capture_options.h
index 4cb19a5..a693803 100644
--- a/modules/desktop_capture/desktop_capture_options.h
+++ b/modules/desktop_capture/desktop_capture_options.h
@@ -98,6 +98,24 @@
}
#if defined(WEBRTC_WIN)
+ // Enumerating windows owned by the current process on Windows has some
+ // complications due to |GetWindowText*()| APIs potentially causing a
+ // deadlock (see the comments in the |GetWindowListHandler()| function in
+ // window_capture_utils.cc for more details on the deadlock).
+ // To avoid this issue, consumers can either ensure that the thread that runs
+ // their message loop never waits on |GetSourceList()|, or they can set this
+ // flag to false which will prevent windows running in the current process
+ // from being enumerated and included in the results. Consumers can still
+ // provide the WindowId for their own windows to |SelectSource()| and capture
+ // them.
+ bool enumerate_current_process_windows() const {
+ return enumerate_current_process_windows_;
+ }
+ void set_enumerate_current_process_windows(
+ bool enumerate_current_process_windows) {
+ enumerate_current_process_windows_ = enumerate_current_process_windows;
+ }
+
bool allow_use_magnification_api() const {
return allow_use_magnification_api_;
}
@@ -158,6 +176,7 @@
rtc::scoped_refptr<FullScreenWindowDetector> full_screen_window_detector_;
#if defined(WEBRTC_WIN)
+ bool enumerate_current_process_windows_ = true;
bool allow_use_magnification_api_ = false;
bool allow_directx_capturer_ = false;
bool allow_cropping_window_capturer_ = false;
diff --git a/modules/desktop_capture/win/wgc_capturer_win.cc b/modules/desktop_capture/win/wgc_capturer_win.cc
index 88859b6..442c827a 100644
--- a/modules/desktop_capture/win/wgc_capturer_win.cc
+++ b/modules/desktop_capture/win/wgc_capturer_win.cc
@@ -57,7 +57,8 @@
const DesktopCaptureOptions& options) {
return std::make_unique<WgcCapturerWin>(
std::make_unique<WgcWindowSourceFactory>(),
- std::make_unique<WindowEnumerator>());
+ std::make_unique<WindowEnumerator>(
+ options.enumerate_current_process_windows()));
}
// static
diff --git a/modules/desktop_capture/win/wgc_capturer_win.h b/modules/desktop_capture/win/wgc_capturer_win.h
index 9d461d3..58f3fc3 100644
--- a/modules/desktop_capture/win/wgc_capturer_win.h
+++ b/modules/desktop_capture/win/wgc_capturer_win.h
@@ -38,7 +38,8 @@
class WindowEnumerator final : public SourceEnumerator {
public:
- WindowEnumerator() = default;
+ explicit WindowEnumerator(bool enumerate_current_process_windows)
+ : enumerate_current_process_windows_(enumerate_current_process_windows) {}
WindowEnumerator(const WindowEnumerator&) = delete;
WindowEnumerator& operator=(const WindowEnumerator&) = delete;
@@ -48,12 +49,13 @@
bool FindAllSources(DesktopCapturer::SourceList* sources) override {
// WGC fails to capture windows with the WS_EX_TOOLWINDOW style, so we
// provide it as a filter to ensure windows with the style are not returned.
- return window_capture_helper_.EnumerateCapturableWindows(sources,
- WS_EX_TOOLWINDOW);
+ return window_capture_helper_.EnumerateCapturableWindows(
+ sources, enumerate_current_process_windows_, WS_EX_TOOLWINDOW);
}
private:
WindowCaptureHelperWin window_capture_helper_;
+ bool enumerate_current_process_windows_;
};
class ScreenEnumerator final : public SourceEnumerator {
diff --git a/modules/desktop_capture/win/window_capture_utils.cc b/modules/desktop_capture/win/window_capture_utils.cc
index 7c5cc70..aaaef0a 100644
--- a/modules/desktop_capture/win/window_capture_utils.cc
+++ b/modules/desktop_capture/win/window_capture_utils.cc
@@ -32,26 +32,21 @@
DesktopCapturer::SourceList* result)
: ignore_untitled(flags & GetWindowListFlags::kIgnoreUntitled),
ignore_unresponsive(flags & GetWindowListFlags::kIgnoreUnresponsive),
+ ignore_current_process_windows(
+ flags & GetWindowListFlags::kIgnoreCurrentProcessWindows),
ex_style_filters(ex_style_filters),
result(result) {}
const bool ignore_untitled;
const bool ignore_unresponsive;
+ const bool ignore_current_process_windows;
const LONG ex_style_filters;
DesktopCapturer::SourceList* const result;
};
-// If a window is owned by the current process and unresponsive, then making a
-// blocking call such as GetWindowText may lead to a deadlock.
-//
-// https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtexta#remarks
-bool CanSafelyMakeBlockingCalls(HWND hwnd) {
+bool IsWindowOwnedByCurrentProcess(HWND hwnd) {
DWORD process_id;
GetWindowThreadProcessId(hwnd, &process_id);
- if (process_id != GetCurrentProcessId() || IsWindowResponding(hwnd)) {
- return true;
- }
-
- return false;
+ return process_id == GetCurrentProcessId();
}
BOOL CALLBACK GetWindowListHandler(HWND hwnd, LPARAM param) {
@@ -85,11 +80,26 @@
window.id = reinterpret_cast<WindowId>(hwnd);
// GetWindowText* are potentially blocking operations if |hwnd| is
- // owned by the current process, and can lead to a deadlock if the message
- // pump is waiting on this thread. If we've filtered out unresponsive
- // windows, this is not a concern, but otherwise we need to check if we can
- // safely make blocking calls.
- if (params->ignore_unresponsive || CanSafelyMakeBlockingCalls(hwnd)) {
+ // owned by the current process. The APIs will send messages to the window's
+ // message loop, and if the message loop is waiting on this operation we will
+ // enter a deadlock.
+ // https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtexta#remarks
+ //
+ // To help consumers avoid this, there is a DesktopCaptureOption to ignore
+ // windows owned by the current process. Consumers should either ensure that
+ // the thread running their message loop never waits on this operation, or use
+ // the option to exclude these windows from the source list.
+ bool owned_by_current_process = IsWindowOwnedByCurrentProcess(hwnd);
+ if (owned_by_current_process && params->ignore_current_process_windows) {
+ return TRUE;
+ }
+
+ // Even if consumers request to enumerate windows owned by the current
+ // process, we should not call GetWindowText* on unresponsive windows owned by
+ // the current process because we will hang. Unfortunately, we could still
+ // hang if the window becomes unresponsive after this check, hence the option
+ // to avoid these completely.
+ if (!owned_by_current_process || IsWindowResponding(hwnd)) {
const size_t kTitleLength = 500;
WCHAR window_title[kTitleLength] = L"";
if (GetWindowTextLength(hwnd) != 0 &&
@@ -445,10 +455,15 @@
bool WindowCaptureHelperWin::EnumerateCapturableWindows(
DesktopCapturer::SourceList* results,
+ bool enumerate_current_process_windows,
LONG ex_style_filters) {
- if (!webrtc::GetWindowList((GetWindowListFlags::kIgnoreUntitled |
- GetWindowListFlags::kIgnoreUnresponsive),
- results, ex_style_filters)) {
+ int flags = (GetWindowListFlags::kIgnoreUntitled |
+ GetWindowListFlags::kIgnoreUnresponsive);
+ if (!enumerate_current_process_windows) {
+ flags |= GetWindowListFlags::kIgnoreCurrentProcessWindows;
+ }
+
+ if (!webrtc::GetWindowList(flags, results, ex_style_filters)) {
return false;
}
diff --git a/modules/desktop_capture/win/window_capture_utils.h b/modules/desktop_capture/win/window_capture_utils.h
index 11b2c2c..a6a295d 100644
--- a/modules/desktop_capture/win/window_capture_utils.h
+++ b/modules/desktop_capture/win/window_capture_utils.h
@@ -78,6 +78,7 @@
kNone = 0x00,
kIgnoreUntitled = 1 << 0,
kIgnoreUnresponsive = 1 << 1,
+ kIgnoreCurrentProcessWindows = 1 << 2,
};
// Retrieves the list of top-level windows on the screen.
@@ -85,7 +86,8 @@
// - Those that are invisible or minimized.
// - Program Manager & Start menu.
// - [with kIgnoreUntitled] windows with no title.
-// - [with kIgnoreUnresponsive] windows that unresponsive.
+// - [with kIgnoreUnresponsive] windows that are unresponsive.
+// - [with kIgnoreCurrentProcessWindows] windows owned by the current process.
// - Any windows with extended styles that match |ex_style_filters|.
// Returns false if native APIs failed.
bool GetWindowList(int flags,
@@ -115,6 +117,7 @@
// extended window styles (e.g. WS_EX_TOOLWINDOW) and prevent windows that
// match from being included in |results|.
bool EnumerateCapturableWindows(DesktopCapturer::SourceList* results,
+ bool enumerate_current_process_windows,
LONG ex_style_filters = 0);
private:
diff --git a/modules/desktop_capture/win/window_capture_utils_unittest.cc b/modules/desktop_capture/win/window_capture_utils_unittest.cc
index 52f6714..4b426fc 100644
--- a/modules/desktop_capture/win/window_capture_utils_unittest.cc
+++ b/modules/desktop_capture/win/window_capture_utils_unittest.cc
@@ -137,4 +137,18 @@
DestroyTestWindow(info);
}
+TEST(WindowCaptureUtilsTest, IgnoreCurrentProcessWindows) {
+ WindowInfo info = CreateTestWindow(kWindowTitle);
+ DesktopCapturer::SourceList window_list;
+ ASSERT_TRUE(GetWindowList(GetWindowListFlags::kIgnoreCurrentProcessWindows,
+ &window_list));
+ EXPECT_EQ(std::find_if(window_list.begin(), window_list.end(),
+ [&info](DesktopCapturer::Source window) {
+ return reinterpret_cast<HWND>(window.id) ==
+ info.hwnd;
+ }),
+ window_list.end());
+ DestroyTestWindow(info);
+}
+
} // namespace webrtc
diff --git a/modules/desktop_capture/win/window_capturer_win_gdi.cc b/modules/desktop_capture/win/window_capturer_win_gdi.cc
index 277c02e..25677e9 100644
--- a/modules/desktop_capture/win/window_capturer_win_gdi.cc
+++ b/modules/desktop_capture/win/window_capturer_win_gdi.cc
@@ -95,11 +95,14 @@
return TRUE;
}
-WindowCapturerWinGdi::WindowCapturerWinGdi() {}
+WindowCapturerWinGdi::WindowCapturerWinGdi(
+ bool enumerate_current_process_windows)
+ : enumerate_current_process_windows_(enumerate_current_process_windows) {}
WindowCapturerWinGdi::~WindowCapturerWinGdi() {}
bool WindowCapturerWinGdi::GetSourceList(SourceList* sources) {
- if (!window_capture_helper_.EnumerateCapturableWindows(sources))
+ if (!window_capture_helper_.EnumerateCapturableWindows(
+ sources, enumerate_current_process_windows_))
return false;
std::map<HWND, DesktopSize> new_map;
@@ -350,7 +353,8 @@
if (!owned_windows_.empty()) {
if (!owned_window_capturer_) {
- owned_window_capturer_ = std::make_unique<WindowCapturerWinGdi>();
+ owned_window_capturer_ = std::make_unique<WindowCapturerWinGdi>(
+ enumerate_current_process_windows_);
}
// Owned windows are stored in top-down z-order, so this iterates in
@@ -389,7 +393,8 @@
// static
std::unique_ptr<DesktopCapturer> WindowCapturerWinGdi::CreateRawWindowCapturer(
const DesktopCaptureOptions& options) {
- return std::unique_ptr<DesktopCapturer>(new WindowCapturerWinGdi());
+ return std::unique_ptr<DesktopCapturer>(
+ new WindowCapturerWinGdi(options.enumerate_current_process_windows()));
}
} // namespace webrtc
diff --git a/modules/desktop_capture/win/window_capturer_win_gdi.h b/modules/desktop_capture/win/window_capturer_win_gdi.h
index c954c23..5091458 100644
--- a/modules/desktop_capture/win/window_capturer_win_gdi.h
+++ b/modules/desktop_capture/win/window_capturer_win_gdi.h
@@ -24,7 +24,7 @@
class WindowCapturerWinGdi : public DesktopCapturer {
public:
- WindowCapturerWinGdi();
+ explicit WindowCapturerWinGdi(bool enumerate_current_process_windows);
// Disallow copy and assign
WindowCapturerWinGdi(const WindowCapturerWinGdi&) = delete;
@@ -61,6 +61,8 @@
WindowCaptureHelperWin window_capture_helper_;
+ bool enumerate_current_process_windows_;
+
// This map is used to avoid flickering for the case when SelectWindow() calls
// are interleaved with Capture() calls.
std::map<HWND, DesktopSize> window_size_map_;
