[webkit-reviews] review granted: [Bug 237708] [GTK] Add a unit test to check the remote inspector HTTP server : [Attachment 454340] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 10 06:10:31 PST 2022
Michael Catanzaro <mcatanzaro at gnome.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 237708: [GTK] Add a unit test to check the remote inspector HTTP server
https://bugs.webkit.org/show_bug.cgi?id=237708
Attachment 454340: Patch
https://bugs.webkit.org/attachment.cgi?id=454340&action=review
--- Comment #2 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 454340
--> https://bugs.webkit.org/attachment.cgi?id=454340
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=454340&action=review
> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:67
> + g_printerr("Failed to connect to inspector server");
This one actually does need to end in \n
> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:105
> + // Wait until server is ready.
> + unsigned timeoutID = g_timeout_add(25000, [](gpointer userData) {
> + g_main_loop_quit(static_cast<GMainLoop*>(userData));
> + return G_SOURCE_REMOVE;
> + }, m_mainLoop);
Hmm, what is the purpose of this? You are trying to fail the test if it takes
more than 2.5 seconds for the server to start? I would either use a way larger
timeout -- say 30 seconds, to match the standard D-Bus timeout -- or else
remove it altogether and let the test time out if it's broken. Otherwise, I
would be afraid the test could be flaky if the system is under heavy load.
I see you've already taken care to retry the connection every 100ms and quit
immediately if it succeeds, which is good.
> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:228
> + // We install a handler to ensure that we kill the child process
> + // if the parent dies because of whatever the reason is.
> + signal(SIGABRT, +[](int) {
Since you're only catching SIGABRT, I would say "if the parent dies because of
an assertion failure."
Alternative: you might try prctl(PR_SET_PDEATHSIG) and see if that works, then
the child should die even if the parent dies to something other than SIGABRT.
But I've seen PR_SET_PDEATHSIG mysteriously fail, so maybe make sure it works
for you if you decide to try this.
More information about the webkit-reviews
mailing list