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.
Created attachment 95774 [details] fix patch
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
Created attachment 96093 [details] fix patch 2 Thanks, Kenneth. Your review made a lot sense and I changed my code accordingly.
This looks good to me; CC Anders Carlsson for a second set of eyes.
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.
(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.
(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?
> 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. :(
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.
(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?
(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.
> > int newStdout = open("/dve/null", O_WRONLY); the problem was this typo (*dve*). :(
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.
(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?
> > 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.
(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?
> > 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
(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.
> 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.
Created attachment 97613 [details] fix patch 3
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.
Created attachment 97616 [details] fix patch 4
Comment on attachment 97616 [details] fix patch 4 Clearing flags on attachment: 97616 Committed r89150: <http://trac.webkit.org/changeset/89150>
All reviewed patches have been landed. Closing bug.
*** Bug 62911 has been marked as a duplicate of this bug. ***