Bug 87672 – [Qt] Add quirks for running the web process in a profiler shell, like valgrind

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

The only reliable way to run the web process in valgrind is to change ProcessLauncherQt.cpp.
I think it's worth to add an env var quirk for this.

[Balazs Kelemen]

Created attachment 144381 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 144381 [details]
Patch

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

Cool! :-)  I have some comments. I wish these debugging features / environment variables were more discoverable, maybe we could add a --help to our main.cpp to describe them?

> Source/WebKit2/ChangeLog:9
> +        Consider the enironment variables QT_WEB_PROCESS_COMMAND_PREFIX
> +        and QT_PLUGIN_PROCESS_COMMAND_PREFIX. If set use their values as

Typo "environment". I think it would be clearer to just say: "If environment variables QT_WEB_... and QT_PLUGIN_... are set, use their values...".

Also other environment variables use QT_WEBKIT_ prefix, maybe we should follow the same here.

> Tools/ChangeLog:8
> +        Add a dedicated environment variable quirk (QT_WTR_NO_TIMEOUT)

Why existing --no-timeout option is not enough?

[Balazs Kelemen]

Created attachment 144525 [details]
Patch

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 144381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144381&action=review
> 
> Cool! :-)  I have some comments. I wish these debugging features / environment variables were more discoverable, maybe we could add a --help to our main.cpp to describe them?

I don't think it belongs to either MiniBrowser or WebKitTestRunner. I will update the wiki with these ;)

> 
> > Source/WebKit2/ChangeLog:9
> > +        Consider the enironment variables QT_WEB_PROCESS_COMMAND_PREFIX
> > +        and QT_PLUGIN_PROCESS_COMMAND_PREFIX. If set use their values as
> 
> Typo "environment". I think it would be clearer to just say: "If environment variables QT_WEB_... and QT_PLUGIN_... are set, use their values...".
> 
> Also other environment variables use QT_WEBKIT_ prefix, maybe we should follow the same here.

Fixed both.

> 
> > Tools/ChangeLog:8
> > +        Add a dedicated environment variable quirk (QT_WTR_NO_TIMEOUT)
> 
> Why existing --no-timeout option is not enough?

Good point, I removed that.

[Balazs Kelemen]

> 
> > 
> > > Tools/ChangeLog:8
> > > +        Add a dedicated environment variable quirk (QT_WTR_NO_TIMEOUT)
> > 
> > Why existing --no-timeout option is not enough?
> 
> Good point, I removed that.

Hm, actually it is not enough, because it only disables the watchdog timer, not the runloop timer. But it's another bug.

[Simon Hausmann]

Comment on attachment 144525 [details]
Patch

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

I think this is a very good idea (r+). But please fix the string handling before landing. Just call qgetenv() every time and remember to use constData() if you can.

In this case I would even argue that the use of arg() is overkill, this is merely about concatenating a bunch of strings, so perhaps it would make sense to use the string builder in Qt with the % operator:

http://doc-snapshot.qt-project.org/5.0/qstring.html#more-efficient-string-construction

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:105
> +    static QByteArray webProcessPrefix = qgetenv("QT_WEBKIT_WEB_PROCESS_COMMAND_PREFIX");
> +    static QByteArray pluginProcessPrefix = qgetenv("QT_WEBKIT_PLUGIN_PROCESS_COMMAND_PREFIX");

I wouldn't simply bother with making those global variables. The strings are already global in the environment and we don't launch processes often enough to IMHO justify the time we "save" by not extracting the strings again out of the environment. At the same time the _copy_ of the string uses memory for no reason :)

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:118
> +    QString commandLine = prefix.isEmpty() ? QStringLiteral("%1 %2") : QString(QStringLiteral("%1 %2 %3")).arg(QLatin1String(prefix.data()));

By calling data() instead of constData() you'll end up creating a copy of the QByteArray for no reason. The non-const data() calls detach() and will detach because the refcount will be 2 (the original prefix variable holds one ref, the local prefix variable another one).

[Balazs Kelemen]

(In reply to comment #5)
> > 
> > > 
> > > > Tools/ChangeLog:8
> > > > +        Add a dedicated environment variable quirk (QT_WTR_NO_TIMEOUT)
> > > 
> > > Why existing --no-timeout option is not enough?
> > 
> > Good point, I removed that.
> 
> Hm, actually it is not enough, because it only disables the watchdog timer, not the runloop timer. But it's another bug.

No, I misread it. It should be enough.

[Balazs Kelemen]

(In reply to comment #6)
> (From update of attachment 144525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144525&action=review
> 
> I think this is a very good idea (r+). But please fix the string handling before landing. Just call qgetenv() every time and remember to use constData() if you can.
> 
> In this case I would even argue that the use of arg() is overkill, this is merely about concatenating a bunch of strings, so perhaps it would make sense to use the string builder in Qt with the % operator:

Thanks for the detailed review. It's enough for another round, so I'm going to upload a new patch.

[Balazs Kelemen]

Created attachment 145449 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 145449 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:110
> +        commandLine = commandLine % static_cast<QString>(executablePathOfWebProcess());

static_cast?

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:115
> +        commandLine = commandLine % static_cast<QString>(executablePathOfPluginProcess());

??

[Balazs Kelemen]

Created attachment 146057 [details]
Patch

[Balazs Kelemen]

(In reply to comment #7)
> (In reply to comment #5)
> > > 
> > > > 
> > > > > Tools/ChangeLog:8
> > > > > +        Add a dedicated environment variable quirk (QT_WTR_NO_TIMEOUT)
> > > > 
> > > > Why existing --no-timeout option is not enough?
> > > 
> > > Good point, I removed that.
> > 
> > Hm, actually it is not enough, because it only disables the watchdog timer, not the runloop timer. But it's another bug.
> 
> No, I misread it. It should be enough.

D'oh, misread again. It's enough with QT_WEBKIT2_DEBUG but we don't want this with valgrind. I'm going to add a switch for the responsiveness timer.

[Balazs Kelemen]

Created attachment 147087 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 147087 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:569
> +    if (!m_useRensponsivityTimer) {
> +        platformRunUntil(done, m_noTimeout);
> +        return;
> +    }
> +

Why don't we just call runUntil() with NoTimeout ?

[Balazs Kelemen]

Created attachment 147315 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 147315 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        and/or QT_WEBKIT_PLUGIN_PROCESS_COMMAND_PREFIX set, use their

*are* set,

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:107
> +        QByteArray webProcessPrefix = qgetenv("QT_WEBKIT_WEB_PROCESS_COMMAND_PREFIX");

1. This is pretty long
2. It would be nice if this name would align with QT_WEBKIT2_DEBUG since their use cases are related.
So maybe we could name them QT_WEBKIT2_WP_CMD_PREFIX and QT_WEBKIT2_PP_CMD_PREFIX?

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:110
> +        if (!webProcessPrefix.isEmpty())
> +            commandLine = QLatin1String(webProcessPrefix.constData()) % QLatin1Char(' ');
> +        commandLine = commandLine % QString(executablePathOfWebProcess());

This can be simplified to:
commandLine = QLatin1String(webProcessPrefix.constData()) % QLatin1Char(' ') % QString(executablePathOfWebProcess());

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:153
> -    commandLine = commandLine.arg(sockets[0]);
> +    commandLine = commandLine % QLatin1Char(' ') % QString::number(sockets[0]);

You can leave that one as it is, arg() is perfect for this and reads better if you have to convert to numbers anyway.

> Tools/ChangeLog:11
> +        profiling purposes because it only effects the timeout

it only *affects*

> Tools/WebKitTestRunner/TestController.cpp:264
> +        if (argument == "--no-runloop-timeout") {

I'm wondering if it wouldn't be better to just also avoid the timer in TestControllerQt if m_useWaitToDumpWatchdogTimer is false. Please try it and if it works I would avoid adding an extra command line argument which might cause confusion. A lot of people that tried to run this into valgrind probably tried --no-timout because it sounds right from that point of view.

[Jocelyn Turcotte]

Comment on attachment 147315 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:264
>> +        if (argument == "--no-runloop-timeout") {
> 
> I'm wondering if it wouldn't be better to just also avoid the timer in TestControllerQt if m_useWaitToDumpWatchdogTimer is false. Please try it and if it works I would avoid adding an extra command line argument which might cause confusion. A lot of people that tried to run this into valgrind probably tried --no-timout because it sounds right from that point of view.

Ok talked on IRC and that would probably break performance tests, so let's add a switch and maybe rename it to something clearer to those not wanting to read the code.

> Tools/WebKitTestRunner/TestController.cpp:571
> +    if (!m_useRunloopTimer) {
> +        platformRunUntil(done, m_noTimeout);
> +        return;
> +    }
> +
>      double timeout;
>      switch (timeoutDuration) {

Try to avoid the duplicate call to platformRunUntil.
Something like:

double timeout = m_noTimeout;
if (m_useRunLoopTimer) {
    switch (timeoutDuration) {
    ....

> Tools/WebKitTestRunner/TestController.h:132
> +    bool m_useRunloopTimer;

Maybe rename this to m_forceNoTimeout.

[Balazs Kelemen]

Created attachment 151715 [details]
Patch

[Balazs Kelemen]

(In reply to comment #16)
> (From update of attachment 147315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147315&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        and/or QT_WEBKIT_PLUGIN_PROCESS_COMMAND_PREFIX set, use their
> 
> *are* set,
> 
> > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:107
> > +        QByteArray webProcessPrefix = qgetenv("QT_WEBKIT_WEB_PROCESS_COMMAND_PREFIX");
> 
> 1. This is pretty long
> 2. It would be nice if this name would align with QT_WEBKIT2_DEBUG since their use cases are related.
> So maybe we could name them QT_WEBKIT2_WP_CMD_PREFIX and QT_WEBKIT2_PP_CMD_PREFIX?

Done, done.

> 
> > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:110
> > +        if (!webProcessPrefix.isEmpty())
> > +            commandLine = QLatin1String(webProcessPrefix.constData()) % QLatin1Char(' ');
> > +        commandLine = commandLine % QString(executablePathOfWebProcess());
> 
> This can be simplified to:
> commandLine = QLatin1String(webProcessPrefix.constData()) % QLatin1Char(' ') % QString(executablePathOfWebProcess());
> 
> > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:153
> > -    commandLine = commandLine.arg(sockets[0]);
> > +    commandLine = commandLine % QLatin1Char(' ') % QString::number(sockets[0]);
> 
> You can leave that one as it is, arg() is perfect for this and reads better if you have to convert to numbers anyway.

Simplified and changed back to use QString::arg as discussed on irc since it is more readable and it only runs once so we should not bother optmising it.

[Balazs Kelemen]

(In reply to comment #17)
> (From update of attachment 147315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147315&action=review
> 
> >> Tools/WebKitTestRunner/TestController.cpp:264
> >> +        if (argument == "--no-runloop-timeout") {
> > 
> > I'm wondering if it wouldn't be better to just also avoid the timer in TestControllerQt if m_useWaitToDumpWatchdogTimer is false. Please try it and if it works I would avoid adding an extra command line argument which might cause confusion. A lot of people that tried to run this into valgrind probably tried --no-timout because it sounds right from that point of view.
> 
> Ok talked on IRC and that would probably break performance tests, so let's add a switch and maybe rename it to something clearer to those not wanting to read the code.
> 
> > Tools/WebKitTestRunner/TestController.cpp:571
> > +    if (!m_useRunloopTimer) {
> > +        platformRunUntil(done, m_noTimeout);
> > +        return;
> > +    }
> > +
> >      double timeout;
> >      switch (timeoutDuration) {
> 
> Try to avoid the duplicate call to platformRunUntil.
> Something like:
> 
> double timeout = m_noTimeout;
> if (m_useRunLoopTimer) {
>     switch (timeoutDuration) {
>     ....
> 
> > Tools/WebKitTestRunner/TestController.h:132
> > +    bool m_useRunloopTimer;
> 
> Maybe rename this to m_forceNoTimeout.

Done, done.

[Jocelyn Turcotte]

Comment on attachment 151715 [details]
Patch

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

> Tools/ChangeLog:12
> +        profiling purposes because it only affects the timeout
> +        for running the actual test and we should keep this

"for running the actual test" could read something like "when waiting for the end of a test" to be clearer.

> Tools/WebKitTestRunner/TestController.cpp:264
> +        if (argument == "--no-timeout-for-all") {

"No timeout for all" doesn't read so well with my limited english skills. Maybe --no-timeout-at-all or --no-timeout-ever or --no-timeout-for-debugging?

> Tools/WebKitTestRunner/TestController.cpp:265
> +            m_useWaitToDumpWatchdogTimer = false;

That line won't have any effect as far as I know, since both ShortTimeout and LongTimeout will be the same as NoTimeout with your changes to runUntil.

[Balazs Kelemen]

Created attachment 152271 [details]
Patch

[Balazs Kelemen]

Created attachment 152272 [details]
Patch

synced changelog

[Balazs Kelemen]

(In reply to comment #21)
> (From update of attachment 151715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151715&action=review
> 
> > Tools/ChangeLog:12
> > +        profiling purposes because it only affects the timeout
> > +        for running the actual test and we should keep this
> 
> "for running the actual test" could read something like "when waiting for the end of a test" to be clearer.

Fixed, and expanded the explanation so hopefully it is more understandable now.

> 
> > Tools/WebKitTestRunner/TestController.cpp:264
> > +        if (argument == "--no-timeout-for-all") {
> 
> "No timeout for all" doesn't read so well with my limited english skills. Maybe --no-timeout-at-all or --no-timeout-ever or --no-timeout-for-debugging?

Fixed, I choose --no-timeout-at-all.

> 
> > Tools/WebKitTestRunner/TestController.cpp:265
> > +            m_useWaitToDumpWatchdogTimer = false;
> 
> That line won't have any effect as far as I know, since both ShortTimeout and LongTimeout will be the same as NoTimeout with your changes to runUntil.

It is also used by the injected bundle, it is passed to it in TestInvocation::invoke.

[Jocelyn Turcotte]

Comment on attachment 152272 [details]
Patch

(In reply to comment #24)
> It is also used by the injected bundle, it is passed to it in TestInvocation::invoke.

Yep you're right, ok looks good!

[Balazs Kelemen]

Comment on attachment 152272 [details]
Patch

Clearing flags on attachment: 152272

Committed r123464: <http://trac.webkit.org/changeset/123464>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.