test-web: Do not clear the WebContent crash handler between tests

Clearing the callback opens a window for the WebContent process to crash
while we do not have a callback set. In practice, we see this with ASAN
crashes, where the crash occurs after we've already signaled that the
test has completed.

We now set the crash handler once during init. This required moving the
clearing of other callbacks to the test completion handler (we were
clearing these callbacks in several different ways anyways, so now we
will at least be consistent).

Author: trflynn89

Tests/LibWeb/test-web/TestWeb.h

@@ -14,6 +14,7 @@
 #include <AK/String.h>
 #include <AK/Time.h>
 #include <AK/Vector.h>
+#include <LibCore/Forward.h>
 #include <LibCore/Promise.h>
 #include <LibGfx/Forward.h>
 
@@ -59,6 +60,8 @@ struct Test {
 
     RefPtr<Gfx::Bitmap const> actual_screenshot {};
     RefPtr<Gfx::Bitmap const> expectation_screenshot {};
+
+    RefPtr<Core::Timer> timeout_timer {};
 };
 
 struct TestCompletion {

Tests/LibWeb/test-web/main.cpp

@@ -38,7 +38,7 @@ namespace TestWeb {
 
 static RefPtr<Core::Promise<Empty>> s_all_tests_complete;
 static Vector<ByteString> s_skipped_tests;
-static HashMap<WebView::ViewImplementation const*, Test const*> s_test_by_view;
+static HashMap<WebView::ViewImplementation const*, Test*> s_test_by_view;
 
 static constexpr StringView test_result_to_string(TestResult result)
 {
@@ -152,13 +152,6 @@ static ErrorOr<void> collect_crash_tests(Application const& app, Vector<Test>& t
     return {};
 }
 
-static void clear_test_callbacks(TestWebView& view)
-{
-    view.on_load_finish = {};
-    view.on_test_finish = {};
-    view.on_web_content_crashed = {};
-}
-
 static String generate_wait_for_test_string(StringView wait_class)
 {
     return MUST(String::formatted(R"(
@@ -195,12 +188,7 @@ static auto wait_for_reftest_completion = generate_wait_for_test_string("reftest
 
 static void run_dump_test(TestWebView& view, Test& test, URL::URL const& url, int timeout_in_milliseconds)
 {
-    auto timer = Core::Timer::create_single_shot(timeout_in_milliseconds, [&view, &test]() {
-        view.on_load_finish = {};
-        view.on_test_finish = {};
-        view.on_set_test_timeout = {};
-        view.reset_zoom();
-
+    test.timeout_timer = Core::Timer::create_single_shot(timeout_in_milliseconds, [&view, &test]() {
         view.on_test_complete({ test, TestResult::Timeout });
     });
 
@@ -259,24 +247,13 @@ static void run_dump_test(TestWebView& view, Test& test, URL::URL const& url, in
         return TestResult::Fail;
     };
 
-    auto on_test_complete = [&view, &test, timer, handle_completed_test]() {
-        view.reset_zoom();
-        clear_test_callbacks(view);
-        timer->stop();
-
+    auto on_test_complete = [&view, &test, handle_completed_test]() {
         if (auto result = handle_completed_test(); result.is_error())
             view.on_test_complete({ test, TestResult::Fail });
         else
             view.on_test_complete({ test, result.value() });
     };
 
-    view.on_web_content_crashed = [&view, &test, timer]() {
-        clear_test_callbacks(view);
-        timer->stop();
-
-        view.on_test_complete({ test, TestResult::Crashed });
-    };
-
     if (test.mode == TestMode::Layout) {
         view.on_load_finish = [&view, &test, url, on_test_complete = move(on_test_complete)](auto const& loaded_url) {
             // We don't want subframe loads to trigger the test finish.
@@ -342,25 +319,18 @@ static void run_dump_test(TestWebView& view, Test& test, URL::URL const& url, in
         };
     }
 
-    view.on_set_test_timeout = [timer, timeout_in_milliseconds](double milliseconds) {
-        if (milliseconds <= timeout_in_milliseconds)
-            return;
-        timer->stop();
-        timer->start(milliseconds);
+    view.on_set_test_timeout = [&test, timeout_in_milliseconds](double milliseconds) {
+        if (milliseconds > timeout_in_milliseconds)
+            test.timeout_timer->restart(milliseconds);
     };
 
     view.load(url);
-    timer->start();
+    test.timeout_timer->start();
 }
 
 static void run_ref_test(TestWebView& view, Test& test, URL::URL const& url, int timeout_in_milliseconds)
 {
     auto timer = Core::Timer::create_single_shot(timeout_in_milliseconds, [&view, &test]() {
-        view.on_load_finish = {};
-        view.on_test_finish = {};
-        view.on_set_test_timeout = {};
-        view.reset_zoom();
-
         view.on_test_complete({ test, TestResult::Timeout });
     });
 
@@ -394,23 +364,13 @@ static void run_ref_test(TestWebView& view, Test& test, URL::URL const& url, int
         return TestResult::Fail;
     };
 
-    auto on_test_complete = [&view, &test, timer, handle_completed_test]() {
-        clear_test_callbacks(view);
-        timer->stop();
-
+    auto on_test_complete = [&view, &test, handle_completed_test]() {
         if (auto result = handle_completed_test(); result.is_error())
             view.on_test_complete({ test, TestResult::Fail });
         else
             view.on_test_complete({ test, result.value() });
     };
 
-    view.on_web_content_crashed = [&view, &test, timer]() {
-        clear_test_callbacks(view);
-        timer->stop();
-
-        view.on_test_complete({ test, TestResult::Crashed });
-    };
-
     view.on_load_finish = [&view](auto const&) {
         view.run_javascript(wait_for_reftest_completion);
     };
@@ -483,11 +443,9 @@ static void run_ref_test(TestWebView& view, Test& test, URL::URL const& url, int
         view.load(URL::Parser::basic_parse(reference_to_load).release_value());
     };
 
-    view.on_set_test_timeout = [timer, timeout_in_milliseconds](double milliseconds) {
-        if (milliseconds <= timeout_in_milliseconds)
-            return;
-        timer->stop();
-        timer->start(milliseconds);
+    view.on_set_test_timeout = [&test, timeout_in_milliseconds](double milliseconds) {
+        if (milliseconds > timeout_in_milliseconds)
+            test.timeout_timer->restart(milliseconds);
     };
 
     view.load(url);
@@ -580,6 +538,11 @@ static void set_ui_callbacks_for_tests(TestWebView& view)
         // For tests, just close the alert right away to unblock JS execution.
         view.alert_closed();
     };
+
+    view.on_web_content_crashed = [&view]() {
+        if (auto test = s_test_by_view.get(&view); test.has_value())
+            view.on_test_complete({ *test.value(), TestResult::Crashed });
+    };
 }
 
 static ErrorOr<int> run_tests(Core::AnonymousBuffer const& theme, Web::DevicePixelSize window_size)
@@ -699,6 +662,17 @@ static ErrorOr<int> run_tests(Core::AnonymousBuffer const& theme, Web::DevicePix
         };
 
         view->test_promise().when_resolved([&, run_next_test, view_id](auto result) {
+            view->on_load_finish = {};
+            view->on_test_finish = {};
+            view->on_reference_test_metadata = {};
+            view->on_set_test_timeout = {};
+            view->reset_zoom();
+
+            if (result.test.timeout_timer) {
+                result.test.timeout_timer->stop();
+                result.test.timeout_timer.clear();
+            }
+
             result.test.end_time = UnixDateTime::now();
             s_test_by_view.remove(view);