From b9282256e4378fca7ffe51edc4ee2ca7f75cc1aa Mon Sep 17 00:00:00 2001
From: Joe Mason <joenotcharles@google.com>
Date: Wed, 03 Jun 2026 13:01:32 -0700
Subject: [PATCH] fix(win): prevent UAF in DirectManipulationHelper during COM calls

DirectManipulationHelper and DirectManipulationEventHandler make COM
calls which can spin nested message loops on Windows. If the helper or
its host is destroyed re-entrantly during these calls, it can lead to
Use-After-Free (UAF) crashes.

This CL:

1. Adds base::WeakPtr guards in DirectManipulationHelper around all COM
call sites to detect re-entrant destruction and return early.

2. Adds Microsoft::WRL::ComPtr keep_alive guards in
DirectManipulationEventHandler COM callbacks to keep the handler alive
during the call.

3. Adds !helper_ guards in DirectManipulationEventHandler to detect
helper destruction.

4. Adds systematic unit tests in direct_manipulation_win_unittest.cc
using mocks to simulate re-entrant destruction during each COM call,
verifying the robustness of the implementation.

The unit tests were substantially modified to ensure full coverage after
the tests and fix were initially generated by Gemini.

Also adds some missed WeakPtr checks at call sites, to supplement
existing checks in those classes.

TAG=agy
CONV=a93fa78e-b8b2-4b01-9fab-6d600aa556fb

Fixed: 516731749
Change-Id: I85ec20c7fa374220e2a678e0d87a0e9c436c6e73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7895539
Commit-Queue: Joe Mason <joenotcharles@google.com>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Auto-Submit: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/main@{#1641134}
---

diff --git a/content/browser/renderer_host/direct_manipulation_event_handler_win.cc b/content/browser/renderer_host/direct_manipulation_event_handler_win.cc
index 904119a..aac62ba 100644
--- a/content/browser/renderer_host/direct_manipulation_event_handler_win.cc
+++ b/content/browser/renderer_host/direct_manipulation_event_handler_win.cc
@@ -125,6 +125,8 @@
     IDirectManipulationViewport* viewport,
     DIRECTMANIPULATION_STATUS current,
     DIRECTMANIPULATION_STATUS previous) {
+  Microsoft::WRL::ComPtr<DirectManipulationEventHandler> keep_alive(this);
+
   // MSDN never mention |viewport| are nullable and we never saw it is null when
   // testing.
   DCHECK(viewport);
@@ -199,6 +201,8 @@
 HRESULT DirectManipulationEventHandler::OnContentUpdated(
     IDirectManipulationViewport* viewport,
     IDirectManipulationContent* content) {
+  Microsoft::WRL::ComPtr<DirectManipulationEventHandler> keep_alive(this);
+
   // MSDN never mention these params are nullable and we never saw they are null
   // when testing.
   DCHECK(viewport);
diff --git a/content/browser/renderer_host/direct_manipulation_helper_win.cc b/content/browser/renderer_host/direct_manipulation_helper_win.cc
index 51dac34..bf9a8e0 100644
--- a/content/browser/renderer_host/direct_manipulation_helper_win.cc
+++ b/content/browser/renderer_host/direct_manipulation_helper_win.cc
@@ -164,10 +164,18 @@
     RemoveAnimationObserver();
   }
 
+  auto weak_ptr = weak_factory_.GetWeakPtr();
+
   if (event_handler_) {
     event_handler_.Reset();
     viewport_->Stop();
+    if (!weak_ptr) {
+      return;
+    }
     viewport_->RemoveEventHandler(view_port_handler_cookie_);
+    if (!weak_ptr) {
+      return;
+    }
   }
 
   window_tree_host_ = window_tree_host;
@@ -186,6 +194,9 @@
   // IDirectManipulationViewportEventHandler.
   HRESULT hr = viewport_->AddEventHandler(window_, event_handler_.Get(),
                                           &view_port_handler_cookie_);
+  if (!weak_ptr) {
+    return;
+  }
   if (!SUCCEEDED(hr)) {
     event_handler_.Reset();
     return;
@@ -206,7 +217,12 @@
     event_handler_->SetViewportSizeInPixels(size_in_pixels);
   }
 
+  auto weak_ptr = weak_factory_.GetWeakPtr();
+
   HRESULT hr = viewport_->Stop();
+  if (!weak_ptr) {
+    return;
+  }
   if (!SUCCEEDED(hr))
     return;
 
@@ -215,6 +231,18 @@
 }
 
 void DirectManipulationHelper::OnPointerHitTest(WPARAM w_param) {
+  UINT32 pointer_id = GET_POINTERID_WPARAM(w_param);
+  POINTER_INPUT_TYPE pointer_type;
+  if (!::GetPointerType(pointer_id, &pointer_type)) {
+    // Use the generic "any pointer type" for unknown.
+    pointer_type = PT_POINTER;
+  }
+  OnPointerHitTest(pointer_id, pointer_type);
+}
+
+void DirectManipulationHelper::OnPointerHitTest(
+    UINT32 pointer_id,
+    POINTER_INPUT_TYPE pointer_type) {
   if (!event_handler_) {
     return;
   }
@@ -229,11 +257,12 @@
   // For WM_POINTER, the pointer type will show the event from mouse.
   // For WM_POINTERACTIVATE, the pointer id will be different with the following
   // message.
-  UINT32 pointer_id = GET_POINTERID_WPARAM(w_param);
-  POINTER_INPUT_TYPE pointer_type;
-  if (::GetPointerType(pointer_id, &pointer_type) &&
-      pointer_type == PT_TOUCHPAD) {
+  if (pointer_type == PT_TOUCHPAD) {
+    auto weak_ptr = weak_factory_.GetWeakPtr();
     viewport_->SetContact(pointer_id);
+    if (!weak_ptr) {
+      return;
+    }
   }
 }
 
@@ -257,8 +286,15 @@
 }
 
 void DirectManipulationHelper::Destroy() {
+  auto weak_ptr = weak_factory_.GetWeakPtr();
   UpdateEventHandler(nullptr, nullptr);
+  if (!weak_ptr) {
+    return;
+  }
   viewport_->Abandon();
+  if (!weak_ptr) {
+    return;
+  }
   manager_->Deactivate(window_);
 }
 
diff --git a/content/browser/renderer_host/direct_manipulation_helper_win.h b/content/browser/renderer_host/direct_manipulation_helper_win.h
index 33639ff..35488dfa 100644
--- a/content/browser/renderer_host/direct_manipulation_helper_win.h
+++ b/content/browser/renderer_host/direct_manipulation_helper_win.h
@@ -6,12 +6,14 @@
 #define CONTENT_BROWSER_RENDERER_HOST_DIRECT_MANIPULATION_HELPER_WIN_H_
 
 #include <windows.h>
+
 #include <directmanipulation.h>
 #include <wrl.h>
 
 #include <memory>
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/browser/renderer_host/direct_manipulation_event_handler_win.h"
@@ -39,6 +41,13 @@
 //    when DM_POINTERHITTEST.
 // 3. OnViewportStatusChanged will be called when the gesture phase change.
 //    OnContentUpdated will be called when the gesture update.
+//
+// IMPORTANT: Almost every function in this class can spin a nested message
+// loop, because they call into DirectManipulation COM objects. The nested
+// message loop can process WM_DESTROY messages that can delete the calling
+// class. So it's vital that the caller of every method take a WeakPtr to any
+// object that could be destroyed, and check if it's still valid after the
+// method returns.
 class CONTENT_EXPORT DirectManipulationHelper
     : public ui::CompositorAnimationObserver {
  public:
@@ -90,9 +99,13 @@
   // Unregister this as an AnimationObserver of ui::Compositor.
   void RemoveAnimationObserver();

diff --git a/content/browser/renderer_host/direct_manipulation_win_unittest.cc b/content/browser/renderer_host/direct_manipulation_win_unittest.cc
index 2ca109d4..6dd5059 100644
--- a/content/browser/renderer_host/direct_manipulation_win_unittest.cc
+++ b/content/browser/renderer_host/direct_manipulation_win_unittest.cc
@@ -6,6 +6,9 @@
 
 #include <utility>
 
+#include "base/functional/bind.h"
+#include "base/functional/callback.h"
+#include "base/time/time.h"
 #include "content/browser/renderer_host/direct_manipulation_helper_win.h"
 #include "content/browser/renderer_host/direct_manipulation_test_helper_win.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -41,6 +44,25 @@
 
   ~MockDirectManipulationViewport() override = default;
 
+  void set_stop_callback(base::OnceClosure callback) {
+    stop_callback_ = std::move(callback);
+  }
+  void set_add_event_handler_callback(base::OnceClosure callback) {
+    add_event_handler_callback_ = std::move(callback);
+  }
+  void set_remove_event_handler_callback(base::OnceClosure callback) {
+    remove_event_handler_callback_ = std::move(callback);
+  }
+  void set_set_contact_callback(base::OnceClosure callback) {
+    set_contact_callback_ = std::move(callback);
+  }
+  void set_abandon_callback(base::OnceClosure callback) {
+    abandon_callback_ = std::move(callback);
+  }
+  void set_zoom_to_rect_callback(base::OnceClosure callback) {
+    zoom_to_rect_callback_ = std::move(callback);
+  }
+
   bool WasZoomToRectCalled() {
     bool called = zoom_to_rect_called_;
     zoom_to_rect_called_ = false;
@@ -52,6 +74,9 @@
   HRESULT STDMETHODCALLTYPE Disable() override { return S_OK; }
 
   HRESULT STDMETHODCALLTYPE SetContact(_In_ UINT32 pointerId) override {
+    if (set_contact_callback_) {
+      std::move(set_contact_callback_).Run();
+    }
     return S_OK;
   }
 
@@ -92,6 +117,9 @@
                                        _In_ const float bottom,
                                        _In_ BOOL animate) override {
     zoom_to_rect_called_ = true;
+    if (zoom_to_rect_callback_) {
+      std::move(zoom_to_rect_callback_).Run();
+    }
     return S_OK;
   }
 
@@ -156,10 +184,16 @@
   AddEventHandler(_In_opt_ HWND window,
                   _In_ IDirectManipulationViewportEventHandler* eventHandler,
                   _Out_ DWORD* cookie) override {
+    if (add_event_handler_callback_) {
+      std::move(add_event_handler_callback_).Run();
+    }
     return S_OK;
   }
 
   HRESULT STDMETHODCALLTYPE RemoveEventHandler(_In_ DWORD cookie) override {
+    if (remove_event_handler_callback_) {
+      std::move(remove_event_handler_callback_).Run();
+    }
     return S_OK;
   }
 
@@ -173,12 +207,28 @@
     return S_OK;
   }
 
-  HRESULT STDMETHODCALLTYPE Stop() override { return S_OK; }
+  HRESULT STDMETHODCALLTYPE Stop() override {
+    if (stop_callback_) {
+      std::move(stop_callback_).Run();
+    }
+    return S_OK;
+  }
 
-  HRESULT STDMETHODCALLTYPE Abandon() override { return S_OK; }
+  HRESULT STDMETHODCALLTYPE Abandon() override {
+    if (abandon_callback_) {
+      std::move(abandon_callback_).Run();
+    }
+    return S_OK;
+  }
 
  private:
   bool zoom_to_rect_called_ = false;
+  base::OnceClosure stop_callback_;
+  base::OnceClosure add_event_handler_callback_;
+  base::OnceClosure remove_event_handler_callback_;
+  base::OnceClosure set_contact_callback_;
+  base::OnceClosure abandon_callback_;
+  base::OnceClosure zoom_to_rect_callback_;
 };
 
 class MockDirectManipulationUpdateManager
@@ -193,6 +243,10 @@
 
   ~MockDirectManipulationUpdateManager() override = default;
 
+  void set_update_callback(base::OnceClosure callback) {
+    update_callback_ = std::move(callback);
+  }
+
   HRESULT STDMETHODCALLTYPE
   RegisterWaitHandleCallback(HANDLE,
                              IDirectManipulationUpdateHandler*,
@@ -207,8 +261,14 @@
 
   HRESULT STDMETHODCALLTYPE
   Update(IDirectManipulationFrameInfoProvider*) override {
+    if (update_callback_) {
+      std::move(update_callback_).Run();
+    }
     return S_OK;
   }
+
+ private:
+  base::OnceClosure update_callback_;
 };
 
 class MockDirectManipulationManager
@@ -257,6 +317,10 @@
     return S_OK;
   }
 
+  ComPtr<MockDirectManipulationUpdateManager> mock_update_manager() {
+    return update_manager_;
+  }
+
  private:
   ComPtr<MockDirectManipulationViewport> viewport_;
   ComPtr<MockDirectManipulationUpdateManager> update_manager_ =
@@ -408,17 +472,36 @@
 
   void SetUp() override {
     testing::Test::SetUp();
+    viewport_ = Microsoft::WRL::Make<MockDirectManipulationViewport>();
+    ASSERT_TRUE(viewport_);
+    manager_ = Microsoft::WRL::Make<MockDirectManipulationManager>(viewport_);
+    ASSERT_TRUE(manager_);
     direct_manipulation_helper_ =
-        DirectManipulationHelper::CreateInstanceForTesting(
-            Microsoft::WRL::Make<MockDirectManipulationManager>(viewport_));
+        DirectManipulationHelper::CreateInstanceForTesting(manager_);
     ASSERT_TRUE(direct_manipulation_helper_);
     direct_manipulation_helper_->UpdateEventHandler(nullptr, &event_target_);
+    content_ = Microsoft::WRL::Make<MockDirectManipulationContent>();
+    ASSERT_TRUE(content_);
   }
 
   DirectManipulationHelper* GetDirectManipulationHelper() {
     return direct_manipulation_helper_.get();
   }
 
+  base::OnceClosure ResetDirectManipulationHelperCallback() {
+    return base::BindOnce(
+        &DirectManipulationUnitTest::ResetDirectManipulationHelper,
+        base::Unretained(this));
+  }
+
+  void RecreateDirectManipulationHelper() {
+    ASSERT_FALSE(direct_manipulation_helper_);
+    ASSERT_TRUE(manager_);
+    direct_manipulation_helper_ =
+        DirectManipulationHelper::CreateInstanceForTesting(manager_);
+    ASSERT_TRUE(direct_manipulation_helper_);
+  }
+
   std::vector<Event> GetEvents() { return event_target_.GetEvents(); }
 
   void ViewportStatusChanged(DIRECTMANIPULATION_STATUS current,
@@ -439,13 +522,16 @@
     direct_manipulation_helper_->SetDeviceScaleFactorForTesting(factor);
   }
 
- private:
-  std::unique_ptr<DirectManipulationHelper> direct_manipulation_helper_;
-  ComPtr<MockDirectManipulationViewport> viewport_ =
-      Microsoft::WRL::Make<MockDirectManipulationViewport>();
-  ComPtr<MockDirectManipulationContent> content_ =
-      Microsoft::WRL::Make<MockDirectManipulationContent>();
+ protected:
+  ComPtr<MockDirectManipulationViewport> viewport_;
+  ComPtr<MockDirectManipulationManager> manager_;
+  ComPtr<MockDirectManipulationContent> content_;
   MockWindowEventTarget event_target_;
+
+ private:
+  void ResetDirectManipulationHelper() { direct_manipulation_helper_.reset(); }
+
+  std::unique_ptr<DirectManipulationHelper> direct_manipulation_helper_;
 };
 
 TEST_F(DirectManipulationUnitTest, ReceiveSimplePanTransform) {
@@ -771,4 +857,77 @@
   EXPECT_EQ(5, events[0].scroll_x_);
 }
 
+// DirectManipulation COM calls on the UI thread can enter a nested message loop
+// while they block on an internal delegate thread. The nested message loop can
+// process WM_DESTROY messages that can invalidate pointers on the stack. To
+// simulate this, these tests delete the DirectManipulationHelper from a mock
+// COM call in each method that makes COM calls. If the methods don't guard
+// their stack objects this will cause ASAN errors. Not all of these functions
+// will enter a nested message loop in practice, but better safe than sorry.
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringOnAnimationStep) {
+  manager_->mock_update_manager()->set_update_callback(
+      ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->OnAnimationStep(base::TimeTicks::Now());
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringUpdateEventHandler) {
+  ASSERT_TRUE(GetDirectManipulationHelper()->HasEventHandlerForTesting());
+  viewport_->set_remove_event_handler_callback(
+      ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->UpdateEventHandler(nullptr, nullptr);
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+
+  RecreateDirectManipulationHelper();
+  ASSERT_FALSE(GetDirectManipulationHelper()->HasEventHandlerForTesting());
+  viewport_->set_add_event_handler_callback(
+      ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->UpdateEventHandler(nullptr, &event_target_);
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringSetSizeInPixels) {
+  viewport_->set_stop_callback(ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->SetSizeInPixels(gfx::Size(2000, 2000));
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringOnPointerHitTest) {
+  ASSERT_TRUE(GetDirectManipulationHelper()->HasEventHandlerForTesting());
+  viewport_->set_set_contact_callback(ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->OnPointerHitTest(0, PT_TOUCHPAD);
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringDestroy) {
+  // OnCompositingShuttingDown can call Destroy(), which makes COM calls that
+  // could enter a nested event loop. Those could process WM_DESTROY messages
+  // that delete the DirectManipulationHelper, causing the destructor to
+  // re-enter Destroy().
+  viewport_->set_abandon_callback(ResetDirectManipulationHelperCallback());
+  GetDirectManipulationHelper()->OnCompositingShuttingDown(
+      GetDirectManipulationHelper()->compositor());
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringZoomToRect) {
+  // ZoomToRect is triggered from
+  // DirectManipulationEventHandler::OnViewportStatusChanged when there's a
+  // content transform.
+  ContentUpdated(1.1f, 0, 0);
+  viewport_->set_zoom_to_rect_callback(ResetDirectManipulationHelperCallback());
+  ViewportStatusChanged(DIRECTMANIPULATION_READY, DIRECTMANIPULATION_RUNNING);
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
+TEST_F(DirectManipulationUnitTest, DestroyDuringGetContentTransform) {
+  // GetContentTransform is triggered from
+  // DirectManipulationEventHandler::OnContentUpdated.
+  content_->set_get_content_transform_callback(
+      ResetDirectManipulationHelperCallback());
+  ContentUpdated(1.1f, 0, 0);
+  EXPECT_EQ(GetDirectManipulationHelper(), nullptr);
+}
+
 }  //  namespace content