New issue
Advanced search Search tips

Issue 778672 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 777924



Sign in to add a comment

Jobs complete successfully on swarming but show up as failed on buildbot

Project Member Reported by thakis@chromium.org, Oct 26 2017

Issue description

Comment 1 by thakis@chromium.org, Oct 26 2017

Cc: dpranke@chromium.org

Comment 2 by thakis@chromium.org, Oct 26 2017

Blocking: 777924

Comment 3 by thakis@chromium.org, Oct 26 2017

Scottmg says: """ I think this is because swarming runs require the use of the stuff in the src test launcher to get --test-launcher-summary-output to get the results back as .json, but that's not in standard gtest that crashpad tests are using."""

Is there any way forward here for these tests then?

Comment 4 by thakis@chromium.org, Oct 26 2017

Cc: scottmg@chromium.org

Comment 5 by maruel@google.com, Oct 26 2017

To clarify, the swarming recipe, not swarming itself.
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/swarming/api.py?l=477

(I'm not sure that the work in crbug.com/743139 has much payoff. :/)

I guess we could do something similar to what we do with base for crashpad and build against a different main() (the test harness launcher) so that it gets --test-launcher-summary-output. That'll mostly require a variety of build file fiddling to switch entry points in and out. I don't think Crashpad uses any custom special main.ccs, rather all using test.gyp:crashpad_gtest_main, so they should be fairly easily switchable.

Alternatively, we could let the swarming recipe respect the exit code and not report the richer information from the .json (which would have been a lot easier for Fuchsia bots too, though not as good in the long term).

I wouldn't do the third option of having crashpad's test runner emit the required json as it's pretty adhoc and complex.

Comment 7 by mark@chromium.org, Oct 27 2017

Cc: mark@chromium.org
I’m also not too interested in duplicating Chromium’s test launcher stuff in mini_chromium.

If running the tests on Chromium bots for swarming requires a nonstandard gtest/gmock main() launcher, then we should probably just use Chromium’s when running Crashpad tests in Chromium.

Crashpad doesn’t use the standard gtest/gmock-provided main(), it does need to do a couple of things to the test environment before invoking tests:

https://chromium.googlesource.com/crashpad/crashpad/+/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gtest_main.cc
https://chromium.googlesource.com/crashpad/crashpad/+/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gmock_main.cc

There are two custom things we do beyond jumping into the tests: we save off the argc and argv passed to main() (this is for our test that checks that we’re able to read argv from out of process), and we register the new dynamic test disabling stuff (https://chromium.googlesource.com/crashpad/crashpad/+/c49da9caef302ad03f7161c58edf34825fd0171e/test/gtest_disabled.h) with gtest.

Looking around at things that call base::LaunchUnitTests, it seems that there’s precedent for modules that need it to have their own main() do set-up. We should be able to do that for Crashpad’s tests too.

Using https://codesearch.chromium.org/search/?q=testing::AddGlobalTestEnvironment+base::LaunchUnitTests as a guide, it appears that there wouldn’t be any trouble with using a main() that does Crashpad-specific setup and then lets Chromium’s test launcher take over. This would basically be the same as Crashpad’s current test main()s, except it’d use base::LaunchUnitTests() instead of gtest’s RUN_ALL_TESTS(). In fact, we could even just do this in Crashpad’s existing test main()s with an #ifdef that detected that it was being built in Chromium.

Comment 8 by thakis@chromium.org, Oct 27 2017

is there a define to detect that already?

Comment 9 by thakis@chromium.org, Oct 27 2017

"that" being "crashpad is being built as part of chromium", that is.

Comment 10 by mark@chromium.org, Oct 27 2017

We used to have one when we shared a build system, but we tore it out when Chromium deleted its .gyp files.

My suggestion would be to define CRASHPAD_IN_CHROMIUM in the Crashpad BUILD.gn files.

Really, we don’t want the shippy parts of Chromium Crashpad to be different from non-Chromium Crashpad. The switchiness was mostly just to deal with a few paths being different when Crashpad’s in Chrome’s tree. An adaptation to fitting into Chromium’s test infrastructure is just fine, though.

Comment 11 by mark@chromium.org, Oct 27 2017

Or just pick up defined(CHROMIUM_BUILD) || defined(GOOGLE_CHROME_BUILD).
Probably better to do this here than on the review.

I did the "if in chromium, use TestLauncher" thing Mark suggested on the swarming bug, see latest patch set (even though it seems a bit unfortunate that swarming doesn't just work with gtest). Things link, but at least one test fails under TestLauncher that was happy before:

C:\src\chrome\src>out\gn\crashpad_util_test.exe --gtest_filter=ExceptionHandlerServerTest.MultipleConnections --single-process-tests
Note: Google Test filter = ExceptionHandlerServerTest.MultipleConnections
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExceptionHandlerServerTest
[ RUN      ] ExceptionHandlerServerTest.MultipleConnections
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = ExceptionHandlerServerTest.MultipleConnections
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExceptionHandlerServerTest
[ RUN      ] ExceptionHandlerServerTest.MultipleConnections
[4036:8948:1027/122737.451:767767765:ERROR:file_io.cc(134)] WriteFile: The handle is invalid. (0x6)
[4036:8948:1027/122737.458:767767765:FATAL:file_io.cc(146)] Check failed: LoggingWriteFile(file, buffer, size).
Backtrace:
        base::debug::StackTrace::StackTrace [0x008BBDE0+32]
        base::debug::StackTrace::StackTrace [0x008BB76D+13]
        logging::LogMessage::~LogMessage [0x008B02FE+78]
        crashpad::CheckedWriteFile [0x008A1BED+81]
        crashpad::test::WinChildProcess::WinChildProcess [0x00901CC0+456]
        std::unique_ptr<crashpad::test::WinChildProcess::Handles,std::default_delete<crashpad::test::WinChildProcess::Handles> >::~unique_pt
r<crashpad::test::WinChildProcess::Handles,std::default_delete<crashpad::test::WinChildProcess::Handles> > [0x0087E579+97]
        base::trace_event::HeapProfilerEventFilter::`scalar deleting destructor' [0x0087E22A+1418]
        testing::Test::Run [0x008992E5+193]
        testing::TestInfo::Run [0x00899967+213]
        testing::TestCase::Run [0x00899D32+246]
        testing::internal::UnitTestImpl::RunAllTests [0x0089DC8E+606]
        testing::UnitTest::Run [0x0089D954+178]
        base::TestSuite::Run [0x0090B104+100]
        std::unique_ptr<logging::ScopedLogAssertHandler,std::default_delete<logging::ScopedLogAssertHandler> >::reset [0x0090BC24+294]
        base::LaunchUnitTestsSerially [0x0090C031+105]
        main [0x0090AF51+121]
        __scrt_common_main_seh [0x0095609A+248] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
        BaseThreadInitThunk [0x7686336A+18]
        RtlInitializeExceptionChain [0x773298F2+99]
        RtlInitializeExceptionChain [0x773298C5+54]
[1/1] ExceptionHandlerServerTest.MultipleConnections (CRASHED)
1 test crashed:
    ExceptionHandlerServerTest.MultipleConnections (../../third_party/crashpad/crashpad/util/win/exception_handler_server_test.cc:183)
Tests took 0 seconds.

https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/test/win/win_child_process.cc?type=cs&sq=package:chromium&l=159
Also

[ RUN      ] ScopedProcessSuspend.ScopedProcessSuspend
[14080:9412:1027/123536.655:768246954:ERROR:file_io.cc(134)] WriteFile: The handle is invalid. (0x6)
[14080:9412:1027/123536.657:768246954:FATAL:file_io.cc(146)] Check failed: LoggingWriteFile(file, buffer, size).
Backtrace:
        base::debug::StackTrace::StackTrace [0x0031BDE0+32]
        base::debug::StackTrace::StackTrace [0x0031B76D+13]
        logging::LogMessage::~LogMessage [0x003102FE+78]
        crashpad::CheckedWriteFile [0x00301BED+81]
        crashpad::test::WinChildProcess::WinChildProcess [0x00361CC0+456]
        std::vector<crashpad::CheckedRange<unsigned __int64,unsigned __int64>,std::allocator<crashpad::CheckedRange<unsigned __int64,unsigne
d __int64> > >::_Tidy [0x002E9899+24615]
        std::vector<crashpad::CheckedRange<unsigned __int64,unsigned __int64>,std::allocator<crashpad::CheckedRange<unsigned __int64,unsigne
d __int64> > >::_Tidy [0x002E9335+23235]
        testing::Test::Run [0x002F92E5+193]
        testing::TestInfo::Run [0x002F9967+213]
        testing::TestCase::Run [0x002F9D32+246]
        testing::internal::UnitTestImpl::RunAllTests [0x002FDC8E+606]
        testing::UnitTest::Run [0x002FD954+178]
        base::TestSuite::Run [0x0036B104+100]
        std::unique_ptr<logging::ScopedLogAssertHandler,std::default_delete<logging::ScopedLogAssertHandler> >::reset [0x0036BC24+294]
        base::LaunchUnitTestsSerially [0x0036C031+105]
        main [0x0036AF51+121]
        __scrt_common_main_seh [0x003B609A+248] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
        BaseThreadInitThunk [0x7686336A+18]
        RtlInitializeExceptionChain [0x773298F2+99]
        RtlInitializeExceptionChain [0x773298C5+54]

[1/1] ScopedProcessSuspend.ScopedProcessSuspend (CRASHED)
1 test crashed:
    ScopedProcessSuspend.ScopedProcessSuspend (../../third_party/crashpad/crashpad/util/win/scoped_process_suspend_test.cc:92)

Comment 14 by mark@chromium.org, Oct 27 2017

OK, I’ll say it again here.

Ah, the WinMultiprocess (--is-multiprocess-child) tests, of which this is one, are a little weird and fragile.
With those two disabled, things pass locally at least, even when not running serially.
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00a0654929787f70b0cd81f30aa14e81c5e11b2f

commit 00a0654929787f70b0cd81f30aa14e81c5e11b2f
Author: Nico Weber <thakis@chromium.org>
Date: Fri Oct 27 19:20:10 2017

Build and run crashpad_util_test on Windows bots.

Ran

    python tools\mb\mb.py run out\gn crashpad_util_test

to locally verify data and data_deps entries for swarming.

Swarming apparently requires the use of base::TestLauncher, so
use that in Crashpad-in-Chromium builds. Disable a bunch of tests
that fail in that setup. 

Bug:  777924 , 778672 
Change-Id: I4aa828040d488755582e5559d5f117e1ed8d1c86
Reviewed-on: https://chromium-review.googlesource.com/737439
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512262}
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/build/secondary/third_party/crashpad/crashpad/test/BUILD.gn
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/build/secondary/third_party/crashpad/crashpad/util/BUILD.gn
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.win.json
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/gn_isolate_map.pyl
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/test/gmock_main.cc
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/test/gtest_main.cc
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/exception_handler_server_test.cc
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/process_info_test.cc
[modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/scoped_process_suspend_test.cc

Status: Fixed (was: Untriaged)
Fixed what I wanted to fix here. It feels like infra could do more work here (have an understandable error; make vanilla gtest tests work on swarming directly), but I'm happy here for now.
Owner: thakis@chromium.org

Sign in to add a comment