Bug 140036 – Simplify WebKitTestRunner timeout tracking

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

WebKitTestRunner has "short timeout" and "long timeout" for IPC, plus it has waitUntilDone timeout, plus run-webkit-tests itself has a timeout.

There are two reasons why WebKitTestRunner IPC timeouts are helpful:

1. If WebProcess freezes between tests, we want to detect that, and not attribute the freeze to the next test. Expecting a "start test" message to be handled quickly achieves this.

2. If WebProcess is so broken that it fails to launch, it's nice to say so, and not just fail with a generic timeout.

Both are handled by short timeout. Long timeout is only used when loading about:blank between tests, which I don't quite see the logic behind.

[Alexey Proskuryakov]

Created attachment 243875 [details]
proposed patch

[Alexey Proskuryakov]

Created attachment 243882 [details]
proposed patch

With Efl and Gtk build fixes.

[Csaba Osztrogonác]

(In reply to comment #2)
> Created attachment 243882 [details]
> proposed patch
> 
> With Efl and Gtk build fixes.

./../Tools/WebKitTestRunner/TestController.cpp:73:45: error: missing binary operator before token "("
 #if defined (__has_feature) && __has_feature(address_sanitizer)
                                             ^

Unfortunately GCC doesn't support short circuit evaluation,
so we need to use nested ifs for has_feature and has_include.

[Csaba Osztrogonác]

It's strange how GTK built it. I thought GTK uses GCC 4.8 too.

[Alexey Proskuryakov]

Created attachment 243895 [details]
proposed patch

OK, replaced with an ugly nested #if for now. I'm looking into adding a macro to WTF, but need to figure out how an existing ADDRESS_SANITIZER macro is used.

[Darin Adler]

Comment on attachment 243895 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243895&action=review

> Tools/WebKitTestRunner/TestController.cpp:78
> +#else

Missing #endif before this #else.

[Alexey Proskuryakov]

Created attachment 243901 [details]
proposed patch

[Darin Adler]

Comment on attachment 243901 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243901&action=review

I think it’s a little strange to compile in this 10 second vs. 5 second default timeout.

> Tools/WebKitTestRunner/TestController.cpp:81
> +#if defined(__has_feature)
> +#if __has_feature(address_sanitizer)
> +const double TestController::shortTimeout = 10.0;
> +#else
> +const double TestController::shortTimeout = 5.0;
> +#endif
> +#else
> +const double TestController::shortTimeout = 5.0;
> +#endif

I would have written:

    #if defined(__has_feature) && __has_feature(address_sanitizer)
    const double TestController::shortTimeout = 10;
    #else
    const double TestController::shortTimeout = 5;
    #endif

But maybe the nested syntax is needed to compile on compilers that don't have __has_feature?

> Tools/WebKitTestRunner/TestController.h:56
> +    constexpr static double noTimeout = -1;

Does this work on Windows? I had problems with constexpr on Windows.

[Darin Adler]

Comment on attachment 243901 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243901&action=review

>> Tools/WebKitTestRunner/TestController.cpp:81
>> +#endif
> 
> I would have written:
> 
>     #if defined(__has_feature) && __has_feature(address_sanitizer)
>     const double TestController::shortTimeout = 10;
>     #else
>     const double TestController::shortTimeout = 5;
>     #endif
> 
> But maybe the nested syntax is needed to compile on compilers that don't have __has_feature?

Oops, I see you discussing this earlier in this bug!

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r177870>. Removed the use of constexpr to be safe, as Windows EWS is not functional.