From 4711d789c95647d31e2435eec2195da4863f7e52 Mon Sep 17 00:00:00 2001 From: Samuel Bowman Date: Thu, 30 Dec 2021 01:17:33 -0500 Subject: [PATCH] CrashReporter: Move progressbar into main window Previously we would create a temporary progress window to show a progressbar while the coredump is processed. Since we're only waiting on backtraces and CPU register states, we can move the progressbar into the main window and show everything else immediately while the slow parts are generated in a BackgroundAction. --- .../CrashReporter/CrashReporterWindow.gml | 6 + Userland/Applications/CrashReporter/main.cpp | 184 ++++++++---------- 2 files changed, 87 insertions(+), 103 deletions(-) diff --git a/Userland/Applications/CrashReporter/CrashReporterWindow.gml b/Userland/Applications/CrashReporter/CrashReporterWindow.gml index 115367bc3cd..529c390bdaf 100644 --- a/Userland/Applications/CrashReporter/CrashReporterWindow.gml +++ b/Userland/Applications/CrashReporter/CrashReporterWindow.gml @@ -73,8 +73,14 @@ } } + @GUI::Progressbar { + name: "progressbar" + text: "Generating crash report: " + } + @GUI::TabWidget { name: "tab_widget" + visible: false } @GUI::Widget { diff --git a/Userland/Applications/CrashReporter/main.cpp b/Userland/Applications/CrashReporter/main.cpp index a2dcc7dfbe8..89d73a87bcb 100644 --- a/Userland/Applications/CrashReporter/main.cpp +++ b/Userland/Applications/CrashReporter/main.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -42,52 +43,15 @@ struct TitleAndText { String text; }; -static NonnullRefPtr create_progress_window() +struct ThreadBacktracesAndCpuRegisters { + Vector thread_backtraces; + Vector thread_cpu_registers; +}; + +static TitleAndText build_backtrace(Coredump::Reader const& coredump, ELF::Core::ThreadInfo const& thread_info, size_t thread_index, Function on_progress) { - auto window = GUI::Window::construct(); - - window->set_title("CrashReporter"); - window->set_resizable(false); - window->resize(240, 64); - window->center_on_screen(); - - auto& main_widget = window->set_main_widget(); - main_widget.set_fill_with_background_color(true); - main_widget.set_layout(); - - auto& label = main_widget.add("Generating crash report..."); - label.set_fixed_height(30); - - auto& progressbar = main_widget.add(); - progressbar.set_name("progressbar"); - progressbar.set_fixed_width(150); - progressbar.set_fixed_height(22); - - window->on_close = [&]() { - if (progressbar.value() != progressbar.max()) - exit(0); - }; - - return window; -} - -static TitleAndText build_backtrace(Coredump::Reader const& coredump, ELF::Core::ThreadInfo const& thread_info, size_t thread_index) -{ - // Show a very simple progress window ASAP to make crashing feel more responsive. - // FIXME: This is not the most beautifully factored thing. - auto progress_window = create_progress_window(); - progress_window->show(); - - auto& progressbar = *progress_window->main_widget()->find_descendant_of_type_named("progressbar"); - auto timer = Core::ElapsedTimer::start_new(); - Coredump::Backtrace backtrace(coredump, thread_info, [&](size_t frame_index, size_t frame_count) { - progress_window->set_progress(100.0f * (float)(frame_index + 1) / (float)frame_count); - progressbar.set_value(frame_index + 1); - progressbar.set_max(frame_count); - Core::EventLoop::current().pump(Core::EventLoop::WaitMode::PollForEvents); - }); - progress_window->close(); + Coredump::Backtrace backtrace(coredump, thread_info, move(on_progress)); auto metadata = coredump.metadata(); @@ -168,7 +132,7 @@ static void unlink_coredump(StringView const& coredump_path) ErrorOr serenity_main(Main::Arguments arguments) { - TRY(Core::System::pledge("stdio recvfd sendfd cpath rpath unix proc exec")); + TRY(Core::System::pledge("stdio recvfd sendfd cpath rpath unix proc exec thread")); auto app = TRY(GUI::Application::try_create(arguments)); @@ -181,51 +145,36 @@ ErrorOr serenity_main(Main::Arguments arguments) args_parser.add_option(unlink_on_exit, "Delete the coredump after its parsed", "unlink", 0); args_parser.parse(arguments); - Vector thread_backtraces; - Vector thread_cpu_registers; - - String executable_path; - Vector crashed_process_arguments; - Vector environment; - Vector memory_regions; - int pid { 0 }; - u8 termination_signal { 0 }; - - { - auto coredump = Coredump::Reader::create(coredump_path); - if (!coredump) { - warnln("Could not open coredump '{}'", coredump_path); - return 1; - } - - size_t thread_index = 0; - coredump->for_each_thread_info([&](auto& thread_info) { - thread_backtraces.append(build_backtrace(*coredump, thread_info, thread_index)); - thread_cpu_registers.append(build_cpu_registers(thread_info, thread_index)); - ++thread_index; - return IterationDecision::Continue; - }); - - coredump->for_each_memory_region_info([&](auto& memory_region_info) { - memory_regions.append(String::formatted("{:p} - {:p}: {}", memory_region_info.region_start, memory_region_info.region_end, (const char*)memory_region_info.region_name)); - return IterationDecision::Continue; - }); - - executable_path = coredump->process_executable_path(); - crashed_process_arguments = coredump->process_arguments(); - environment = coredump->process_environment(); - pid = coredump->process_pid(); - termination_signal = coredump->process_termination_signal(); + auto coredump = Coredump::Reader::create(coredump_path); + if (!coredump) { + warnln("Could not open coredump '{}'", coredump_path); + return 1; } - TRY(Core::System::unveil(executable_path.characters(), "r")); - TRY(Core::System::unveil("/res", "r")); - TRY(Core::System::unveil("/tmp/portal/launch", "rw")); + Vector memory_regions; + coredump->for_each_memory_region_info([&](auto& memory_region_info) { + memory_regions.append(String::formatted("{:p} - {:p}: {}", memory_region_info.region_start, memory_region_info.region_end, (const char*)memory_region_info.region_name)); + return IterationDecision::Continue; + }); + + auto executable_path = coredump->process_executable_path(); + auto crashed_process_arguments = coredump->process_arguments(); + auto environment = coredump->process_environment(); + auto pid = coredump->process_pid(); + auto termination_signal = coredump->process_termination_signal(); if (unlink_on_exit) TRY(Core::System::unveil(coredump_path, "c")); - + TRY(Core::System::unveil(executable_path.characters(), "r")); TRY(Core::System::unveil("/bin/HackStudio", "rx")); + TRY(Core::System::unveil("/res", "r")); + TRY(Core::System::unveil("/tmp/portal/launch", "rw")); + TRY(Core::System::unveil("/usr/lib", "r")); + coredump->for_each_library([](auto library_info) { + // FIXME: Make for_each_library propagate ErrorOr values so we can use TRY. + if (library_info.path.starts_with('/')) + MUST(Core::System::unveil(library_info.path, "r")); + }); TRY(Core::System::unveil(nullptr, nullptr)); auto app_icon = GUI::Icon::default_icon("app-crash-reporter"); @@ -233,7 +182,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto window = TRY(GUI::Window::try_create()); window->set_title("Crash Reporter"); window->set_icon(app_icon.bitmap_for_size(16)); - window->resize(460, 340); + window->resize(460, 190); window->center_on_screen(); window->on_close = [unlink_on_exit, &coredump_path]() { if (unlink_on_exit) @@ -271,6 +220,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto& arguments_label = *widget->find_descendant_of_type_named("arguments_label"); arguments_label.set_text(String::join(" ", crashed_process_arguments)); + auto& progressbar = *widget->find_descendant_of_type_named("progressbar"); auto& tab_widget = *widget->find_descendant_of_type_named("tab_widget"); auto backtrace_tab = TRY(tab_widget.try_add_tab("Backtrace")); @@ -283,15 +233,6 @@ ErrorOr serenity_main(Main::Arguments arguments) auto backtrace_tab_widget = TRY(backtrace_tab->try_add()); backtrace_tab_widget->set_tab_position(GUI::TabWidget::TabPosition::Bottom); - for (auto& backtrace : thread_backtraces) { - auto container = TRY(backtrace_tab_widget->try_add_tab(backtrace.title)); - (void)TRY(container->try_set_layout()); - container->layout()->set_margins(4); - auto backtrace_text_editor = TRY(container->try_add()); - backtrace_text_editor->set_text(backtrace.text); - backtrace_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); - backtrace_text_editor->set_should_hide_unnecessary_scrollbars(true); - } auto cpu_registers_tab = TRY(tab_widget.try_add_tab("CPU Registers")); cpu_registers_tab->set_layout(); @@ -303,15 +244,6 @@ ErrorOr serenity_main(Main::Arguments arguments) auto cpu_registers_tab_widget = TRY(cpu_registers_tab->try_add()); cpu_registers_tab_widget->set_tab_position(GUI::TabWidget::TabPosition::Bottom); - for (auto& cpu_registers : thread_cpu_registers) { - auto container = TRY(cpu_registers_tab_widget->try_add_tab(cpu_registers.title)); - (void)TRY(container->try_set_layout()); - container->layout()->set_margins(4); - auto cpu_registers_text_editor = TRY(container->try_add()); - cpu_registers_text_editor->set_text(cpu_registers.text); - cpu_registers_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); - cpu_registers_text_editor->set_should_hide_unnecessary_scrollbars(true); - } auto environment_tab = TRY(tab_widget.try_add_tab("Environment")); (void)TRY(environment_tab->try_set_layout()); @@ -352,6 +284,52 @@ ErrorOr serenity_main(Main::Arguments arguments) } }; + (void)Threading::BackgroundAction::construct( + [&, coredump = move(coredump)](auto&) { + ThreadBacktracesAndCpuRegisters results; + size_t thread_index = 0; + coredump->for_each_thread_info([&](auto& thread_info) { + results.thread_backtraces.append(build_backtrace(*coredump, thread_info, thread_index, [&](size_t frame_index, size_t frame_count) { + window->set_progress(100.0f * (float)(frame_index + 1) / (float)frame_count); + progressbar.set_value(frame_index + 1); + progressbar.set_max(frame_count); + Core::EventLoop::wake(); + })); + results.thread_cpu_registers.append(build_cpu_registers(thread_info, thread_index)); + ++thread_index; + return IterationDecision::Continue; + }); + return results; + }, + [&](auto results) { + // FIXME: Make BackgroundAction propagate ErrorOr values so we can replace these MUSTs with TRYs. + + for (auto& backtrace : results.thread_backtraces) { + auto container = MUST(backtrace_tab_widget->try_add_tab(backtrace.title)); + (void)MUST(container->template try_set_layout()); + container->layout()->set_margins(4); + auto backtrace_text_editor = MUST(container->template try_add()); + backtrace_text_editor->set_text(backtrace.text); + backtrace_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); + backtrace_text_editor->set_should_hide_unnecessary_scrollbars(true); + } + + for (auto& cpu_registers : results.thread_cpu_registers) { + auto container = MUST(cpu_registers_tab_widget->try_add_tab(cpu_registers.title)); + (void)MUST(container->template try_set_layout()); + container->layout()->set_margins(4); + auto cpu_registers_text_editor = MUST(container->template try_add()); + cpu_registers_text_editor->set_text(cpu_registers.text); + cpu_registers_text_editor->set_mode(GUI::TextEditor::Mode::ReadOnly); + cpu_registers_text_editor->set_should_hide_unnecessary_scrollbars(true); + } + + progressbar.set_visible(false); + tab_widget.set_visible(true); + window->resize(window->width(), max(340, window->height())); + window->set_progress(0); + }); + window->show(); return app->exec();