Bug 61940 – [Qt] [WK2] Debug info leaks to stdout from plugins in Qt WebKit2 layout tests

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

[Chang Shu]

The current implementation of WebKit2 loads plugins from WebCore default plugin directories for Layout tests. On Qt platform, we should overwrite these default directories for WebKitTestRunner. This matches the behavior of DumpRenderTreeQt.

[Chang Shu]

Created attachment 95774 [details]
fix patch

[Kenneth Rohde Christiansen]

Comment on attachment 95774 [details]
fix patch

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

> Source/WebKit2/UIProcess/WebContext.h:106
> +    void setAdditionalPluginsDirectory(const String&, bool);

the bool value is not obvious here, add the override arguemnt

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:131
> +void _WKContextSetAdditionalPluginsDirectory(WKContextRef contextRef, WKStringRef pluginsDirectory, bool overwrite)

isnt it override and not overwrite ?

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:46
> -void PluginInfoStore::setAdditionalPluginsDirectories(const Vector<String>& directories)
> +void PluginInfoStore::setAdditionalPluginsDirectories(const Vector<String>& directories, bool overwrite)

This seems like a misuse of the method.

When you are overriding you are not adding anything additional. I would add another method and maybe make an internal method if code can be shared

[Chang Shu]

Created attachment 96093 [details]
fix patch 2

Thanks, Kenneth. Your review made a lot sense and I changed my code accordingly.

[Laszlo Gombos]

This looks good to me; CC Anders Carlsson for a second set of eyes.

[Anders Carlsson]

Comment on attachment 96093 [details]
fix patch 2

I'd like to understand why this patch is needed at all; we don't need to do anything like this on Mac and Windows.

[Chang Shu]

(In reply to comment #5)
> (From update of attachment 96093 [details])
> I'd like to understand why this patch is needed at all; we don't need to do anything like this on Mac and Windows.

Thanks, this is valid question and I did some investigation.
Both Mac and QtLinux will call NetscapePluginModule::getPluginInfo from UIProcess. But their implementations are different. On Mac, the code does not have to load the plugin but QtLinux has to. See the following code in Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:
bool NetscapePluginModule::getPluginInfo(const String& pluginPath, PluginInfoStore::Plugin& plugin)
{
    // We are loading the plugin here since it does not seem to be a standardized way to
    // get the needed informations from a UNIX plugin without loading it.

    RefPtr<PluginPackage> package = PluginPackage::createPackage(pluginPath, 0 /*lastModified*/);

The createPackage function will load the plugin and there are some debug info printouts leaking into console/output.

Comparing the behavior to DumpRenderTreeQt, it overwrites the default plugin paths and does not really load anything. This is what I was trying to do in this patch.

[Anders Carlsson]

(In reply to comment #6)
> The createPackage function will load the plugin and there are some debug info printouts leaking into console/output.
> 
> Comparing the behavior to DumpRenderTreeQt, it overwrites the default plugin paths and does not really load anything. This is what I was trying to do in this patch.

What debug info is being printed? Would it be possible to redirect standard output and standard error to /dev/null while loading the plug-ins?

[Chang Shu]

> What debug info is being printed? Would it be possible to redirect standard output and standard error to /dev/null while loading the plug-ins?

The printout from plugins look like this:
** (<unknown>:32710): DEBUG: NP_Initialize
** (<unknown>:32710): DEBUG: NP_Initialize succeeded
** (<unknown>:32710): DEBUG: NP_Shutdown

I tried to do a redirect using freopen. But this doesn't seem to be undo-able. :(

[Chang Shu]

Anders, thanks for the code on IRC. For some reason, the printout still leaked out using the code below:
int newStdout = open("/dve/null", O_WRONLY);
int savedStdout = dup(STDOUT_FILENO);
dup2(newStdout, STDOUT_FILENO);
close(newStdout);
    RefPtr<PluginPackage> package = PluginPackage::createPackage(pluginPath, 0 /*lastModified*/);
dup2(savedStdout, STDOUT_FILENO);

I remember using freopen before calling createPackage suppressed the printout.
So it seems the first part didn't work. I googled some example but couldn't find anything suspicious. any idea? thx.

[Anders Carlsson]

(In reply to comment #8)
> > What debug info is being printed? Would it be possible to redirect standard output and standard error to /dev/null while loading the plug-ins?
> 
> The printout from plugins look like this:
> ** (<unknown>:32710): DEBUG: NP_Initialize
> ** (<unknown>:32710): DEBUG: NP_Initialize succeeded
> ** (<unknown>:32710): DEBUG: NP_Shutdown
> 
> I tried to do a redirect using freopen. But this doesn't seem to be undo-able. :(

Do you know what plug-in this is?

[Anders Carlsson]

(In reply to comment #9)
> Anders, thanks for the code on IRC. For some reason, the printout still leaked out using the code below:
> int newStdout = open("/dve/null", O_WRONLY);
> int savedStdout = dup(STDOUT_FILENO);
> dup2(newStdout, STDOUT_FILENO);
> close(newStdout);
>     RefPtr<PluginPackage> package = PluginPackage::createPackage(pluginPath, 0 /*lastModified*/);
> dup2(savedStdout, STDOUT_FILENO);
> 
> I remember using freopen before calling createPackage suppressed the printout.
> So it seems the first part didn't work. I googled some example but couldn't find anything suspicious. any idea? thx.

It's possible that you need to do the same thing for standard error.

[Chang Shu]

> > int newStdout = open("/dve/null", O_WRONLY);
the problem was this typo (*dve*). :(

[Chang Shu]

Thanks to Anders. The redirecting stdout approach works now. However, I have a couple of concerns before submitting the patch.
Ideally, the redirection should only happen right before and after all plugin calls, which is inside WebCore. It should also only happen for WebKitTestRunner. So I still feel it's difficult to make it work without hacking the code. Any opinions? Thanks.

[Anders Carlsson]

(In reply to comment #13)
> Thanks to Anders. The redirecting stdout approach works now. However, I have a couple of concerns before submitting the patch.
> Ideally, the redirection should only happen right before and after all plugin calls, which is inside WebCore. It should also only happen for WebKitTestRunner. So I still feel it's difficult to make it work without hacking the code. Any opinions? Thanks.

Which plug-in calls are inside WebCore?

[Chang Shu]

> > Ideally, the redirection should only happen right before and after all plugin calls, which is inside WebCore. It should also only happen for WebKitTestRunner. So I still feel it's difficult to make it work without hacking the code. Any opinions? Thanks.
> 
> Which plug-in calls are inside WebCore?

For example, PluginPackage::load() calls NP_Initialize.

[Anders Carlsson]

(In reply to comment #15)
> > > Ideally, the redirection should only happen right before and after all plugin calls, which is inside WebCore. It should also only happen for WebKitTestRunner. So I still feel it's difficult to make it work without hacking the code. Any opinions? Thanks.
> > 
> > Which plug-in calls are inside WebCore?
> 
> For example, PluginPackage::load() calls NP_Initialize.

Does the Qt port not use NetscapePluginModule?

[Chang Shu]

> > For example, PluginPackage::load() calls NP_Initialize.
> 
> Does the Qt port not use NetscapePluginModule?

Yes, it does. But NetscapePluginModule does not call NPAPIs directly.
The call stack looks like this:
#0  WebCore::PluginPackage::load () at Source/WebCore/plugins/qt/PluginPackageQt.cpp:186
#1  WebCore::PluginPackage::fetchInfo () at Source/WebCore/plugins/qt/PluginPackageQt.cpp:40
#2  WebCore::PluginPackage::createPackage () at Source/WebCore/plugins/PluginPackage.cpp:162
#3  0xb6278a24 in WebKit::NetscapePluginModule::getPluginInfo () at Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:78

[Anders Carlsson]

(In reply to comment #17)
> > > For example, PluginPackage::load() calls NP_Initialize.
> > 
> > Does the Qt port not use NetscapePluginModule?
> 
> Yes, it does. But NetscapePluginModule does not call NPAPIs directly.
> The call stack looks like this:
> #0  WebCore::PluginPackage::load () at Source/WebCore/plugins/qt/PluginPackageQt.cpp:186
> #1  WebCore::PluginPackage::fetchInfo () at Source/WebCore/plugins/qt/PluginPackageQt.cpp:40
> #2  WebCore::PluginPackage::createPackage () at Source/WebCore/plugins/PluginPackage.cpp:162
> #3  0xb6278a24 in WebKit::NetscapePluginModule::getPluginInfo () at Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:78

I think NetscapePluginModuleX11 shouldn't use PluginPackage. In any case, it should be possible to redirect stdout/stderr around the PluginPackage calls. Could even be done with an RAII object.

[Chang Shu]

> I think NetscapePluginModuleX11 shouldn't use PluginPackage. In any case, it should be possible to redirect stdout/stderr around the PluginPackage calls. Could even be done with an RAII object.

I understand putting the code around PluginPackage calls will work. My concern is if people add traces inside PluginPackage::createPackage, for example, they won't see them. The real problem is that we could not specify which process should do the redirect but the stdout as a whole. This may accidentally supress useful printouts.

[Chang Shu]

Created attachment 97613 [details]
fix patch 3

[Anders Carlsson]

Comment on attachment 97613 [details]
fix patch 3

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

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:43
> +#if OS(UNIX)
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#endif

I don't think OS(UNIX) is needed here; using X11 pretty much implies Unix.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:51
> +class StdoutRedirect {

Maybe a noun would be better here. StdoutRedirector?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:53
> +    StdoutRedirect(const char* redirectPath);

I don't think we need to support redirecting to an arbitrary path. In fact, maybe the class name could be StdoutDevNullRedirector?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:63
> +#if OS(UNIX)

Again, I don't think OS(UNIX) is needed here.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:86
> +#if OS(UNIX)
> +#define SUPPRESS_STDOUT_LOCALLY StdoutRedirect stdoutRedirect("/dev/null")
> +#else
> +#define SUPPRESS_STDOUT_LOCALLY
> +#endif

There's no real reason to have a macro here. The class is already a no-op on Non-Unix platforms (which I again don't think we need to have an #ifdef check for)

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:114
> +    SUPPRESS_STDOUT_LOCALLY;

Again, just declaring the object instead of using a macro here is more clear. Also, I'd like to see a comment explaining why this is here.

[Chang Shu]

Created attachment 97616 [details]
fix patch 4

[WebKit Review Bot]

Comment on attachment 97616 [details]
fix patch 4

Clearing flags on attachment: 97616

Committed r89150: <http://trac.webkit.org/changeset/89150>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

*** Bug 62911 has been marked as a duplicate of this bug. ***