From 77d987a724c0d62494bd61b8b932d0c596ab3160 Mon Sep 17 00:00:00 2001 From: Jeroen Dhollander Date: Fri, 6 May 2022 14:47:13 +0200 Subject: [PATCH] Fix crash when resizing the display This crash happened when: * The cursor was located in the corner * The screen was resized so that the cursor position is outside of the frame. This caused us to add out-of-bound coordinates to the frame's updated_region, which caused crashes further down the pipeline. Bug: chromium:1323241 Test: new unittest Test: manually reproduced crash Change-Id: Ie71db58c8a347f00af8a3803fcd55cdcad6eafac Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261263 Reviewed-by: Alexander Cooper Commit-Queue: Jeroen Dhollander Cr-Commit-Position: refs/heads/main@{#36809} --- .../desktop_and_cursor_composer.cc | 6 +- .../desktop_and_cursor_composer_unittest.cc | 61 +++++++++++++++++-- 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/modules/desktop_capture/desktop_and_cursor_composer.cc b/modules/desktop_capture/desktop_and_cursor_composer.cc index bde91bd07b..7ff983c2d1 100644 --- a/modules/desktop_capture/desktop_and_cursor_composer.cc +++ b/modules/desktop_capture/desktop_and_cursor_composer.cc @@ -105,7 +105,11 @@ DesktopFrameWithCursor::DesktopFrameWithCursor( if (!previous_cursor_rect.equals(cursor_rect_)) { mutable_updated_region()->AddRect(cursor_rect_); - mutable_updated_region()->AddRect(previous_cursor_rect); + + // Only add the previous cursor if it is inside the frame. + // (it can be outside the frame if the desktop has been resized). + if (original_frame_->rect().ContainsRect(previous_cursor_rect)) + mutable_updated_region()->AddRect(previous_cursor_rect); } else if (cursor_changed) { mutable_updated_region()->AddRect(cursor_rect_); } diff --git a/modules/desktop_capture/desktop_and_cursor_composer_unittest.cc b/modules/desktop_capture/desktop_and_cursor_composer_unittest.cc index 5596576d9e..4d879a740e 100644 --- a/modules/desktop_capture/desktop_and_cursor_composer_unittest.cc +++ b/modules/desktop_capture/desktop_and_cursor_composer_unittest.cc @@ -15,18 +15,22 @@ #include #include +#include #include "modules/desktop_capture/desktop_capturer.h" #include "modules/desktop_capture/desktop_frame.h" #include "modules/desktop_capture/mouse_cursor.h" #include "modules/desktop_capture/shared_desktop_frame.h" #include "rtc_base/arraysize.h" +#include "test/gmock.h" #include "test/gtest.h" namespace webrtc { namespace { +using testing::ElementsAre; + const int kFrameXCoord = 100; const int kFrameYCoord = 200; const int kScreenWidth = 100; @@ -75,12 +79,12 @@ uint32_t BlendPixels(uint32_t dest, uint32_t src) { return b + (g << 8) + (r << 16) + 0xff000000; } -DesktopFrame* CreateTestFrame() { - DesktopFrame* frame = - new BasicDesktopFrame(DesktopSize(kScreenWidth, kScreenHeight)); +DesktopFrame* CreateTestFrame(int width = kScreenWidth, + int height = kScreenHeight) { + DesktopFrame* frame = new BasicDesktopFrame(DesktopSize(width, height)); uint32_t* data = reinterpret_cast(frame->data()); - for (int y = 0; y < kScreenHeight; ++y) { - for (int x = 0; x < kScreenWidth; ++x) { + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { *(data++) = GetFakeFramePixelValue(DesktopVector(x, y)); } } @@ -101,6 +105,15 @@ MouseCursor* CreateTestCursor(DesktopVector hotspot) { return new MouseCursor(image.release(), hotspot); } +std::vector GetUpdatedRegions(const DesktopFrame& frame) { + std::vector result; + for (webrtc::DesktopRegion::Iterator i(frame.updated_region()); !i.IsAtEnd(); + i.Advance()) { + result.push_back(i.rect()); + } + return result; +} + class FakeScreenCapturer : public DesktopCapturer { public: FakeScreenCapturer() {} @@ -184,10 +197,20 @@ void VerifyFrame(const DesktopFrame& frame, } // namespace +bool operator==(const DesktopRect& left, const DesktopRect& right) { + return left.equals(right); +} + +std::ostream& operator<<(std::ostream& out, const DesktopRect& rect) { + out << "{" << rect.left() << "+" << rect.top() << "-" << rect.width() << "x" + << rect.height() << "}"; + return out; +} + class DesktopAndCursorComposerTest : public ::testing::Test, public DesktopCapturer::Callback { public: - DesktopAndCursorComposerTest(bool include_cursor = true) + explicit DesktopAndCursorComposerTest(bool include_cursor = true) : fake_screen_(new FakeScreenCapturer()), fake_cursor_(include_cursor ? new FakeMouseMonitor() : nullptr), blender_(fake_screen_, fake_cursor_) { @@ -413,6 +436,32 @@ TEST_F(DesktopAndCursorComposerNoCursorMonitorTest, EXPECT_TRUE(frame_->updated_region().Equals(expected_region)); } +TEST_F(DesktopAndCursorComposerNoCursorMonitorTest, + UpdatedRegionDoesNotIncludeOldCursorIfOutOfBounds) { + blender_.OnMouseCursor(CreateTestCursor(DesktopVector(0, 0))); + + std::unique_ptr first_frame( + SharedDesktopFrame::Wrap(CreateTestFrame(1000, 1000))); + blender_.OnMouseCursorPosition(DesktopVector(900, 900)); + fake_screen_->SetNextFrame(first_frame->Share()); + + blender_.CaptureFrame(); + + // Second frame is smaller than first frame, and the first cursor is outside + // of the bounds of the new frame, so it should not be in the updated region. + std::unique_ptr second_frame( + SharedDesktopFrame::Wrap(CreateTestFrame(500, 500))); + auto second_cursor_rect = + DesktopRect::MakeXYWH(400, 400, kCursorWidth, kCursorHeight); + blender_.OnMouseCursorPosition(DesktopVector(400, 400)); + fake_screen_->SetNextFrame(second_frame->Share()); + blender_.CaptureFrame(); + + DesktopRegion expected_region; + expected_region.AddRect(second_cursor_rect); + EXPECT_THAT(GetUpdatedRegions(*frame_), ElementsAre(second_cursor_rect)); +} + TEST_F(DesktopAndCursorComposerNoCursorMonitorTest, UpdatedRegionIncludesOldAndNewCursorRectsIfShapeChanged) { std::unique_ptr frame(