fuchsia: Work around lack of packaging in Fuchsia tree build

Packaged test running seems to be a ways off, but with a bit of path
fiddling in test_paths.cc we can actually use the paths where the tests
are copied, so do that instead to get all the tests re-enabled. The
setup in BUILD.gn should be mostly-useful once packaging is working as
all helper/data files will need to specified there anyway.

Also, attempted fix to flaky behaviour in
ProcessReaderFuchsia.ChildThreads exposed because the tests are now
being run. zx_object_wait_many() waits on *any* of the objects, not
*all* of them. Derp!

And finally, for the same test, work around some unintuitive behaviour
in zx_task_suspend(), in particular that the thread will not be
suspended for the purpose of reading registers right away, but instead
only "sometime later", which appears in pratice to be after the next
context switch. Have ScopedTaskSuspend block for a while to try to
ensure the registers become readble, and if they don't, at least fail
noisily at that point.

Bug: crashpad:196
Change-Id: I01fb3590ede96301c941c2a88eba47fdbfe74ea7
Reviewed-on: https://chromium-review.googlesource.com/1053797
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>

BUILD.gn

@@ -40,12 +40,54 @@ if (crashpad_is_in_chromium || crashpad_is_in_fuchsia) {
 
       deps = [
         ":crashpad_tests",
+        "snapshot:crashpad_snapshot_test_both_dt_hash_styles(//build/toolchain/fuchsia:x64-shared)",
+        "snapshot:crashpad_snapshot_test_module(//build/toolchain/fuchsia:x64-shared)",
+        "snapshot:crashpad_snapshot_test_module_large(//build/toolchain/fuchsia:x64-shared)",
+        "snapshot:crashpad_snapshot_test_module_small(//build/toolchain/fuchsia:x64-shared)",
+        "test:crashpad_test_test_multiprocess_exec_test_child",
+        "util:http_transport_test_server",
       ]
 
       tests = [
         {
           name = "crashpad_tests"
         },
+        {
+          name = "crashpad_test_test_multiprocess_exec_test_child"
+        },
+        {
+          name = "http_transport_test_server"
+        },
+      ]
+
+      libraries = [
+        {
+          name = "crashpad_snapshot_test_both_dt_hash_styles.so"
+        },
+        {
+          name = "crashpad_snapshot_test_module.so"
+        },
+        {
+          name = "crashpad_snapshot_test_module_large.so"
+        },
+        {
+          name = "crashpad_snapshot_test_module_small.so"
+        },
+      ]
+
+      resources = [
+        {
+          path = rebase_path("util/net/testdata/ascii_http_body.txt")
+          dest = "crashpad_test_data/util/net/testdata/ascii_http_body.txt"
+        },
+        {
+          path = rebase_path("util/net/testdata/binary_http_body.dat")
+          dest = "crashpad_test_data/util/net/testdata/binary_http_body.dat"
+        },
+        {
+          path = rebase_path("test/test_paths_test_data_root.txt")
+          dest = "crashpad_test_data/test/test_paths_test_data_root.txt"
+        },
       ]
     }
 

snapshot/crashpad_info_client_options_test.cc

@@ -22,7 +22,6 @@
 #include "client/crashpad_info.h"
 #include "gtest/gtest.h"
 #include "test/errors.h"
-#include "test/gtest_disabled.h"
 #include "test/scoped_module_handle.h"
 #include "test/test_paths.h"
 
@@ -148,9 +147,6 @@ TEST(CrashpadInfoClientOptions, OneModule) {
 }
 
 TEST(CrashpadInfoClientOptions, TwoModules) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   // Open the module, which has its own CrashpadInfo structure.
   base::FilePath module_path =
       TestPaths::BuildArtifact(FILE_PATH_LITERAL("snapshot"),
@@ -244,9 +240,6 @@ class CrashpadInfoSizes_ClientOptions
     : public testing::TestWithParam<base::FilePath::StringType> {};
 
 TEST_P(CrashpadInfoSizes_ClientOptions, DifferentlySizedStruct) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   base::FilePath::StringType artifact(FILE_PATH_LITERAL("module_"));
   artifact += GetParam();
 

snapshot/elf/elf_image_reader_test.cc

@@ -21,7 +21,6 @@
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "test/multiprocess_exec.h"
 #include "test/process_type.h"
 #include "test/scoped_module_handle.h"
@@ -315,9 +314,6 @@ TEST(ElfImageReader, OneModuleChild) {
 // TODO(scottmg): Separately, the location of the ELF on Android needs some
 // work, and then the test could also be enabled there.
 TEST(ElfImageReader, DtHashAndDtGnuHashMatch) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   base::FilePath module_path =
       TestPaths::BuildArtifact(FILE_PATH_LITERAL("snapshot"),
                                FILE_PATH_LITERAL("both_dt_hash_styles"),

snapshot/fuchsia/process_reader_fuchsia_test.cc

@@ -17,9 +17,9 @@
 #include <pthread.h>
 #include <zircon/process.h>
 #include <zircon/syscalls.h>
+#include <zircon/syscalls/port.h>
 
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "test/multiprocess_exec.h"
 #include "test/test_paths.h"
 #include "util/fuchsia/scoped_task_suspend.h"
@@ -100,28 +100,33 @@ TEST(ProcessReaderFuchsia, ChildBasic) {
 }
 
 void* SignalAndSleep(void* arg) {
-  zx_object_signal(*reinterpret_cast<zx_handle_t*>(arg), 0, ZX_EVENT_SIGNALED);
+  zx_port_packet_t packet = {};
+  packet.type = ZX_PKT_TYPE_USER;
+  zx_port_queue(*reinterpret_cast<zx_handle_t*>(arg), &packet, 1);
   zx_nanosleep(UINT64_MAX);
   return nullptr;
 }
 
 CRASHPAD_CHILD_TEST_MAIN(ProcessReaderChildThreadsTestMain) {
   // Create 5 threads with stack sizes of 4096, 8192, ...
-  zx_handle_t events[5];
-  zx_wait_item_t items[arraysize(events)];
-  for (size_t i = 0; i < arraysize(events); ++i) {
-    pthread_t thread;
-    EXPECT_EQ(zx_event_create(0, &events[i]), ZX_OK);
+  zx_handle_t port;
+  zx_status_t status = zx_port_create(0, &port);
+  EXPECT_EQ(status, ZX_OK);
+
+  constexpr size_t kNumThreads = 5;
+  for (size_t i = 0; i < kNumThreads; ++i) {
     pthread_attr_t attr;
     EXPECT_EQ(pthread_attr_init(&attr), 0);
     EXPECT_EQ(pthread_attr_setstacksize(&attr, (i + 1) * 4096), 0);
-    EXPECT_EQ(pthread_create(&thread, &attr, &SignalAndSleep, &events[i]), 0);
-    items[i].waitfor = ZX_EVENT_SIGNALED;
-    items[i].handle = events[i];
+    pthread_t thread;
+    EXPECT_EQ(pthread_create(&thread, &attr, &SignalAndSleep, &port), 0);
   }
 
-  EXPECT_EQ(zx_object_wait_many(items, arraysize(items), ZX_TIME_INFINITE),
-            ZX_OK);
+  // Wait until all threads are ready.
+  for (size_t i = 0; i < kNumThreads; ++i) {
+    zx_port_packet_t packet;
+    zx_port_wait(port, ZX_TIME_INFINITE, &packet, 1);
+  }
 
   char c = ' ';
   CheckedWriteFile(
@@ -161,9 +166,6 @@ class ThreadsChildTest : public MultiprocessExec {
 };
 
 TEST(ProcessReaderFuchsia, ChildThreads) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   ThreadsChildTest test;
   test.Run();
 }

test/multiprocess_exec_test.cc

@@ -19,7 +19,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "test/test_paths.h"
 #include "util/file/file_io.h"
 
@@ -49,9 +48,6 @@ class TestMultiprocessExec final : public MultiprocessExec {
 };
 
 TEST(MultiprocessExec, MultiprocessExec) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   TestMultiprocessExec multiprocess_exec;
   base::FilePath child_test_executable = TestPaths::BuildArtifact(
       FILE_PATH_LITERAL("test"),

test/test_paths.cc

@@ -45,6 +45,13 @@ bool IsTestDataRoot(const base::FilePath& candidate) {
 base::FilePath TestDataRootInternal() {
 #if defined(OS_FUCHSIA)
   base::FilePath asset_path("/pkg/assets");
+#if defined(CRASHPAD_IS_IN_FUCHSIA)
+  // Tests are not yet packaged when running in the Fuchsia tree, so assets do
+  // not appear as expected at /pkg/assets. Override the default so that tests
+  // can find their data for now.
+  // https://crashpad.chromium.org/bug/196.
+  asset_path = base::FilePath("/system/data/crashpad_test_data");
+#endif
   if (!IsTestDataRoot(asset_path)) {
     LOG(WARNING) << "Test data root seems invalid, continuing anyway";
   }
@@ -121,6 +128,13 @@ base::FilePath Output32BitDirectory() {
 base::FilePath TestPaths::Executable() {
   base::FilePath executable_path;
   CHECK(Paths::Executable(&executable_path));
+#if defined(CRASHPAD_IS_IN_FUCHSIA)
+  // Tests are not yet packaged when running in the Fuchsia tree, so binaries do
+  // not appear as expected at /pkg/bin. Override the default of /pkg/bin/app
+  // so that tests can find the correct location for now.
+  // https://crashpad.chromium.org/bug/196.
+  executable_path = base::FilePath("/system/test/app");
+#endif
   return executable_path;
 }
 
@@ -188,7 +202,15 @@ base::FilePath TestPaths::BuildArtifact(
 #if defined(OS_WIN)
       extension = FILE_PATH_LITERAL(".exe");
 #elif defined(OS_FUCHSIA)
+#if defined(CRASHPAD_IS_IN_FUCHSIA)
+      // Tests are not yet packaged when running in the Fuchsia tree, so
+      // binaries do not appear as expected at /pkg/bin. Override the default of
+      // /pkg/bin/app so that tests can find the correct location for now.
+      // https://crashpad.chromium.org/bug/196.
+      directory = base::FilePath(FILE_PATH_LITERAL("/system/test"));
+#else
       directory = base::FilePath(FILE_PATH_LITERAL("/pkg/bin"));
+#endif
 #endif  // OS_WIN
       break;
 
@@ -222,14 +244,5 @@ bool TestPaths::Has32BitBuildArtifacts() {
 
 #endif  // defined(OS_WIN) && defined(ARCH_CPU_64_BITS)
 
-// static
-bool TestPaths::ExternalFilesUnavailable() {
-#if defined(OS_FUCHSIA) && defined(CRASHPAD_IS_IN_FUCHSIA)
-  return true;
-#else
-  return false;
-#endif
-}
-
 }  // namespace test
 }  // namespace crashpad

test/test_paths.h

@@ -138,15 +138,6 @@ class TestPaths {
   static bool Has32BitBuildArtifacts();
 #endif  // OS_WIN && ARCH_CPU_64_BITS
 
-  //! \return `true` if no external data files are available.
-  //!
-  //! TODO(scottmg): https://crashpad.chromium.org/bug/196: This is a temporary
-  //! hack to disable tests that require external files, either to relaunch
-  //! subprocesses, or to load data files. The Fuchsia packaging system is in
-  //! flux (see PKG-46 upstream) and when that's resolved this should be
-  //! deleted.
-  static bool ExternalFilesUnavailable();
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(TestPaths);
 };
 

test/test_paths_test.cc

@@ -16,7 +16,6 @@
 
 #include "base/files/file_path.h"
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "util/file/file_io.h"
 
 namespace crashpad {
@@ -24,9 +23,6 @@ namespace test {
 namespace {
 
 TEST(TestPaths, TestDataRoot) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   base::FilePath test_data_root = TestPaths::TestDataRoot();
   ScopedFileHandle file(LoggingOpenFileForRead(
       test_data_root.Append(FILE_PATH_LITERAL("test"))

util/fuchsia/scoped_task_suspend.cc

@@ -16,6 +16,7 @@
 
 #include <zircon/process.h>
 #include <zircon/syscalls.h>
+#include <zircon/syscalls/debug.h>
 
 #include <vector>
 
@@ -39,10 +40,33 @@ zx_obj_type_t GetHandleType(zx_handle_t handle) {
   return basic.type;
 }
 
-bool SuspendThread(zx_handle_t thread) {
+enum class SuspensionResult {
+  FailedSuspendCall,
+  FailedToSuspendInTimelyFashion,
+  Succeeded,
+};
+
+SuspensionResult SuspendThread(zx_handle_t thread) {
   zx_status_t status = zx_task_suspend(thread);
   ZX_LOG_IF(ERROR, status != ZX_OK, status) << "zx_task_suspend";
-  return status == ZX_OK;
+  if (status != ZX_OK)
+    return SuspensionResult::FailedSuspendCall;
+  // zx_task_suspend() suspends the thread "sometime soon", but it's hard to
+  // use when it's not guaranteed to be suspended after return. Try reading the
+  // thread state until the registers are retrievable, which means that the
+  // thread is actually suspended. Don't wait forever in case the suspend
+  // failed for whatever reason, but try a few times.
+  for (int i = 0; i < 5; ++i) {
+    zx_thread_state_general_regs_t regs;
+    status = zx_thread_read_state(
+        thread, ZX_THREAD_STATE_GENERAL_REGS, &regs, sizeof(regs));
+    if (status == ZX_OK) {
+      return SuspensionResult::Succeeded;
+    }
+    zx_nanosleep(zx_deadline_after(ZX_MSEC(10)));
+  }
+  LOG(ERROR) << "thread failed to suspend";
+  return SuspensionResult::FailedToSuspendInTimelyFashion;
 }
 
 bool ResumeThread(zx_handle_t thread) {
@@ -59,7 +83,11 @@ ScopedTaskSuspend::ScopedTaskSuspend(zx_handle_t task) : task_(task) {
 
   zx_obj_type_t type = GetHandleType(task_);
   if (type == ZX_OBJ_TYPE_THREAD) {
-    if (!SuspendThread(task_)) {
+    // Note that task_ is only marked invalid if the zx_task_suspend() call
+    // completely fails, otherwise the suspension might just not have taken
+    // effect yet, so avoid leaving it suspended forever by still resuming on
+    // destruction.
+    if (SuspendThread(task_) == SuspensionResult::FailedSuspendCall) {
       task_ = ZX_HANDLE_INVALID;
     }
   } else if (type == ZX_OBJ_TYPE_PROCESS) {

util/net/http_body_test.cc

@@ -17,7 +17,6 @@
 #include <string.h>
 
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "test/test_paths.h"
 #include "util/misc/implicit_cast.h"
 #include "util/net/http_body_test_util.h"
@@ -98,9 +97,6 @@ TEST(StringHTTPBodyStream, MultipleReads) {
 }
 
 TEST(FileReaderHTTPBodyStream, ReadASCIIFile) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   base::FilePath path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
 
@@ -121,9 +117,6 @@ TEST(FileReaderHTTPBodyStream, ReadASCIIFile) {
 }
 
 TEST(FileReaderHTTPBodyStream, ReadBinaryFile) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   // HEX contents of file: |FEEDFACE A11A15|.
   base::FilePath path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/binary_http_body.dat"));
@@ -194,9 +187,6 @@ TEST_P(CompositeHTTPBodyStreamBufferSize, ThreeStringParts) {
 }
 
 TEST_P(CompositeHTTPBodyStreamBufferSize, StringsAndFile) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   std::string string1("Hello! ");
   std::string string2(" Goodbye :)");
 

util/net/http_multipart_builder_test.cc

@@ -20,7 +20,6 @@
 
 #include "gtest/gtest.h"
 #include "test/gtest_death.h"
-#include "test/gtest_disabled.h"
 #include "test/test_paths.h"
 #include "util/net/http_body.h"
 #include "util/net/http_body_test_util.h"
@@ -100,9 +99,6 @@ TEST(HTTPMultipartBuilder, ThreeStringFields) {
 }
 
 TEST(HTTPMultipartBuilder, ThreeFileAttachments) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   base::FilePath ascii_http_body_path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
@@ -187,9 +183,6 @@ TEST(HTTPMultipartBuilder, OverwriteFormDataWithEscapedKey) {
 }
 
 TEST(HTTPMultipartBuilder, OverwriteFileAttachment) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   static constexpr char kValue[] = "1 2 3 test";
   builder.SetFormData("a key", kValue);
@@ -248,9 +241,6 @@ TEST(HTTPMultipartBuilder, OverwriteFileAttachment) {
 }
 
 TEST(HTTPMultipartBuilder, SharedFormDataAndAttachmentKeyNamespace) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   static constexpr char kValue1[] = "11111";
   builder.SetFormData("one", kValue1);

util/net/http_transport_test.cc

@@ -30,7 +30,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "gtest/gtest.h"
-#include "test/gtest_disabled.h"
 #include "test/multiprocess_exec.h"
 #include "test/test_paths.h"
 #include "util/file/file_io.h"
@@ -211,9 +210,6 @@ void ValidFormData(HTTPTransportTestFixture* fixture,
 }
 
 TEST(HTTPTransport, ValidFormData) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   builder.SetFormData("key1", "test");
   builder.SetFormData("key2", "--abcdefg123");
@@ -227,9 +223,6 @@ TEST(HTTPTransport, ValidFormData) {
 }
 
 TEST(HTTPTransport, ValidFormData_Gzip) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   builder.SetGzipEnabled(true);
   builder.SetFormData("key1", "test");
@@ -253,9 +246,6 @@ void ErrorResponse(HTTPTransportTestFixture* fixture,
 }
 
 TEST(HTTPTransport, ErrorResponse) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   HTTPMultipartBuilder builder;
   HTTPHeaders headers;
   headers[kContentType] = kTextPlain;
@@ -284,9 +274,6 @@ void UnchunkedPlainText(HTTPTransportTestFixture* fixture,
 }
 
 TEST(HTTPTransport, UnchunkedPlainText) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   std::unique_ptr<HTTPBodyStream> body_stream(
       new StringHTTPBodyStream(kTextBody));
 
@@ -327,16 +314,10 @@ void RunUpload33k(bool has_content_length) {
 }
 
 TEST(HTTPTransport, Upload33k) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   RunUpload33k(true);
 }
 
 TEST(HTTPTransport, Upload33k_LengthUnknown) {
-  if (TestPaths::ExternalFilesUnavailable())
-    DISABLED_TEST();
-
   // The same as Upload33k, but without declaring Content-Length ahead of time.
   RunUpload33k(false);
 }