Initialize PostAsyncResults returned value

Before this change, PostAsyncResults returned a value that was not
explicitly initialized if the IAsyncOperation failed. This was not
causing a problem in the code today because ComPtrs are implicitly
initialized to a valid (though empty) state and all of the consumers of
this code currently are for IAsyncOperations that return types that
inherit from IUnknown (which causes the PostAsyncResults method to
return them inside a ComPtr). If, however, PostAsyncResults is called
for an IAsyncOperation that returns a different kind of value (e.g. an
int), the returned value was unitialized memory (in the failure case).

To protect against future use of PostAsyncResults with non-IUnknown
types (like that planned as part of http://crbug.com/1035527) this
change adds default value initialization of the result, as well as unit
tests that validate this behavior and a test-helper class for
simulating IAsyncOperations.

Bug: 1121285
Change-Id: If87d817c3cff8830a9e91eeb8fa51d2c32d17f01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2373165Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Hoch Hochkeppel <mhochk@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#802241}

base/BUILD.gn

@@ -3005,6 +3005,7 @@ test("base_unittests") {
       "win/object_watcher_unittest.cc",
       "win/pe_image_reader_unittest.cc",
       "win/pe_image_unittest.cc",
+      "win/post_async_results_unittest.cc",
       "win/reference_unittest.cc",
       "win/registry_unittest.cc",
       "win/scoped_bstr_unittest.cc",

base/test/BUILD.gn

@@ -172,6 +172,7 @@ static_library("test_support") {
 
   if (is_win) {
     sources += [
+      "fake_iasync_operation_win.h",
       "scoped_os_info_override_win.cc",
       "scoped_os_info_override_win.h",
       "test_file_util_win.cc",

base/test/fake_iasync_operation_win.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TEST_FAKE_IASYNC_OPERATION_WIN_H_
+#define BASE_TEST_FAKE_IASYNC_OPERATION_WIN_H_
+
+#include <wrl/client.h>
+
+#include "base/notreached.h"
+#include "base/win/winrt_foundation_helpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace win {
+
+namespace internal {
+
+// Templates used to allow easy reference to the correct types.
+// See base/win/winrt_foundation_helpers.h for explanation.
+template <typename T>
+using AsyncOperationComplex =
+    typename ABI::Windows::Foundation::IAsyncOperation<T>::TResult_complex;
+
+template <typename T>
+using AsyncOperationAbi = AbiType<AsyncOperationComplex<T>>;
+
+template <typename T>
+using AsyncOperationOptionalStorage =
+    OptionalStorageType<AsyncOperationComplex<T>>;
+
+template <typename T>
+using AsyncOperationStorage = StorageType<AsyncOperationComplex<T>>;
+
+}  // namespace internal
+
+// Provides an implementation of Windows::Foundation::IAsyncOperation for
+// use in GTests.
+template <typename T>
+class FakeIAsyncOperation final
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<
+              Microsoft::WRL::WinRt | Microsoft::WRL::InhibitRoOriginateError>,
+          ABI::Windows::Foundation::IAsyncOperation<T>,
+          ABI::Windows::Foundation::IAsyncInfo> {
+ public:
+  FakeIAsyncOperation() = default;
+  FakeIAsyncOperation(const FakeIAsyncOperation&) = delete;
+  FakeIAsyncOperation& operator=(const FakeIAsyncOperation&) = delete;
+
+  // ABI::Windows::Foundation::IAsyncOperation:
+  IFACEMETHODIMP put_Completed(
+      ABI::Windows::Foundation::IAsyncOperationCompletedHandler<T>* handler)
+      final {
+    EXPECT_EQ(nullptr, handler_)
+        << "put_Completed called on IAsyncOperation with a CompletedHandler "
+           "already defined.";
+    handler_ = handler;
+    return S_OK;
+  }
+  IFACEMETHODIMP get_Completed(
+      ABI::Windows::Foundation::IAsyncOperationCompletedHandler<T>** handler)
+      final {
+    NOTREACHED();
+    return E_NOTIMPL;
+  }
+  IFACEMETHODIMP GetResults(internal::AsyncOperationAbi<T>* results) final {
+    if (!is_complete_) {
+      ADD_FAILURE() << "GetResults called on incomplete IAsyncOperation.";
+      return E_PENDING;
+    }
+    if (status_ != AsyncStatus::Completed)
+      return E_UNEXPECTED;
+    return base::win::internal::CopyTo(results_, results);
+  }
+
+  // ABI::Windows::Foundation::IAsyncInfo:
+  IFACEMETHODIMP get_Id(uint32_t* id) final {
+    NOTREACHED();
+    return E_NOTIMPL;
+  }
+  IFACEMETHODIMP get_Status(AsyncStatus* status) final {
+    *status = status_;
+    return S_OK;
+  }
+  IFACEMETHODIMP get_ErrorCode(HRESULT* error_code) final {
+    *error_code = error_code_;
+    return S_OK;
+  }
+  IFACEMETHODIMP Cancel() final {
+    NOTREACHED();
+    return E_NOTIMPL;
+  }
+  IFACEMETHODIMP Close() final {
+    NOTREACHED();
+    return E_NOTIMPL;
+  }
+
+  // Completes the operation with |error_code|.
+  //
+  // The get_ErrorCode API will be set to return |error_code|, the remainder of
+  // the APIs will be set to represent an error state, and the CompletedHandler
+  // (if defined) will be run.
+  void CompleteWithError(HRESULT error_code) {
+    error_code_ = error_code;
+    status_ = AsyncStatus::Error;
+    InvokeCompletedHandler();
+  }
+
+  // Completes the operation with |results|.
+  //
+  // The GetResults API will be set to return |results|, the remainder of the
+  // APIs will be set to represent a successfully completed state, and the
+  // CompletedHandler (if defined) will be run.
+  void CompleteWithResults(internal::AsyncOperationStorage<T> results) {
+    error_code_ = S_OK;
+    results_ = std::move(results);
+    status_ = AsyncStatus::Completed;
+    InvokeCompletedHandler();
+  }
+
+ private:
+  void InvokeCompletedHandler() {
+    ASSERT_FALSE(is_complete_)
+        << "Attempted to invoke completion on an already "
+           "completed IAsyncOperation.";
+    is_complete_ = true;
+    if (handler_)
+      handler_->Invoke(this, status_);
+  }
+
+  HRESULT error_code_ = S_OK;
+  Microsoft::WRL::ComPtr<
+      ABI::Windows::Foundation::IAsyncOperationCompletedHandler<T>>
+      handler_;
+  bool is_complete_ = false;
+  internal::AsyncOperationOptionalStorage<T> results_;
+  AsyncStatus status_ = AsyncStatus::Started;
+};
+
+}  // namespace win
+}  // namespace base
+
+#endif  // BASE_TEST_FAKE_IASYNC_OPERATION_WIN_H_

base/win/post_async_results.h

@@ -65,6 +65,7 @@ AsyncResultsT<T> GetAsyncResults(
   if (FAILED(hr)) {
     VLOG(2) << "GetAsyncResults failed: "
             << logging::SystemErrorCodeToString(hr);
+    return AsyncResultsT<T>{};
   }
 
   return results;

base/win/post_async_results_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/win/post_async_results.h"
+
+#include "base/test/bind_test_util.h"
+#include "base/test/fake_iasync_operation_win.h"
+#include "base/test/task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ABI::Windows::Foundation::IAsyncOperation;
+using ABI::Windows::Foundation::IAsyncOperation_impl;
+using ABI::Windows::Foundation::IAsyncOperationCompletedHandler;
+using ABI::Windows::Foundation::IAsyncOperationCompletedHandler_impl;
+using Microsoft::WRL::ComPtr;
+
+namespace {
+class TestClassImplementingIUnknown
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<
+              Microsoft::WRL::WinRtClassicComMix |
+              Microsoft::WRL::InhibitRoOriginateError>,
+          IUnknown> {};
+}  // namespace
+
+namespace ABI {
+namespace Windows {
+namespace Foundation {
+
+template <>
+struct __declspec(uuid("3895C200-8F26-4F5A-B29D-2B5D72E68F99"))
+    IAsyncOperation<IUnknown*> : IAsyncOperation_impl<IUnknown*> {};
+
+template <>
+struct __declspec(uuid("CD99A253-6473-4810-AF0D-763DAB79AC42"))
+    IAsyncOperationCompletedHandler<IUnknown*>
+    : IAsyncOperationCompletedHandler_impl<IUnknown*> {};
+
+template <>
+struct __declspec(uuid("CB52D855-8121-4AC8-A164-084A27FB377E"))
+    IAsyncOperation<int*> : IAsyncOperation_impl<int*> {};
+
+template <>
+struct __declspec(uuid("EA868415-A724-40BC-950A-C7DB6B1723C6"))
+    IAsyncOperationCompletedHandler<int*>
+    : IAsyncOperationCompletedHandler_impl<int*> {};
+
+// These specialization templates were included in windows.foundation.h, but
+// removed in 10.0.19041.0 SDK, so are included here conditionally
+#ifdef NTDDI_WIN10_VB  // Windows 10.0.19041
+template <>
+struct __declspec(uuid("968b9665-06ed-5744-8f53-8edeabd5f7b5"))
+    IAsyncOperation<int> : IAsyncOperation_impl<int> {};
+
+template <>
+struct __declspec(uuid("d60cae9d-88cb-59f1-8576-3fba44796be8"))
+    IAsyncOperationCompletedHandler<int>
+    : IAsyncOperationCompletedHandler_impl<int> {};
+#endif
+
+}  // namespace Foundation
+}  // namespace Windows
+}  // namespace ABI
+
+namespace base {
+namespace win {
+
+TEST(PostAsyncResultsTest, ValueType_Success) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<int>>();
+  ComPtr<IAsyncOperation<int>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  int value_received = 1;
+  ASSERT_EQ(
+      PostAsyncResults(async_op, base::BindLambdaForTesting([&](int result) {
+                         value_received = result;
+                         std::move(quit_closure).Run();
+                       })),
+      S_OK);
+
+  ASSERT_NO_FATAL_FAILURE(fake_iasync_op->CompleteWithResults(7));
+  run_loop.Run();
+  ASSERT_EQ(7, value_received);
+}
+
+TEST(PostAsyncResultsTest, ValueType_Failure) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<int>>();
+  ComPtr<IAsyncOperation<int>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  int value_received = 1;
+  ASSERT_EQ(
+      PostAsyncResults(async_op, base::BindLambdaForTesting([&](int result) {
+                         value_received = result;
+                         std::move(quit_closure).Run();
+                       })),
+      S_OK);
+
+  ASSERT_NO_FATAL_FAILURE(fake_iasync_op->CompleteWithError(E_FAIL));
+  run_loop.Run();
+  ASSERT_EQ(value_received, 0);
+}
+
+TEST(PostAsyncResultsTest, PointerType_Success) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<int*>>();
+  ComPtr<IAsyncOperation<int*>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  int* value_received = nullptr;
+  ASSERT_EQ(
+      PostAsyncResults(async_op, base::BindLambdaForTesting([&](int* result) {
+                         value_received = result;
+                         std::move(quit_closure).Run();
+                       })),
+      S_OK);
+
+  int test_value = 4;
+  ASSERT_NO_FATAL_FAILURE(fake_iasync_op->CompleteWithResults(&test_value));
+  run_loop.Run();
+  ASSERT_EQ(&test_value, value_received);
+}
+
+TEST(PostAsyncResultsTest, PointerType_Failure) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<int*>>();
+  ComPtr<IAsyncOperation<int*>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  int test_value = 2;
+  int* value_received = &test_value;
+  ASSERT_EQ(
+      PostAsyncResults(async_op, base::BindLambdaForTesting([&](int* result) {
+                         value_received = result;
+                         std::move(quit_closure).Run();
+                       })),
+      S_OK);
+
+  ASSERT_NO_FATAL_FAILURE(fake_iasync_op->CompleteWithError(E_FAIL));
+  run_loop.Run();
+  ASSERT_EQ(nullptr, value_received);
+}
+
+TEST(PostAsyncResultsTest, IUnknownType_Success) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<IUnknown*>>();
+  ComPtr<IAsyncOperation<IUnknown*>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  ComPtr<IUnknown> value_received = nullptr;
+  ASSERT_EQ(PostAsyncResults(async_op, base::BindLambdaForTesting(
+                                           [&](ComPtr<IUnknown> result) {
+                                             value_received = result;
+                                             std::move(quit_closure).Run();
+                                           })),
+            S_OK);
+
+  auto test_value = Microsoft::WRL::Make<TestClassImplementingIUnknown>();
+  ComPtr<IUnknown> value_to_send;
+  ASSERT_EQ(test_value.As(&value_to_send), S_OK);
+  ASSERT_NO_FATAL_FAILURE(
+      fake_iasync_op->CompleteWithResults(value_to_send.Get()));
+  run_loop.Run();
+  ASSERT_EQ(value_to_send.Get(), value_received.Get());
+}
+
+TEST(PostAsyncResultsTest, IUnknownType_Failure) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+  auto fake_iasync_op = Microsoft::WRL::Make<FakeIAsyncOperation<IUnknown*>>();
+  ComPtr<IAsyncOperation<IUnknown*>> async_op;
+  ASSERT_EQ(fake_iasync_op.As(&async_op), S_OK);
+
+  RunLoop run_loop;
+  auto quit_closure = run_loop.QuitClosure();
+  auto test_value = Microsoft::WRL::Make<TestClassImplementingIUnknown>();
+  ComPtr<IUnknown> value_received;
+  ASSERT_EQ(test_value.As(&value_received), S_OK);
+  ASSERT_EQ(PostAsyncResults(async_op, base::BindLambdaForTesting(
+                                           [&](ComPtr<IUnknown> result) {
+                                             value_received = result;
+                                             std::move(quit_closure).Run();
+                                           })),
+            S_OK);
+
+  ASSERT_NO_FATAL_FAILURE(fake_iasync_op->CompleteWithError(E_FAIL));
+  run_loop.Run();
+  ASSERT_EQ(nullptr, value_received.Get());
+}
+
+}  // namespace win
+}  // namespace base