Web Share: Handle destruction in callback for synchronous execution

The existing ShowShareUIForWindowOperation did not handle the case where
the Window's ShowShareUIForWindow call executed its DataRequested event
synchronously AND the the provided DataRequestedCallback destroyed the
ShowShareUIForWindowOperation.

Updating the ShowShareUIForWindowOperation to handle this case, and
updating the ShowShareUIForWindowOperationTests to expose this case by
disposing of Operation instances as part of the DataRequested callbacks
(mirroring the behavior of the production code that uses this class).

Bug: 1162330
Change-Id: I1c85fb3e16420a9742a75b2a049588c692886e41
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606786
Commit-Queue: Hoch Hochkeppel <mhochk@microsoft.com>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839685}

chrome/browser/webshare/win/show_share_ui_for_window_operation.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/webshare/win/show_share_ui_for_window_operation.h"
 
+#include <EventToken.h>
 #include <shlobj.h>
 #include <windows.applicationmodel.datatransfer.h>
 #include <wrl/event.h>
@@ -72,7 +73,6 @@ HRESULT GetDataTransferManagerHandles(
 
 ShowShareUIForWindowOperation::ShowShareUIForWindowOperation(HWND hwnd)
     : hwnd_(hwnd) {
-  data_requested_token_.value = 0;
 }
 
 ShowShareUIForWindowOperation::~ShowShareUIForWindowOperation() {
@@ -93,12 +93,13 @@ void ShowShareUIForWindowOperation::Run(
 
   // Fetch the OS handles needed
   ComPtr<IDataTransferManagerInterop> data_transfer_manager_interop;
+  ComPtr<IDataTransferManager> data_transfer_manager;
   HRESULT hr = GetDataTransferManagerHandles(
-      hwnd_, &data_transfer_manager_interop, &data_transfer_manager_);
+      hwnd_, &data_transfer_manager_interop, &data_transfer_manager);
   if (FAILED(hr))
     return Cancel();
 
-  // Create and register a data request handler
+  // Create and register a data requested handler
   auto weak_ptr = weak_factory_.GetWeakPtr();
   auto raw_data_requested_callback = Callback<
       ITypedEventHandler<DataTransferManager*, DataRequestedEventArgs*>>(
@@ -113,59 +114,73 @@ void ShowShareUIForWindowOperation::Run(
         // operation will fail gracefully with messaging to the user.
         return S_OK;
       });
-  hr = data_transfer_manager_->add_DataRequested(
-      raw_data_requested_callback.Get(), &data_requested_token_);
-  if (FAILED(hr))
+  EventRegistrationToken data_requested_token{};
+  hr = data_transfer_manager->add_DataRequested(
+      raw_data_requested_callback.Get(), &data_requested_token);
+
+  // Create a callback to clean up the data requested handler that doesn't rely
+  // on |this| so it can still be run even if |this| has been destroyed
+  auto remove_data_requested_listener = base::BindOnce(
+      [](ComPtr<IDataTransferManager> data_transfer_manager,
+         EventRegistrationToken data_requested_token) {
+        if (data_transfer_manager && data_requested_token.value) {
+          data_transfer_manager->remove_DataRequested(data_requested_token);
+        }
+      },
+      data_transfer_manager, data_requested_token);
+
+  // If the call to register the data requested handler failed, clean up
+  // listener and cancel the operation
+  if (FAILED(hr)) {
+    std::move(remove_data_requested_listener).Run();
     return Cancel();
+  }
 
   // Request showing the Share UI
-  show_share_ui_for_window_call_in_progress_ = true;
   hr = data_transfer_manager_interop->ShowShareUIForWindow(hwnd_);
-  show_share_ui_for_window_call_in_progress_ = false;
 
-  // If the call is expected to complete later, schedule a timeout to cover
-  // any cases where it fails (and therefore never comes)
-  if (SUCCEEDED(hr) && data_requested_callback_) {
+  // If the call is expected to complete later, save the clean-up callback for
+  // later use and schedule a timeout to cover any cases where it fails (and
+  // therefore never comes)
+  if (SUCCEEDED(hr) && weak_ptr && data_requested_callback_) {
+    remove_data_requested_listener_ = std::move(remove_data_requested_listener);
     if (!base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
             FROM_HERE,
-            base::BindOnce(&ShowShareUIForWindowOperation::Cancel,
-                           weak_factory_.GetWeakPtr()),
+            base::BindOnce(&ShowShareUIForWindowOperation::Cancel, weak_ptr),
             kMaxExecutionTime)) {
       return Cancel();
     }
   } else {
-    RemoveDataRequestedListener();
+    // In all other cases (i.e. failure or synchronous completion), remove the
+    // listener right away
+    std::move(remove_data_requested_listener).Run();
   }
 }
 
 void ShowShareUIForWindowOperation::Cancel() {
-  RemoveDataRequestedListener();
-  if (data_requested_callback_) {
+  if (remove_data_requested_listener_)
+    std::move(remove_data_requested_listener_).Run();
+
+  if (data_requested_callback_)
     std::move(data_requested_callback_).Run(nullptr);
-  }
 }
 
 void ShowShareUIForWindowOperation::OnDataRequested(
     IDataTransferManager* data_transfer_manager,
     IDataRequestedEventArgs* event_args) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK_EQ(data_transfer_manager, data_transfer_manager_.Get());
 
-  // Remove the DataRequested handler if this is being invoked asynchronously.
-  // If this is an in-progress invocation the system APIs don't handle the
-  // event being unregistered while it is being executed, but we will unregister
-  // it after the ShowShareUIForWindow call completes.
-  if (!show_share_ui_for_window_call_in_progress_)
-    RemoveDataRequestedListener();
+  // If the callback to remove the DataRequested listener has been stored on
+  // |this| (i.e. this function is being invoked asynchronously), invoke it now
+  // before invoking the |data_requested_callback_|, as that may result in
+  // |this| being destroyed. Note that the callback to remove the DataRequested
+  // listener will not have been set if this is being invoked synchronously as
+  // part of the ShowShareUIForWindow call, as the system APIs don't handle
+  // unregistering at that point.
+  if (remove_data_requested_listener_)
+    std::move(remove_data_requested_listener_).Run();
 
   std::move(data_requested_callback_).Run(event_args);
 }
 
-void ShowShareUIForWindowOperation::RemoveDataRequestedListener() {
-  if (data_transfer_manager_ && data_requested_token_.value) {
-    data_transfer_manager_->remove_DataRequested(data_requested_token_);
-    data_requested_token_.value = 0;
-  }
-}
-
 }  // namespace webshare

chrome/browser/webshare/win/show_share_ui_for_window_operation.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_WEBSHARE_WIN_SHOW_SHARE_UI_FOR_WINDOW_OPERATION_H_
 #define CHROME_BROWSER_WEBSHARE_WIN_SHOW_SHARE_UI_FOR_WINDOW_OPERATION_H_
 
-#include <EventToken.h>
 #include <wrl/client.h>
 
 #include "base/callback.h"
@@ -71,15 +70,10 @@ class ShowShareUIForWindowOperation {
           data_transfer_manager,
       ABI::Windows::ApplicationModel::DataTransfer::IDataRequestedEventArgs*
           event_args);
-  void RemoveDataRequestedListener();
 
   DataRequestedCallback data_requested_callback_;
-  EventRegistrationToken data_requested_token_;
-  Microsoft::WRL::ComPtr<
-      ABI::Windows::ApplicationModel::DataTransfer::IDataTransferManager>
-      data_transfer_manager_;
   const HWND hwnd_;
-  bool show_share_ui_for_window_call_in_progress_;
+  base::OnceClosure remove_data_requested_listener_;
   base::WeakPtrFactory<ShowShareUIForWindowOperation> weak_factory_{this};
 };
 

chrome/browser/webshare/win/show_share_ui_for_window_operation_unittest.cc

@@ -48,6 +48,7 @@ class ShowShareUIForWindowOperationTest : public ::testing::Test {
     if (!IsSupportedEnvironment())
       return;
     ASSERT_NO_FATAL_FAILURE(scoped_interop_.SetUp());
+    operation_ = std::make_unique<ShowShareUIForWindowOperation>(hwnd_);
     auto weak_ptr = weak_factory_.GetWeakPtr();
     test_callback_ = base::BindOnce(
         [](base::WeakPtr<ShowShareUIForWindowOperationTest> weak_ptr,
@@ -58,6 +59,9 @@ class ShowShareUIForWindowOperationTest : public ::testing::Test {
             weak_ptr->test_callback_state_ =
                 event_args ? TestCallbackState::RunWithValue
                            : TestCallbackState::RunWithoutValue;
+            // Explicitly free the operation to validate it handles being
+            // destroyed as soon as it has invoked the callback
+            weak_ptr->operation_.reset();
           }
         },
         weak_ptr);
@@ -75,6 +79,7 @@ class ShowShareUIForWindowOperationTest : public ::testing::Test {
   }
 
   const HWND hwnd_ = reinterpret_cast<HWND>(1);
+  std::unique_ptr<ShowShareUIForWindowOperation> operation_;
   ScopedFakeDataTransferManagerInterop scoped_interop_;
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
@@ -90,8 +95,7 @@ TEST_F(ShowShareUIForWindowOperationTest, AsyncSuccess) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::SucceedWithoutAction);
 
-  ShowShareUIForWindowOperation operation{hwnd_};
-  operation.Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::NotRun);
   auto data_requested_invoker = fake_interop().GetDataRequestedInvoker(hwnd_);
 
@@ -106,8 +110,7 @@ TEST_F(ShowShareUIForWindowOperationTest, AsyncFailure) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::SucceedWithoutAction);
 
-  ShowShareUIForWindowOperation operation{hwnd_};
-  operation.Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::NotRun);
 
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
@@ -125,12 +128,11 @@ TEST_F(ShowShareUIForWindowOperationTest, AsyncEarlyDestruction) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::SucceedWithoutAction);
 
-  auto operation = std::make_unique<ShowShareUIForWindowOperation>(hwnd_);
-  operation->Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::NotRun);
 
   auto data_requested_invoker = fake_interop().GetDataRequestedInvoker(hwnd_);
-  ASSERT_NO_FATAL_FAILURE(operation.reset());
+  ASSERT_NO_FATAL_FAILURE(operation_.reset());
   ASSERT_EQ(test_callback_state_, TestCallbackState::RunWithoutValue);
   ASSERT_NO_FATAL_FAILURE(std::move(data_requested_invoker).Run());
 }
@@ -142,8 +144,7 @@ TEST_F(ShowShareUIForWindowOperationTest, SyncSuccess) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::InvokeEventSynchronously);
 
-  ShowShareUIForWindowOperation operation{hwnd_};
-  operation.Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::RunWithValue);
 }
 
@@ -154,8 +155,7 @@ TEST_F(ShowShareUIForWindowOperationTest, SyncEarlyFailure) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::FailImmediately);
 
-  ShowShareUIForWindowOperation operation{hwnd_};
-  operation.Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::NotRun);
 }
 
@@ -166,8 +166,7 @@ TEST_F(ShowShareUIForWindowOperationTest, SyncLateFailure) {
   fake_interop().SetShowShareUIForWindowBehavior(
       ShowShareUIForWindowBehavior::InvokeEventSynchronouslyAndReturnFailure);
 
-  ShowShareUIForWindowOperation operation{hwnd_};
-  operation.Run(std::move(test_callback_));
+  operation_->Run(std::move(test_callback_));
   ASSERT_EQ(test_callback_state_, TestCallbackState::RunWithValue);
 }
 
@@ -175,8 +174,7 @@ TEST_F(ShowShareUIForWindowOperationTest, DestructionWithoutRun) {
   if (!IsSupportedEnvironment())
     return;
 
-  auto operation = std::make_unique<ShowShareUIForWindowOperation>(hwnd_);
-  ASSERT_NO_FATAL_FAILURE(operation.reset());
+  ASSERT_NO_FATAL_FAILURE(operation_.reset());
 }
 
 }  // namespace webshare