Reland "Re-enable EngineRequestsNoBlockingTest on Win8 by initializing...

Reland "Re-enable EngineRequestsNoBlockingTest on Win8 by initializing TaskEnvironment with COM disabled"

This is a reland of 2dd4658f with a fixed unit test.

The original was using CHECK_DEATH to test a code path that called DCHECK.

Original change's description:
> Re-enable EngineRequestsNoBlockingTest on Win8 by initializing TaskEnvironment with COM disabled
>
> R=fdoray
>
> Bug: 947576
> Change-Id: I1f901367e6fcc5243f34f50e5704b68727a40731
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1990135
> Commit-Queue: Joe Mason <joenotcharles@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#729802}

R=fdoray

Bug: 947576
Change-Id: If83301ccc260f5ca6911c7ee787eb7719ef2c064
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1993842Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730216}

base/test/task_environment.cc

@@ -354,11 +354,13 @@ TaskEnvironment::TaskEnvironment(
     MainThreadType main_thread_type,
     ThreadPoolExecutionMode thread_pool_execution_mode,
     ThreadingMode threading_mode,
+    ThreadPoolCOMEnvironment thread_pool_com_environment,
     bool subclass_creates_default_taskrunner,
     trait_helpers::NotATraitTag)
     : main_thread_type_(main_thread_type),
       thread_pool_execution_mode_(thread_pool_execution_mode),
       threading_mode_(threading_mode),
+      thread_pool_com_environment_(thread_pool_com_environment),
       subclass_creates_default_taskrunner_(subclass_creates_default_taskrunner),
       sequence_manager_(
           CreateSequenceManagerForMainThreadType(main_thread_type)),
@@ -434,18 +436,10 @@ void TaskEnvironment::InitializeThreadPool() {
   ThreadPoolInstance::InitParams init_params(kMaxThreads);
   init_params.suggested_reclaim_time = TimeDelta::Max();
 #if defined(OS_WIN)
-  // Enable the MTA in unit tests to match the browser process's
-  // ThreadPoolInstance configuration.
-  //
-  // This has the adverse side-effect of enabling the MTA in non-browser unit
-  // tests as well but the downside there is not as bad as not having it in
-  // browser unit tests. It just means some COM asserts may pass in unit tests
-  // where they wouldn't in integration tests or prod. That's okay because unit
-  // tests are already generally very loose on allowing I/O, waits, etc. Such
-  // misuse will still be caught in later phases (and COM usage should already
-  // be pretty much inexistent in sandboxed processes).
-  init_params.common_thread_pool_environment =
-      ThreadPoolInstance::InitParams::CommonThreadPoolEnvironment::COM_MTA;
+  if (thread_pool_com_environment_ == ThreadPoolCOMEnvironment::COM_MTA) {
+    init_params.common_thread_pool_environment =
+        ThreadPoolInstance::InitParams::CommonThreadPoolEnvironment::COM_MTA;
+  }
 #endif
 
   auto task_tracker = std::make_unique<TestTaskTracker>();

base/test/task_environment.h

@@ -156,6 +156,28 @@ class TaskEnvironment {
     DEFAULT = MULTIPLE_THREADS
   };
 
+  // On Windows, sets the COM environment for the ThreadPoolInstance. Ignored
+  // on other platforms.
+  enum class ThreadPoolCOMEnvironment {
+    // Do not initialize COM for the pool's workers.
+    NONE,
+
+    // Place the pool's workers in a COM MTA.
+    COM_MTA,
+
+    // Enable the MTA by default in unit tests to match the browser process's
+    // ThreadPoolInstance configuration.
+    //
+    // This has the adverse side-effect of enabling the MTA in non-browser unit
+    // tests as well but the downside there is not as bad as not having it in
+    // browser unit tests. It just means some COM asserts may pass in unit
+    // tests where they wouldn't in integration tests or prod. That's okay
+    // because unit tests are already generally very loose on allowing I/O,
+    // waits, etc. Such misuse will still be caught in later phases (and COM
+    // usage should already be pretty much inexistent in sandboxed processes).
+    DEFAULT = COM_MTA,
+  };
+
   // List of traits that are valid inputs for the constructor below.
   struct ValidTraits {
     ValidTraits(TimeSource);
@@ -163,6 +185,7 @@ class TaskEnvironment {
     ValidTraits(ThreadPoolExecutionMode);
     ValidTraits(SubclassCreatesDefaultTaskRunner);
     ValidTraits(ThreadingMode);
+    ValidTraits(ThreadPoolCOMEnvironment);
   };
 
   // Constructor accepts zero or more traits which customize the testing
@@ -180,6 +203,9 @@ class TaskEnvironment {
                                    ThreadPoolExecutionMode::DEFAULT>(traits...),
             trait_helpers::GetEnum<ThreadingMode, ThreadingMode::DEFAULT>(
                 traits...),
+            trait_helpers::GetEnum<ThreadPoolCOMEnvironment,
+                                   ThreadPoolCOMEnvironment::DEFAULT>(
+                traits...),
             trait_helpers::HasTrait<SubclassCreatesDefaultTaskRunner,
                                     TaskEnvironmentTraits...>(),
             trait_helpers::NotATraitTag()) {}
@@ -329,12 +355,14 @@ class TaskEnvironment {
                   MainThreadType main_thread_type,
                   ThreadPoolExecutionMode thread_pool_execution_mode,
                   ThreadingMode threading_mode,
+                  ThreadPoolCOMEnvironment thread_pool_com_environment,
                   bool subclass_creates_default_taskrunner,
                   trait_helpers::NotATraitTag tag);
 
   const MainThreadType main_thread_type_;
   const ThreadPoolExecutionMode thread_pool_execution_mode_;
   const ThreadingMode threading_mode_;
+  const ThreadPoolCOMEnvironment thread_pool_com_environment_;
   const bool subclass_creates_default_taskrunner_;
 
   std::unique_ptr<sequence_manager::SequenceManager> sequence_manager_;

base/test/task_environment_unittest.cc

@@ -44,6 +44,10 @@
 #include "base/files/file_descriptor_watcher_posix.h"
 #endif  // defined(OS_POSIX)
 
+#if defined(OS_WIN)
+#include "base/win/scoped_com_initializer.h"
+#endif
+
 namespace base {
 namespace test {
 
@@ -1199,5 +1203,58 @@ TEST_F(TaskEnvironmentTest, CurrentThread) {
   run_loop.Run();
 }
 
+#if defined(OS_WIN)
+namespace {
+
+enum class ApartmentType {
+  kSTA,
+  kMTA,
+};
+
+void InitializeSTAApartment() {
+  base::win::ScopedCOMInitializer initializer;
+  EXPECT_TRUE(initializer.Succeeded());
+}
+
+void InitializeMTAApartment() {
+  base::win::ScopedCOMInitializer initializer(
+      base::win::ScopedCOMInitializer::kMTA);
+  EXPECT_TRUE(initializer.Succeeded());
+}
+
+void InitializeCOMOnWorker(
+    TaskEnvironment::ThreadPoolCOMEnvironment com_environment,
+    ApartmentType apartment_type) {
+  TaskEnvironment task_environment(com_environment);
+  PostTask(FROM_HERE, BindOnce(apartment_type == ApartmentType::kSTA
+                                   ? &InitializeSTAApartment
+                                   : &InitializeMTAApartment));
+  task_environment.RunUntilIdle();
+}
+
+}  // namespace
+
+TEST_F(TaskEnvironmentTest, DefaultCOMEnvironment) {
+  // Attempt to initialize an MTA COM apartment. Expect this to succeed since
+  // the thread is already in an MTA apartment.
+  InitializeCOMOnWorker(TaskEnvironment::ThreadPoolCOMEnvironment::DEFAULT,
+                        ApartmentType::kMTA);
+
+  // Attempt to initialize an STA COM apartment. Expect this to fail since the
+  // thread is already in an MTA apartment.
+  EXPECT_DCHECK_DEATH(InitializeCOMOnWorker(
+      TaskEnvironment::ThreadPoolCOMEnvironment::DEFAULT, ApartmentType::kSTA));
+}
+
+TEST_F(TaskEnvironmentTest, NoCOMEnvironment) {
+  // Attempt to initialize both MTA and STA COM apartments. Both should succeed
+  // when the thread is not already in an apartment.
+  InitializeCOMOnWorker(TaskEnvironment::ThreadPoolCOMEnvironment::NONE,
+                        ApartmentType::kMTA);
+  InitializeCOMOnWorker(TaskEnvironment::ThreadPoolCOMEnvironment::NONE,
+                        ApartmentType::kSTA);
+}
+#endif
+
 }  // namespace test
 }  // namespace base

chrome/chrome_cleaner/engines/broker/engine_requests_no_blocking_unittest.cc

@@ -437,7 +437,9 @@ class TestEngineRequestInvoker {
 };
 
 MULTIPROCESS_TEST_MAIN(EngineRequestsNoBlocking) {
-  base::test::TaskEnvironment task_environment;
+  // COM can't be initialized inside the sandbox.
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::ThreadPoolCOMEnvironment::NONE);
 
   auto child_process = SetupSandboxedChildProcess();
   if (!child_process)
@@ -520,11 +522,6 @@ class EngineRequestsNoBlockingTest
     : public ::testing::TestWithParam<const char*> {};
 
 TEST_P(EngineRequestsNoBlockingTest, TestRequest) {
-  // All of these tests fail when run on win8 bots so return right away.
-  // TODO(crbug.com/947576): Find out why and re-enable them.
-  if (base::win::GetVersion() == base::win::Version::WIN8)
-    return;
-
   base::test::TaskEnvironment task_environment;
 
   // This event will be shared between the parent and child processes. The