Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add crash recovery on Windows #5012

Merged
merged 28 commits into from
Dec 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Checks: "-*,
-readability-function-cognitive-complexity,
-bugprone-easily-swappable-parameters,
-cert-err58-cpp,
-modernize-avoid-c-arrays
"
CheckOptions:
- key: readability-identifier-naming.ClassCase
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ jobs:
run: |
cd build
set cl=/MP
nmake /S /NOLOGO crashpad_handler
nmake /S /NOLOGO chatterino-crash-handler
mkdir Chatterino2/crashpad
cp bin/crashpad/crashpad_handler.exe Chatterino2/crashpad/crashpad_handler.exe
7z a bin/chatterino-Qt-${{ matrix.qt-version }}.pdb.7z bin/chatterino.pdb
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/test-windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ jobs:
..
set cl=/MP
nmake /S /NOLOGO
nmake /S /NOLOGO chatterino-crash-handler-test
working-directory: build-test

- name: Download and extract Twitch PubSub Server Test
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
- Bugfix: Fixed support for Windows 11 Snap layouts. (#4994)
- Bugfix: Fixed some windows appearing between screens. (#4797)
- Bugfix: Fixed a bug on Wayland where tooltips would spawn as separate windows instead of behaving like tooltips. (#4998)
- Bugfix: Fixed _Restart on crash_ option not working on Windows. (#5012)
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
- Dev: Run miniaudio in a separate thread, and simplify it to not manage the device ourselves. There's a chance the simplification is a bad idea. (#4978)
- Dev: Change clang-format from v14 to v16. (#4929)
- Dev: Fixed UTF16 encoding of `modes` file for the installer. (#4791)
Expand Down
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,6 @@ if (BUILD_BENCHMARKS)
add_subdirectory(benchmarks)
endif ()

add_subdirectory(auxiliary)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need the auxiliary subdir, could crash-handler just not live in the repo root dir?

Copy link
Contributor Author

@Nerixyz Nerixyz Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I chose a subdirectory is that I expect other tools to live here in the future and for those to not pollute the top level directory. For example, if plugins get some CLI or there's some qt plugin, this could live there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, the crashpad project should be a submodule, and it will live under tools. Neither of these things need to be actioned until this has been approved


feature_summary(WHAT ALL)
55 changes: 55 additions & 0 deletions auxiliary/.clang-format
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
Language: Cpp

AccessModifierOffset: -4
AlignEscapedNewlinesLeft: true
AllowShortFunctionsOnASingleLine: false
AllowShortIfStatementsOnASingleLine: false
AllowShortLambdasOnASingleLine: Empty
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: false
AlwaysBreakBeforeMultilineStrings: false
BasedOnStyle: Google
BraceWrapping:
AfterClass: "true"
AfterControlStatement: "true"
AfterFunction: "true"
AfterNamespace: "false"
BeforeCatch: "true"
BeforeElse: "true"
BreakBeforeBraces: Custom
BreakConstructorInitializersBeforeComma: true
ColumnLimit: 80
ConstructorInitializerAllOnOneLineOrOnePerLine: false
DerivePointerBinding: false
FixNamespaceComments: true
IndentCaseLabels: true
IndentWidth: 4
IndentWrappedFunctionNames: true
IndentPPDirectives: AfterHash
SortIncludes: CaseInsensitive
IncludeBlocks: Regroup
IncludeCategories:
# Project includes
- Regex: '^"[a-zA-Z\._-]+(/[a-zA-Z0-9\._-]+)*"$'
Priority: 1
# Third party library includes
- Regex: '<[[:alnum:].]+/[a-zA-Z0-9\._\/-]+>'
Priority: 3
# Qt includes
- Regex: '^<Q[a-zA-Z0-9\._\/-]+>$'
Priority: 3
CaseSensitive: true
# LibCommuni includes
- Regex: "^<Irc[a-zA-Z]+>$"
Priority: 3
# Misc libraries
- Regex: '^<[a-zA-Z_0-9]+\.h(pp)?>$'
Priority: 3
# Standard library includes
- Regex: "^<[a-zA-Z_]+>$"
Priority: 4
NamespaceIndentation: Inner
PointerBindsToType: false
SpacesBeforeTrailingComments: 2
Standard: Auto
ReflowComments: false
8 changes: 8 additions & 0 deletions auxiliary/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
if(BUILD_WITH_CRASHPAD)
if(WIN32)
# crash-handler is only used on Windows for now.
add_subdirectory(crash-handler)
else()
message(WARNING "Crashpad was enabled but the custom crash handler is Windows only!")
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
endif()
endif()
95 changes: 95 additions & 0 deletions auxiliary/crash-handler/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
project(chatterino-crash-handler
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
VERSION 0.1.0
LANGUAGES CXX
)
set(PROJECT_LIB "${PROJECT_NAME}-lib")

set(LIB_SOURCE_FILES
commandline.hpp
commandline.cpp
recovery.hpp
recovery.cpp
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
)

set(EXE_SOURCE_FILES
main.cpp
)

if(WIN32)
list(APPEND LIB_SOURCE_FILES
win_support.hpp
win_support.cpp
)
endif()

add_library(${PROJECT_LIB} OBJECT ${LIB_SOURCE_FILES})
target_link_libraries(${PROJECT_LIB}
PUBLIC
crashpad_handler_lib
crashpad_tools
MagicEnum
)
set_target_properties(${PROJECT_LIB}
PROPERTIES
CXX_STANDARD 23
CXX_STANDARD_REQUIRED On
)

add_executable(${PROJECT_NAME} WIN32 ${EXE_SOURCE_FILES})
target_link_libraries(${PROJECT_NAME} PRIVATE ${PROJECT_LIB})
set_target_properties(${PROJECT_NAME}
PROPERTIES
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin/crashpad"
OUTPUT_NAME "crashpad_handler"
CXX_STANDARD 23
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
CXX_STANDARD_REQUIRED On
)

if(WIN32)
target_compile_definitions(${PROJECT_LIB} PUBLIC NOMINMAX WIN32_LEAN_AND_MEAN)
endif()

# Configure compilers
if(MSVC)
target_compile_options(${PROJECT_LIB} PUBLIC /EHsc /bigobj /utf-8)
target_compile_options(${PROJECT_LIB} PUBLIC
/W4
# 5038 - warnings about initialization order
/w15038
)
else()
message(WARNING "No warnings configured for this compiler!")
endif()

if(CHATTERINO_ENABLE_LTO)
message(STATUS "Enabling LTO for ${PROJECT_NAME}")
set_property(TARGET ${PROJECT_NAME}
PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
endif()

# TESTS

if(TARGET gtest)
message(STATUS "Tests enabled for ${PROJECT_NAME}")

set(PROJECT_TESTS "${PROJECT_NAME}-test")
set(TEST_FILES
commandline_test.cpp
recovery_test.cpp
)
if(WIN32)
list(APPEND TEST_FILES
win_support_test.cpp
)
endif()

add_executable(${PROJECT_TESTS} ${TEST_FILES})
target_link_libraries(${PROJECT_TESTS} PRIVATE ${PROJECT_LIB} GTest::gtest_main gmock)
set_target_properties(${PROJECT_TESTS}
PROPERTIES
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
CXX_STANDARD 23
CXX_STANDARD_REQUIRED On
)
gtest_discover_tests(${PROJECT_TESTS})
endif()
45 changes: 45 additions & 0 deletions auxiliary/crash-handler/commandline.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "commandline.hpp"

std::vector<std::wstring> splitChatterinoArgs(const std::wstring &args)
{
std::vector<std::wstring> parts;

std::wstring_view view(args);
std::wstring_view::size_type pos{};
std::wstring part;

while ((pos = view.find(L'+')) != std::wstring_view::npos)
{
if (pos + 1 == view.length()) // string ends with +
{
parts.emplace_back(std::move(part));
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
return parts;
}

auto next = view[pos + 1];
if (next == L'+') // escaped plus (++)
{
part += view.substr(0, pos);
part.push_back(L'+');
view = view.substr(pos + 2);
continue;
}

// actual separator
part += view.substr(0, pos);
parts.emplace_back(std::move(part));
part = {};
view = view.substr(pos + 1);
}

if (!view.empty())
{
part += view;
}
if (!part.empty())
{
parts.emplace_back(std::move(part));
}

return parts;
}
10 changes: 10 additions & 0 deletions auxiliary/crash-handler/commandline.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#pragma once

#include <string>
#include <vector>

/// Parse a command line string from chatterino into its arguments.
///
/// The command line arguments are joined by '+'. A plus is escaped by an
/// additional plus ('++' -> '+').
std::vector<std::wstring> splitChatterinoArgs(const std::wstring &args);
41 changes: 41 additions & 0 deletions auxiliary/crash-handler/commandline_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#include "commandline.hpp"
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved

#include <gtest/gtest.h>

using namespace std::string_literals;

TEST(CommandLineTest, splitChatterinoArgs)
{
struct TestCase {
std::wstring input;
std::vector<std::wstring> output;
};

std::initializer_list<TestCase> testCases{
{
L"-c+t:alien+--safe-mode"s,
{L"-c"s, L"t:alien"s, L"--safe-mode"s},
},
{
L"-c+t:++++++breaking news++++++!!+-V"s,
{L"-c"s, L"t:+++breaking news+++!!"s, L"-V"s},
},
{
L"++"s,
{L"+"s},
},
{
L""s,
{},
},
{
L"--channels=t:foo;t:bar;t:++++++foo++++++"s,
{L"--channels=t:foo;t:bar;t:+++foo+++"},
},
};

for (const auto &testCase : testCases)
{
EXPECT_EQ(splitChatterinoArgs(testCase.input), testCase.output);
}
}
54 changes: 54 additions & 0 deletions auxiliary/crash-handler/main.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#include "recovery.hpp"

#include <handler/handler_main.h>
#include <tools/tool_support.h>

#if BUILDFLAG(IS_WIN)
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
# include <Windows.h>
#endif

namespace {

// NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays)
int actualMain(int argc, char *argv[])
{
// We're tapping into the crash handling by registering a data source.
// Our source doesn't actually provide any data, but it records the crash.
// Once the crash is recorded, we know that one happened and can attempt to restart
// the host application.
crashpad::UserStreamDataSources sources;
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
sources.emplace_back(std::make_unique<CrashRecoverer>());

auto ret = crashpad::HandlerMain(argc, argv, &sources);

if (ret == 0)
{
auto *recoverer = dynamic_cast<CrashRecoverer *>(sources.front().get());
if (recoverer != nullptr)
{
recoverer->attemptRecovery();
}
}
return ret;
}

} // namespace

// The following is adapted from handler/main.cc
#if BUILDFLAG(IS_POSIX)

int main(int argc, char *argv[])
{
return actualMain(argc, argv);
Nerixyz marked this conversation as resolved.
Show resolved Hide resolved
}

#elif BUILDFLAG(IS_WIN)

// The default entry point for /subsystem:windows.
int APIENTRY wWinMain(HINSTANCE /*hInstance*/, HINSTANCE /*hPrevInstance*/,
PWSTR /*pCmdLine*/, int /*nCmdShow*/)
{
return crashpad::ToolSupport::Wmain(__argc, __wargv, actualMain);
}

#endif // BUILDFLAG(IS_POSIX)
Loading
Loading