From bd47957c3adb1554b9ff9d58ef6b5ffc39534cfd Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Sat, 26 Dec 2020 10:27:57 +0100
Subject: [PATCH] Profiler: Show a GUI message box with the error when
 profiling fails

This came up in @jonathandturner's video walking through the system
and playing with things: https://www.youtube.com/watch?v=TtV86uL5oD4

At one point, he tried profiling his Terminal app, and since the
Terminal was completely idle while profiling, no samples were captured
and there was no profile to show.

Make sure we propagate any error when loading the profile, and show it
in a GUI message box instead of stderr. :^)
---
 DevTools/Profiler/Profile.cpp | 28 +++++++++++-----------------
 DevTools/Profiler/Profile.h   |  3 ++-
 DevTools/Profiler/main.cpp    | 11 ++++++-----
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/DevTools/Profiler/Profile.cpp b/DevTools/Profiler/Profile.cpp
index 7449c6697e..8de2fe9c82 100644
--- a/DevTools/Profiler/Profile.cpp
+++ b/DevTools/Profiler/Profile.cpp
@@ -263,29 +263,23 @@ void Profile::rebuild_tree()
     m_model->update();
 }
 
-OwnPtr<Profile> Profile::load_from_perfcore_file(const StringView& path)
+Result<NonnullOwnPtr<Profile>, String> Profile::load_from_perfcore_file(const StringView& path)
 {
     auto file = Core::File::construct(path);
-    if (!file->open(Core::IODevice::ReadOnly)) {
-        warnln("Unable to open {}, error: {}", path, file->error_string());
-        return nullptr;
-    }
+    if (!file->open(Core::IODevice::ReadOnly))
+        return String::formatted("Unable to open {}, error: {}", path, file->error_string());
 
     auto json = JsonValue::from_string(file->read_all());
     ASSERT(json.has_value());
-    if (!json.value().is_object()) {
-        warnln("Invalid perfcore format (not a JSON object)");
-        return nullptr;
-    }
+    if (!json.value().is_object())
+        return String { "Invalid perfcore format (not a JSON object)" };
 
     auto& object = json.value().as_object();
     auto executable_path = object.get("executable").to_string();
 
-    auto coredump = CoreDumpReader::create(String::format("/tmp/profiler_coredumps/%d", object.get("pid").as_u32()));
-    if (!coredump) {
-        warnln("Could not open coredump");
-        return nullptr;
-    }
+    auto coredump = CoreDumpReader::create(String::formatted("/tmp/profiler_coredumps/{}", object.get("pid").as_u32()));
+    if (!coredump)
+        return String { "Could not open coredump" };
 
     MappedFile kernel_elf_file("/boot/Kernel");
     OwnPtr<ELF::Image> kernel_elf;
@@ -294,11 +288,11 @@ OwnPtr<Profile> Profile::load_from_perfcore_file(const StringView& path)
 
     auto events_value = object.get("events");
     if (!events_value.is_array())
-        return nullptr;
+        return String { "Malformed profile (events is not an array)" };
 
     auto& perf_events = events_value.as_array();
     if (perf_events.is_empty())
-        return nullptr;
+        return String { "No events captured (targeted process was never on CPU)" };
 
     Vector<Event> events;
 
@@ -346,7 +340,7 @@ OwnPtr<Profile> Profile::load_from_perfcore_file(const StringView& path)
         events.append(move(event));
     }
 
-    return NonnullOwnPtr<Profile>(NonnullOwnPtr<Profile>::Adopt, *new Profile(executable_path, move(events)));
+    return adopt_own(*new Profile(executable_path, move(events)));
 }
 
 void ProfileNode::sort_children()
diff --git a/DevTools/Profiler/Profile.h b/DevTools/Profiler/Profile.h
index 88cb07339f..24740434f2 100644
--- a/DevTools/Profiler/Profile.h
+++ b/DevTools/Profiler/Profile.h
@@ -32,6 +32,7 @@
 #include <AK/JsonValue.h>
 #include <AK/NonnullRefPtrVector.h>
 #include <AK/OwnPtr.h>
+#include <AK/Result.h>
 #include <LibGUI/Forward.h>
 #include <LibGUI/ModelIndex.h>
 
@@ -128,7 +129,7 @@ private:
 
 class Profile {
 public:
-    static OwnPtr<Profile> load_from_perfcore_file(const StringView& path);
+    static Result<NonnullOwnPtr<Profile>, String> load_from_perfcore_file(const StringView& path);
     ~Profile();
 
     GUI::Model& model();
diff --git a/DevTools/Profiler/main.cpp b/DevTools/Profiler/main.cpp
index 90d505ab0a..2851674d44 100644
--- a/DevTools/Profiler/main.cpp
+++ b/DevTools/Profiler/main.cpp
@@ -71,13 +71,14 @@ int main(int argc, char** argv)
         path = argv[1];
     }
 
-    auto profile = Profile::load_from_perfcore_file(path);
-
-    if (!profile) {
-        warnln("Unable to load profile '{}'", path);
-        return 1;
+    auto profile_or_error = Profile::load_from_perfcore_file(path);
+    if (profile_or_error.is_error()) {
+        GUI::MessageBox::show(nullptr, profile_or_error.error(), "Profiler", GUI::MessageBox::Type::Error);
+        return 0;
     }
 
+    auto& profile = profile_or_error.value();
+
     auto window = GUI::Window::construct();
     window->set_title("Profiler");
     window->set_icon(app_icon.bitmap_for_size(16));