diff --git a/BUILD.gn b/BUILD.gn index a23bbd3e..c9da7c8a 100644 --- a/BUILD.gn +++ b/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" + }, ] } diff --git a/snapshot/crashpad_info_client_options_test.cc b/snapshot/crashpad_info_client_options_test.cc index f23a179f..1fe38acf 100644 --- a/snapshot/crashpad_info_client_options_test.cc +++ b/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 {}; TEST_P(CrashpadInfoSizes_ClientOptions, DifferentlySizedStruct) { - if (TestPaths::ExternalFilesUnavailable()) - DISABLED_TEST(); - base::FilePath::StringType artifact(FILE_PATH_LITERAL("module_")); artifact += GetParam(); diff --git a/snapshot/elf/elf_image_reader_test.cc b/snapshot/elf/elf_image_reader_test.cc index e0f0ea3d..d64fd5c4 100644 --- a/snapshot/elf/elf_image_reader_test.cc +++ b/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"), diff --git a/snapshot/fuchsia/process_reader_fuchsia_test.cc b/snapshot/fuchsia/process_reader_fuchsia_test.cc index b5997fe6..329752b8 100644 --- a/snapshot/fuchsia/process_reader_fuchsia_test.cc +++ b/snapshot/fuchsia/process_reader_fuchsia_test.cc @@ -17,9 +17,9 @@ #include #include #include +#include #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(arg), 0, ZX_EVENT_SIGNALED); + zx_port_packet_t packet = {}; + packet.type = ZX_PKT_TYPE_USER; + zx_port_queue(*reinterpret_cast(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(); } diff --git a/test/multiprocess_exec_test.cc b/test/multiprocess_exec_test.cc index 1f3c2449..99a1b01f 100644 --- a/test/multiprocess_exec_test.cc +++ b/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"), diff --git a/test/test_paths.cc b/test/test_paths.cc index 674d04f4..b06fb7f3 100644 --- a/test/test_paths.cc +++ b/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 diff --git a/test/test_paths.h b/test/test_paths.h index df07b5fe..5540f77d 100644 --- a/test/test_paths.h +++ b/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); }; diff --git a/test/test_paths_test.cc b/test/test_paths_test.cc index cec54543..9d43b594 100644 --- a/test/test_paths_test.cc +++ b/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")) diff --git a/util/fuchsia/scoped_task_suspend.cc b/util/fuchsia/scoped_task_suspend.cc index 594edf5c..c5871963 100644 --- a/util/fuchsia/scoped_task_suspend.cc +++ b/util/fuchsia/scoped_task_suspend.cc @@ -16,6 +16,7 @@ #include #include +#include #include @@ -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, ®s, 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) { diff --git a/util/net/http_body_test.cc b/util/net/http_body_test.cc index caa53c28..e48cbf91 100644 --- a/util/net/http_body_test.cc +++ b/util/net/http_body_test.cc @@ -17,7 +17,6 @@ #include #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 :)"); diff --git a/util/net/http_multipart_builder_test.cc b/util/net/http_multipart_builder_test.cc index d1bcf544..dab9e7e7 100644 --- a/util/net/http_multipart_builder_test.cc +++ b/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); diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index f8413338..abaa1727 100644 --- a/util/net/http_transport_test.cc +++ b/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 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); }