Bug 61940

Summary: [Qt] [WK2] Debug info leaks to stdout from plugins in Qt WebKit2 layout tests
Product: WebKit Reporter: Chang Shu <cshu>
Component: Tools / TestsAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, laszlo.gombos, venkatpenukonda, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
fix patch
kenneth: review-
fix patch 2
andersca: review-
fix patch 3
andersca: review-
fix patch 4 none

Description Chang Shu 2011-06-02 10:37:10 PDT
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.
Comment 1 Chang Shu 2011-06-02 10:44:00 PDT
Created attachment 95774 [details]
fix patch
Comment 2 Kenneth Rohde Christiansen 2011-06-05 09:25:37 PDT
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
Comment 3 Chang Shu 2011-06-06 08:12:12 PDT
Created attachment 96093 [details]
fix patch 2

Thanks, Kenneth. Your review made a lot sense and I changed my code accordingly.
Comment 4 Laszlo Gombos 2011-06-07 14:30:06 PDT
This looks good to me; CC Anders Carlsson for a second set of eyes.
Comment 5 Anders Carlsson 2011-06-07 15:11:46 PDT
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.
Comment 6 Chang Shu 2011-06-08 08:09:34 PDT
(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.
Comment 7 Anders Carlsson 2011-06-15 13:50:30 PDT
(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?
Comment 8 Chang Shu 2011-06-15 13:55:05 PDT
> 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. :(
Comment 9 Chang Shu 2011-06-15 14:31:31 PDT
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.
Comment 10 Anders Carlsson 2011-06-15 14:33:50 PDT
(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?
Comment 11 Anders Carlsson 2011-06-15 14:34:06 PDT
(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.
Comment 12 Chang Shu 2011-06-15 14:38:38 PDT
> > int newStdout = open("/dve/null", O_WRONLY);
the problem was this typo (*dve*). :(
Comment 13 Chang Shu 2011-06-16 09:00:55 PDT
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.
Comment 14 Anders Carlsson 2011-06-16 09:59:21 PDT
(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?
Comment 15 Chang Shu 2011-06-16 10:08:21 PDT
> > 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.
Comment 16 Anders Carlsson 2011-06-16 10:38:21 PDT
(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?
Comment 17 Chang Shu 2011-06-16 11:18:23 PDT
> > 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
Comment 18 Anders Carlsson 2011-06-16 11:21:45 PDT
(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.
Comment 19 Chang Shu 2011-06-16 11:33:23 PDT
> 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.
Comment 20 Chang Shu 2011-06-17 10:17:04 PDT
Created attachment 97613 [details]
fix patch 3
Comment 21 Anders Carlsson 2011-06-17 10:23:36 PDT
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.
Comment 22 Chang Shu 2011-06-17 10:43:00 PDT
Created attachment 97616 [details]
fix patch 4
Comment 23 WebKit Review Bot 2011-06-17 11:23:36 PDT
Comment on attachment 97616 [details]
fix patch 4

Clearing flags on attachment: 97616

Committed r89150: <http://trac.webkit.org/changeset/89150>
Comment 24 WebKit Review Bot 2011-06-17 11:23:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Laszlo Gombos 2011-06-17 15:44:41 PDT
*** Bug 62911 has been marked as a duplicate of this bug. ***