Bug 237708

Created attachment 454340 [details]
Patch

Comment on attachment 454340 [details]
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.

Comment on attachment 454340 [details]
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

Ok.

>> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:105
>> +        }, 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.

I think this was needed before because the server was started in beforeAll() and the test runner handles the timeout per test. Now that the server is started/stopped as part of the test setup/teardown we can probably remove this and let the runner handle the timeout.

>> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:228
>> +    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.

Yes, I think the idea here was to kill the server when the test fails, not when it crashes.

Committed r291152 (248312@trunk): <https://commits.webkit.org/248312@trunk>