Bug 57617

Created attachment 87853 [details] Patch

Expect EWS redness because it has been created on top of the dependency patch.

Comment on attachment 87853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87853&action=review > Source/WebKit2/PluginProcess/PluginProcess.cpp:160 > +#elif PLATFORM(QT) > + int sockets[2]; > + if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sockets) == -1) { > + qDebug() << "Creation of socket failed with errno:" << errno; Wouldn't this work for any UNIX? What about making a PluginProcessUNIX instead? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:92 > + "QtWebProcess" : "QtPluginProcess"); I think that should be called QtWebPluginProcess not just plugin process. > Source/WebKit2/qt/EnvHttpProxyFactory.h:34 > +#include <QList> > +#include <QNetworkProxyFactory> > + > +namespace WebKit { > + > +class EnvHttpProxyFactory : public QNetworkProxyFactory { > +public: > + static void initializeProxy(); I would have done this splitting out as a separate patch > Source/WebKit2/qt/PluginMainQt.cpp:3 > + * Copyright (C) 2011 University of Szeged Don't you want a dot at the end?

(In reply to comment #3) > (From update of attachment 87853 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87853&action=review > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:160 > > +#elif PLATFORM(QT) > > + int sockets[2]; > > + if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sockets) == -1) { > > + qDebug() << "Creation of socket failed with errno:" << errno; > > Wouldn't this work for any UNIX? What about making a PluginProcessUNIX instead? Definitely this patch should not be Qt-only. Furthermore I would like a change that replacing the "PLATFORM(QT) || PLATFORM(GTK)" conditions in IPC code with smg new macro flag (UNIX_IPC is my candidate). This should be handled first (maybe in #49791?). I just uploaded this patch now for feedback. > > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:92 > > + "QtWebProcess" : "QtPluginProcess"); > > I think that should be called QtWebPluginProcess not just plugin process. Yep. > > > Source/WebKit2/qt/EnvHttpProxyFactory.h:34 > > +#include <QList> > > +#include <QNetworkProxyFactory> > > + > > +namespace WebKit { > > + > > +class EnvHttpProxyFactory : public QNetworkProxyFactory { > > +public: > > + static void initializeProxy(); > > I would have done this splitting out as a separate patch Ok. > > > Source/WebKit2/qt/PluginMainQt.cpp:3 > > + * Copyright (C) 2011 University of Szeged > > Don't you want a dot at the end? Yep. I will return to this after the UNIX_IPC thing will be done.

Created attachment 88915 [details] Patch

Comment on attachment 88915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88915&action=review The patch doesn't seem to apply to current trunk > Source/WebKit2/PluginProcess/PluginProcess.cpp:161 > + qDebug() << "Creation of socket failed with errno:" << errno; This looks qt specific

(In reply to comment #6) > (From update of attachment 88915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88915&action=review > > The patch doesn't seem to apply to current trunk Sorry, I haven't noticed this patch depends on patch attached to bug #55719

(In reply to comment #6) > (From update of attachment 88915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88915&action=review > > The patch doesn't seem to apply to current trunk > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:161 > > + qDebug() << "Creation of socket failed with errno:" << errno; > > This looks qt specific Good catch. I will fix it after the dependency patch has been landed.

Created attachment 89585 [details] Patch

Attachment 89585 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 Source/WebKit2/qt/PluginMainQt.cpp:27: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #10) > Attachment 89585 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 > > Source/WebKit2/qt/PluginMainQt.cpp:27: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 15 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Need to be fixed by s/<config.h>/"config.h"/ I would handle this before landing if the patch is good otherwise.

ping

Comment on attachment 89585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89585&action=review > Source/WebKit2/Platform/CoreIPC/Attachment.h:53 > - Attachment(int fileDescriptor, size_t); > + Attachment(int fileDescriptor, size_t = 0); This API look rather strange, it should be split into two constructors. > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-72 > - ASSERT(!isNull()); > - Benjamin: we are in SharedMemoryUnix, so the assert totally make sense. So Either you should have a new encoder for Arguments (with new asserts depending on the type), or you need to fix the broken assertions. > Source/WebKit2/PluginProcess/PluginProcess.cpp:48 > +// FIXME: the LOG macro don't work without that. Should be fixed there. > +using namespace WebCore; Benjamin: Should you rely add a dependency to WebCore here? You don't want to link to WebCore. > Source/WebKit2/PluginProcess/PluginProcess.cpp:190 > + ASSERT_NOT_REACHED(); > + return; Benjamin: you have cleaning code to close the socket on the previous call to fcntl, but not on this one. > Source/WebKit2/PluginProcess/PluginProcess.cpp:200 > - ASSERT_NOT_REACHED(); > + notImplemented(); With your change it does not assert in debug. The notImplemented() instead of the comment is a good idea. > Source/WebKit2/PluginProcess/qt/PluginControllerProxyQt.cpp:48 > -void PluginControllerProxy::platformGeometryDidChange(const IntRect& frameRect, const IntRect&) > +void PluginControllerProxy::platformGeometryDidChange() Benjamin: ??? > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:36 > +#include <QDebug> Benjamin: ??? > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:38 > +#include <QtGlobal> Benjamin: ??? > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:50 > + QApplication* app = new QApplication(argc, argv); Benjamin: why are you creating that on the heap (and leaking it?)?

(In reply to comment #13) > (From update of attachment 89585 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89585&action=review > > > Source/WebKit2/Platform/CoreIPC/Attachment.h:53 > > - Attachment(int fileDescriptor, size_t); > > + Attachment(int fileDescriptor, size_t = 0); > > This API look rather strange, it should be split into two constructors. Accept. > > > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-72 > > - ASSERT(!isNull()); > > - > > Benjamin: we are in SharedMemoryUnix, so the assert totally make sense. > So Either you should have a new encoder for Arguments (with new asserts depending on the type), or you need to fix the broken assertions. As I wrote in the changelog this change was made because in PluginProxy::geometryDidChange we pass a null Handle if we do not need to allocate a new backing store. It has nothing to do with the Attachment changes. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:48 > > +// FIXME: the LOG macro don't work without that. Should be fixed there. > > +using namespace WebCore; > > Benjamin: Should you rely add a dependency to WebCore here? You don't want to link to WebCore. This is in the core part (i.e. the webkit library), not in the binary of the plugin process. Naturally this depends on WebCore. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:190 > > + ASSERT_NOT_REACHED(); > > + return; > > Benjamin: you have cleaning code to close the socket on the previous call to fcntl, but not on this one. Accept. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:200 > > - ASSERT_NOT_REACHED(); > > + notImplemented(); > > With your change it does not assert in debug. The notImplemented() instead of the comment is a good idea. Do you mean keep the ASSERT_NOT_REACHED()? Actually the notImplemented() is also a debug only thing. I can keep the assert but in this case an ASSERT_WITH_MESSAGE makes more sense. > > > Source/WebKit2/PluginProcess/qt/PluginControllerProxyQt.cpp:48 > > -void PluginControllerProxy::platformGeometryDidChange(const IntRect& frameRect, const IntRect&) > > +void PluginControllerProxy::platformGeometryDidChange() > > Benjamin: ??? This is a build fix. > > > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:36 > > +#include <QDebug> > > Benjamin: ??? It is used. What is the question? :) > > > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:38 > > +#include <QtGlobal> > > Benjamin: ??? For Q_DECL_EXPORT. > > > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:50 > > + QApplication* app = new QApplication(argc, argv); > > Benjamin: why are you creating that on the heap (and leaking it?)? Accept.

Created attachment 91292 [details] Patch

Attachment 91292 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 Source/WebKit2/Platform/CoreIPC/Attachment.h:69: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 91292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91292&action=review > Source/WebKit2/PluginProcess/PluginProcess.cpp:45 > +#if PLATFORM(QT) > +#include <QDebug> > +#include <errno.h> > +#include <fcntl.h> > +#include <sys/resource.h> > +#include <sys/socket.h> > +#include <unistd.h> This should be splitted into PLATFORM(QT) with QDebug only and everythying else in another #ifdef using USE(UNIX_DOMAIN_SOCKETS) > Source/WebKit2/PluginProcess/PluginProcess.cpp:165 > + qDebug() << "Creation of socket failed with errno:" << errno; This is still qt specific

Comment on attachment 91292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91292&action=review > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-71 > - ASSERT(!isNull()); Why? > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-91 > - ASSERT(!isNull()); Why? > Source/WebKit2/PluginProcess/PluginProcess.cpp:40 > +#include <QDebug> Please remove. >> Source/WebKit2/PluginProcess/PluginProcess.cpp:165 >> + qDebug() << "Creation of socket failed with errno:" << errno; > > This is still qt specific And even Qt specific we don't put qDebugs. Please remove. > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:37 > +#include <QDebug> Please remove. > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:64 > + qDebug() << "Error: wrong number of arguments."; Please remove. > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:71 > + qDebug() << "Error: connection identifier wrong."; Please remove.

Comment on attachment 91292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91292&action=review > Source/WebKit2/ChangeLog:29 > + Adjusted to the changes in Attachment. > + Remove assertions that check that the Handle is not null. > + In PluginProxy::geometryDidChange we pass a null Handle if > + we do not need to allocate a new backing store. This is not possible with our current ConnectionUnix impementation. Handles are sent as attachments using control messages. sendmsg() fails with EBADF when an invalid file descriptor (-1) is passed. Because of this, geometryDidChange is only received the first time in the plugin process, then a -1 handle is passed and sendmsg() always fails.

Thanks for the review! It's a timed out reaction and this patch is obsolete now but I have a wish and a question. (In reply to comment #18) > (From update of attachment 91292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91292&action=review > > > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-71 > > - ASSERT(!isNull()); > > Why? > > > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-91 > > - ASSERT(!isNull()); > > Why? Noem has also asked about these and it was explained in the ChangeLog both the first and the second time. Although the change is wrong as Carlos pointed out I would ask you guys to read the ChangeLog when reviewing. It takes a lot of time (at least for me) to write a ChangeLog that correctly explain the change so at least we should use it. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:40 > > +#include <QDebug> > > Please remove. > > >> Source/WebKit2/PluginProcess/PluginProcess.cpp:165 > >> + qDebug() << "Creation of socket failed with errno:" << errno; > > > > This is still qt specific > > And even Qt specific we don't put qDebugs. Please remove. I'm surprised because there is a lot of qDebug in our codebase. So, what should I use for situations like this?

> > Noem has also asked about these and it was explained in the ChangeLog both the first and the second time. Sorry, it was Benjamin of course.

*** This bug has been marked as a duplicate of bug 60629 ***