Bug 80691

Created attachment 133229 [details] Patch

Comment on attachment 133229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133229&action=review Looks good in general, but I have a couple of nitpicks. I have the feeling that there's a bit more code that can be shared and that there may be ways of getting access to the things like window handles. > Source/WebCore/plugins/qt/PluginPackageQt.cpp:143 > + const QLatin1String pluginBlackList[] = { > + QStringLiteral("skypebuttons") Type mismatch :) > Source/WebCore/plugins/qt/PluginPackageQt.cpp:150 > + QString baseName = QFileInfo(static_cast<QString>(m_path)).baseName(); > + for (unsigned i = 0; i < sizeof(pluginBlackList) / sizeof(QLatin1String); ++i) { > + if (baseName == pluginBlackList[i]) > + return false; > + } Would it perhaps make sense to use a pattern here similar to PluginPackageWin with a dedicated isPluginBlacklisted() method? (just for clarity) > Source/WebCore/plugins/qt/PluginViewQt.cpp:122 > +#if HAVE(QT5) > + static Display* dedicatedDisplay = 0; > + if (!dedicatedDisplay) > + dedicatedDisplay = XOpenDisplay(0); > + ASSERT(dedicatedDisplay); > + return dedicatedDisplay; > +#else Any particular reason for a dedicated display connection? Perhaps the xcb implementation of QPlatformNativeInterface::nativeResourceForWindow could return the default display if no window pointer is passed. > Source/WebCore/plugins/qt/PluginViewQt.cpp:133 > +#if HAVE(QT5) > + return XDefaultScreen(x11Display()); > +#else > + return QX11Info::appScreen(); > +#endif Why not always use XDefaultScreen? The Qt4 implementation appears to cache the return value of XDefaultScreen. I think it would be good to check if recent xcb/xlib always does a round-trip or also caches, and if it does cache then I don't see any harm in simply using XDefaultScreen with all Qt versions (or we cache ourselves). > Source/WebCore/plugins/qt/PluginViewQt.cpp:142 > +#if HAVE(QT5) > + return XDefaultRootWindow(x11Display()); > +#else > + return QX11Info::appRootWindow(); > +#endif Ditto :) > Source/WebCore/plugins/qt/PluginViewQt.cpp:151 > +#if HAVE(QT5) > + return XDefaultDepth(x11Display(), x11Screen()); > +#else > + return static_cast<QWidget*>(platformPluginWidget())->x11Info().depth(); > +#endif Ditto ;) > Source/WebCore/plugins/qt/PluginViewQt.cpp:160 > +#if HAVE(QT5) > + XSync(x11Display(), false); > +#else > + QApplication::syncX(); > +#endif Same here, perhaps we can avoid the #ifdefs altogether... > Source/WebCore/plugins/qt/PluginViewQt.cpp:250 > +#if !HAVE(QT5) > if (platformPluginWidget()) { > if (focused) > static_cast<QWidget*>(platformPluginWidget())->setFocus(Qt::OtherFocusReason); > - } else { > + } else > +#endif Hm, I suppose you commented this out because we cannot use QWidget methods here? I recall running into a similar issue with widget visibility when removing the widgets dependency from WebCore and I just added a hook in QWebPageClient (setWidgetVisible). The same could be done for focus, to avoid regressing here. > Source/WebCore/plugins/qt/PluginViewQt.cpp:417 > +#if HAVE(QT5) > + xEvent->xany.window = 0; > +#else Isn't there a way to still get the window handle for ownerWidget? > Source/WebCore/plugins/qt/PluginViewQt.cpp:954 > +#if HAVE(QT5) > + if (m_isWindowed) > + return false; > +#else I think the fact that we skip this branch for Qt 5 deserves a short explaining comment. > Source/WebCore/plugins/qt/PluginViewQt.cpp:1024 > + bool found = getVisualAndColormap(depth, m_visual, m_colormap, false); Please use /* forceARGB32 = */ false instead of just false to increase the readability of the code.

Comment on attachment 133229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133229&action=review >> Source/WebCore/plugins/qt/PluginViewQt.cpp:122 >> +#else > > Any particular reason for a dedicated display connection? Perhaps the xcb implementation of QPlatformNativeInterface::nativeResourceForWindow could return the default display if no window pointer is passed. I just checked, calling nativeResouceForWindow("display", 0) should work and return the primary display.

(In reply to comment #3) > (From update of attachment 133229 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133229&action=review > > >> Source/WebCore/plugins/qt/PluginViewQt.cpp:122 > >> +#else > > > > Any particular reason for a dedicated display connection? Perhaps the xcb implementation of QPlatformNativeInterface::nativeResourceForWindow could return the default display if no window pointer is passed. > > I just checked, calling nativeResouceForWindow("display", 0) should work and return the primary display. Cool. I think I should take a deeper look to these new QPlatformNativeInterface getters before continuing to work on this. Maybe there is also a way to avoid copying the frames from the server in which case the patch needs a bigger reworking.

Created attachment 138551 [details] Patch

(In reply to comment #2) > (From update of attachment 133229 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133229&action=review > > Looks good in general, but I have a couple of nitpicks. I have the feeling that there's a bit more code that can be shared and that there may be ways of getting access to the things like window handles. > > > Source/WebCore/plugins/qt/PluginPackageQt.cpp:143 > > + const QLatin1String pluginBlackList[] = { > > + QStringLiteral("skypebuttons") > > Type mismatch :) Fixed. > > Source/WebCore/plugins/qt/PluginPackageQt.cpp:150 > > + QString baseName = QFileInfo(static_cast<QString>(m_path)).baseName(); > > + for (unsigned i = 0; i < sizeof(pluginBlackList) / sizeof(QLatin1String); ++i) { > > + if (baseName == pluginBlackList[i]) > > + return false; > > + } Fixed. > Would it perhaps make sense to use a pattern here similar to PluginPackageWin with a dedicated isPluginBlacklisted() method? (just for clarity) Fixed. > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:122 > > +#if HAVE(QT5) > > + static Display* dedicatedDisplay = 0; > > + if (!dedicatedDisplay) > > + dedicatedDisplay = XOpenDisplay(0); > > + ASSERT(dedicatedDisplay); > > + return dedicatedDisplay; > > +#else > > Any particular reason for a dedicated display connection? Perhaps the xcb implementation of QPlatformNativeInterface::nativeResourceForWindow could return the default display if no window pointer is passed. > I have changed the patch to use nativeResourceForWindow, and also added a cache for these values (and removed the ifdefs). Unfortunately it's a bit overkill. We need the top level QWindow, but for QGraphicsWidget I did not find any other way to get it other than QApplicationPrivate::windowForWidgets. This means we need to add "QT+=widgets-private gui-private core-private" (the latter two is necessary unless include paths are broken). I guess using a dedicated connection is not worst technically, so maybe it's better to go with that. I can redid this part of the patch if you agree with that. > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:250 > > +#if !HAVE(QT5) > > if (platformPluginWidget()) { > > if (focused) > > static_cast<QWidget*>(platformPluginWidget())->setFocus(Qt::OtherFocusReason); > > - } else { > > + } else > > +#endif > > Hm, I suppose you commented this out because we cannot use QWidget methods here? I recall running into a similar issue with widget visibility when removing the widgets dependency from WebCore and I just added a hook in QWebPageClient (setWidgetVisible). The same could be done for focus, to avoid regressing here. There is no need to do that now since platformPluginWidget() is always null for windowless plugins and I only implemented them. Added a comment to make it clear. > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:417 > > +#if HAVE(QT5) > > + xEvent->xany.window = 0; > > +#else > > Isn't there a way to still get the window handle for ownerWidget? Fixed, but also this is not important for windowless plugins. > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:954 > > +#if HAVE(QT5) > > + if (m_isWindowed) > > + return false; > > +#else > > I think the fact that we skip this branch for Qt 5 deserves a short explaining comment. Added comment. > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:1024 > > + bool found = getVisualAndColormap(depth, m_visual, m_colormap, false); > > Please use /* forceARGB32 = */ false instead of just false to increase the readability of the code. Fixed.

Comment on attachment 138551 [details] Patch Attachment 138551 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12523238

Created attachment 138555 [details] Patch

Comment on attachment 138555 [details] Patch Attachment 138555 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12520221

Created attachment 138588 [details] Patch

Comment on attachment 138588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138588&action=review > Source/WebCore/plugins/qt/PluginViewQt.cpp:940 > + QWebPageClient* pageClient = platformPageClient(); > + if (!pageClient) > + return false; > + QWindow* window = pageClient->ownerWindow(); > + display = static_cast<Display*>(QGuiApplication::platformNativeInterface()->nativeResourceForWindow("display", window)); I don't think you need all this and instead can simply use display = static_cast<Display*>(QGuiApplication::platformNativeInterface()->nativeResourceForWindow("display", 0)); Functionally that's equivalent to QX11Info::display() I believe. > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:497 > + return ownerWidget() ? QApplicationPrivate::windowForWidget(ownerWidget()) : 0; Looking at the implementation of windowForWidget makes me wonder: Why not simply do what this function does ourselves and thus avoid the dependency to private API?

Created attachment 140532 [details] Patch

(In reply to comment #11) > (From update of attachment 138588 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138588&action=review > > > Source/WebCore/plugins/qt/PluginViewQt.cpp:940 > > + QWebPageClient* pageClient = platformPageClient(); > > + if (!pageClient) > > + return false; > > + QWindow* window = pageClient->ownerWindow(); > > + display = static_cast<Display*>(QGuiApplication::platformNativeInterface()->nativeResourceForWindow("display", window)); > > I don't think you need all this and instead can simply use > > display = static_cast<Display*>(QGuiApplication::platformNativeInterface()->nativeResourceForWindow("display", 0)); > > Functionally that's equivalent to QX11Info::display() I believe. Done. > > > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:497 > > + return ownerWidget() ? QApplicationPrivate::windowForWidget(ownerWidget()) : 0; > > Looking at the implementation of windowForWidget makes me wonder: Why not simply do what this function does ourselves and thus avoid the dependency to private API? Done.

Comment on attachment 140532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140532&action=review > Source/WebCore/ChangeLog:15 > + No new tests, covered by existing plugin tests. ... that I suppose can be unskipped now in the qt5 skip file? :)

(In reply to comment #14) > (From update of attachment 140532 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140532&action=review > > > Source/WebCore/ChangeLog:15 > > + No new tests, covered by existing plugin tests. > > ... that I suppose can be unskipped now in the qt5 skip file? :) Unfortunately not yet, because of this bug: #83024

> Unfortunately not yet, because of this bug: #83024 Hm, I thought it works. So, the bug is wkb.ug/83024

Comment on attachment 140532 [details] Patch Clearing flags on attachment: 140532 Committed r116403: <http://trac.webkit.org/changeset/116403>

All reviewed patches have been landed. Closing bug.

Reopen this one too. https://bugs.webkit.org/show_bug.cgi?id=83024 made tests crash on Qt-WK2 bot once ... Now this one made the same tests crash: - compositing/repaint/become-overlay-composited-layer.html - fast/frames/iframe-remove-after-id-change.html Could you check it? It seems there is a serious bug near plugins on Qt-WK2. (FYI: I double checked and reverted this patch locally and everything works.)

(In reply to comment #19) > Reopen this one too. https://bugs.webkit.org/show_bug.cgi?id=83024 made tests crash on Qt-WK2 bot once ... Now this one made the same tests crash: > - compositing/repaint/become-overlay-composited-layer.html > - fast/frames/iframe-remove-after-id-change.html > > Could you check it? It seems there is a serious bug near plugins on Qt-WK2. > (FYI: I double checked and reverted this patch locally and everything works.) Thanks for pointing this out. I think it is some issue with accelerated compositing. The backtraces I get when tried to debug these crashes with the testnetscapeplugin patch supports this theory. My knowladge is extremely limited in AC but I know that plugins can be a reason why a layer become accelerated. I will try to find some time debugging in this week but I won't promise I will be able to solve this.

Are you guys working on fixing these crashes?

(In reply to comment #21) > Are you guys working on fixing these crashes? I promise I will :)

I think we are hitten by an nrwt bug. I could not reproduce any crash, even when nrwt reports crashes there is no core dump generated (with ulimit -c unlimited), so I think this is a false alarm from nrwt. Filed https://bugs.webkit.org/show_bug.cgi?id=86467. It's still a question how it is related to plugins and what can we do.

(In reply to comment #23) > I think we are hitten by an nrwt bug. I could not reproduce any crash, even when nrwt reports crashes there is no core dump generated (with ulimit -c unlimited), so I think this is a false alarm from nrwt. Filed https://bugs.webkit.org/show_bug.cgi?id=86467. It's still a question how it is related to plugins and what can we do. I was wrong. This is not an nrwt bug. What really happening is that the web process is somehow blocked while waiting for the plugin process connection. The tests that actually crash are not using plugins so it is likely a result of a previous test. It is considered as a crash because WTR reports crash if the web process don't answer back when it should reset the default settings before a new test. Maybe this should be improved. This patch triggered this problem because it turned on the build of the test netscape plugin. The long term solution is to fix the blocking bug but I would like to avoid the crashers ASAP. There is two possible solution for that: * move all tests that use plugins to the skip list * remove testnetscapeplugin from the build until fix I vote for the second choice because: * some of these tests can work without the test plugin. These are usually test some behaviour in connection to plugins but don't really test the plugin itself. * no need to skip new tests that use plugins in some way (use the obect or embed tags)

Filed https://bugs.webkit.org/show_bug.cgi?id=86620

(In reply to comment #25) > Filed https://bugs.webkit.org/show_bug.cgi?id=86620 Workaround landed, crashed disappeared.