From ba81de266ae46fac4e8f6a758394191e8a005e04 Mon Sep 17 00:00:00 2001 From: Ray Kraesig Date: Tue, 17 Sep 2024 19:16:46 +0000 Subject: [PATCH] Bug 1915665 - [2/2] Simplify the Chromium-inherited API r=win-reviewers,handyman The original Chromium listener code has a few infelicities in its API. * The current session ID is always available, so the `Nothing()` branch is impossible. Remove the `Maybe` from the interface. * If session-change messages for other sessions are received, discard them early rather than propagating them. * Use the whole DWORD value supplied by `GUID_SESSION_DISPLAY_STATUS`, rather than just the first byte. (This has no effect here, since `MONITOR_DISPLAY_STATE` is an enum with very few values, but might if someone blindly copies this code with minimal adjustments to be used with another `PBT_POWERSETTINGCHANGE` value.) Differential Revision: https://phabricator.services.mozilla.com/D220749 --- widget/windows/WinEventObserver.cpp | 65 ++++++++++++++------ widget/windows/WinWindowOcclusionTracker.cpp | 7 +-- widget/windows/WinWindowOcclusionTracker.h | 2 +- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/widget/windows/WinEventObserver.cpp b/widget/windows/WinEventObserver.cpp index c3cbc35551e0..90300e941290 100644 --- a/widget/windows/WinEventObserver.cpp +++ b/widget/windows/WinEventObserver.cpp @@ -4,7 +4,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include #include +#include #include #include @@ -14,7 +16,6 @@ #include "mozilla/Assertions.h" #include "mozilla/ClearOnShutdown.h" #include "mozilla/Logging.h" -#include "mozilla/Maybe.h" #include "nsWindowDbg.h" #include "nsdefs.h" #include "nsXULAppAPI.h" @@ -94,20 +95,38 @@ namespace evtwin_details { static void OnSessionChange(WPARAM wParam, LPARAM lParam) { if (wParam == WTS_SESSION_LOCK || wParam == WTS_SESSION_UNLOCK) { - Maybe isCurrentSession; - DWORD currentSessionId = 0; - if (!::ProcessIdToSessionId(::GetCurrentProcessId(), ¤tSessionId)) { - isCurrentSession = Nothing(); - } else { - OBS_LOG( - "WinEventWindow OnSessionChange() wParam %zu lParam " - "%" PRIdLPTR " currentSessionId %lu", - wParam, lParam, currentSessionId); - - isCurrentSession = Some(static_cast(lParam) == currentSessionId); + DWORD currentSessionId; + BOOL const rv = + ::ProcessIdToSessionId(::GetCurrentProcessId(), ¤tSessionId); + if (!rv) { + // A process should always have the relevant access privileges for itself, + // but the above call could still fail if, e.g., someone's playing games + // with function imports. If so, just assert and/or skip out. + // + // Should this turn out to somehow be a real concern, we could do + // ``` + // DWORD const currentSessionId = + // ::NtCurrentTeb()->ProcessEnvironmentBlock->SessionId; + // ``` + // instead, which is actually documented (albeit abjured against). + MOZ_ASSERT(false, "::ProcessIdToSessionId() failed"); + return; } + + OBS_LOG("WinEventWindow OnSessionChange(): wParam=%zu lParam=%" PRIdLPTR + " currentSessionId=%lu", + wParam, lParam, currentSessionId); + + // Ignore lock/unlock messages for other sessions -- which Windows actually + // _does_ send in some scenarios; see review of Chromium changeset 1929489: + // + // https://chromium-review.googlesource.com/c/chromium/src/+/1929489 + if (currentSessionId != (DWORD)lParam) { + return; + } + if (auto* wwot = WinWindowOcclusionTracker::Get()) { - wwot->OnSessionChange(wParam, isCurrentSession); + wwot->OnSessionChange(wParam); } } } @@ -115,14 +134,22 @@ static void OnSessionChange(WPARAM wParam, LPARAM lParam) { static void OnPowerBroadcast(WPARAM wParam, LPARAM lParam) { if (wParam == PBT_POWERSETTINGCHANGE) { POWERBROADCAST_SETTING* setting = (POWERBROADCAST_SETTING*)lParam; + MOZ_ASSERT(setting); - if (setting && - ::IsEqualGUID(setting->PowerSetting, GUID_SESSION_DISPLAY_STATUS) && + if (::IsEqualGUID(setting->PowerSetting, GUID_SESSION_DISPLAY_STATUS) && setting->DataLength == sizeof(DWORD)) { - bool displayOn = PowerMonitorOff != - static_cast(setting->Data[0]); + MONITOR_DISPLAY_STATE state{}; + errno_t const err = + ::memcpy_s(&state, sizeof(state), setting->Data, setting->DataLength); + if (err) { + MOZ_ASSERT(false, "bad data in POWERBROADCAST_SETTING in lParam"); + return; + } - OBS_LOG("WinEventWindow OnPowerBroadcast() displayOn %d", displayOn); + bool const displayOn = MONITOR_DISPLAY_STATE::PowerMonitorOff != state; + + OBS_LOG("WinEventWindow OnPowerBroadcast(): displayOn=%d", + int(displayOn ? 1 : 0)); if (auto* wwot = WinWindowOcclusionTracker::Get()) { wwot->OnDisplayStateChanged(displayOn); @@ -154,7 +181,7 @@ LRESULT CALLBACK WinEventWindow::WndProc(HWND hwnd, UINT msg, WPARAM wParam, } break; } - LRESULT ret = ::DefWindowProcW(hwnd, msg, wParam, lParam); + LRESULT const ret = ::DefWindowProcW(hwnd, msg, wParam, lParam); eventLogger.SetResult(ret, false); return ret; } diff --git a/widget/windows/WinWindowOcclusionTracker.cpp b/widget/windows/WinWindowOcclusionTracker.cpp index d2da809e5bc7..a5a238e8340d 100644 --- a/widget/windows/WinWindowOcclusionTracker.cpp +++ b/widget/windows/WinWindowOcclusionTracker.cpp @@ -703,18 +703,13 @@ void WinWindowOcclusionTracker::UpdateOcclusionState( } } -void WinWindowOcclusionTracker::OnSessionChange(WPARAM aStatusCode, - Maybe aIsCurrentSession) { +void WinWindowOcclusionTracker::OnSessionChange(WPARAM aStatusCode) { MOZ_ASSERT(NS_IsMainThread()); if (!StaticPrefs:: widget_windows_window_occlusion_tracking_session_lock_enabled()) { return; } - if (aIsCurrentSession.isNothing() || !*aIsCurrentSession) { - return; - } - if (aStatusCode == WTS_SESSION_UNLOCK) { LOG(LogLevel::Info, "WinWindowOcclusionTracker::OnSessionChange() WTS_SESSION_UNLOCK"); diff --git a/widget/windows/WinWindowOcclusionTracker.h b/widget/windows/WinWindowOcclusionTracker.h index 04bbd7d8eb3e..3b44a132c493 100644 --- a/widget/windows/WinWindowOcclusionTracker.h +++ b/widget/windows/WinWindowOcclusionTracker.h @@ -274,7 +274,7 @@ class WinWindowOcclusionTracker final { public: // This is called with session changed notifications. If the screen is locked // by the current session, it marks app windows as occluded. - void OnSessionChange(WPARAM aStatusCode, Maybe aIsCurrentSession); + void OnSessionChange(WPARAM aStatusCode); // This is called when the display is put to sleep. If the display is sleeping // it marks app windows as occluded.