Bug 80691 - [Qt] X11 plugins need to be reworked for Qt5+WK1
Summary: [Qt] X11 plugins need to be reworked for Qt5+WK1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 79668
  Show dependency treegraph
 
Reported: 2012-03-09 02:41 PST by Fehér Zsolt
Modified: 2012-05-16 09:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch Wk1 (22.48 KB, patch)
2012-03-09 02:41 PST, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (28.02 KB, patch)
2012-03-22 05:22 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (34.69 KB, patch)
2012-04-24 07:01 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (34.69 KB, patch)
2012-04-24 07:22 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (34.75 KB, patch)
2012-04-24 10:03 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (33.58 KB, patch)
2012-05-07 08:41 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fehér Zsolt 2012-03-09 02:41:34 PST
Created attachment 131018 [details]
Patch Wk1

The video don't work in this patch.
Any idea why?
Comment 1 Balazs Kelemen 2012-03-22 05:22:32 PDT
Created attachment 133229 [details]
Patch
Comment 2 Simon Hausmann 2012-04-16 01:30:01 PDT
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 3 Simon Hausmann 2012-04-16 01:51:34 PDT
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.
Comment 4 Balazs Kelemen 2012-04-16 04:18:35 PDT
(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.
Comment 5 Balazs Kelemen 2012-04-24 07:01:23 PDT
Created attachment 138551 [details]
Patch
Comment 6 Balazs Kelemen 2012-04-24 07:17:34 PDT
(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 7 Early Warning System Bot 2012-04-24 07:18:18 PDT
Comment on attachment 138551 [details]
Patch

Attachment 138551 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12523238
Comment 8 Balazs Kelemen 2012-04-24 07:22:55 PDT
Created attachment 138555 [details]
Patch
Comment 9 Early Warning System Bot 2012-04-24 07:48:53 PDT
Comment on attachment 138555 [details]
Patch

Attachment 138555 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12520221
Comment 10 Balazs Kelemen 2012-04-24 10:03:32 PDT
Created attachment 138588 [details]
Patch
Comment 11 Simon Hausmann 2012-05-02 11:34:17 PDT
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?
Comment 12 Balazs Kelemen 2012-05-07 08:41:15 PDT
Created attachment 140532 [details]
Patch
Comment 13 Balazs Kelemen 2012-05-07 08:41:50 PDT
(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 14 Simon Hausmann 2012-05-07 13:15:20 PDT
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? :)
Comment 15 Balazs Kelemen 2012-05-07 15:16:58 PDT
(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
Comment 16 Balazs Kelemen 2012-05-07 15:25:22 PDT
> Unfortunately not yet, because of this bug: #83024

Hm, I thought it works. So, the bug is wkb.ug/83024
Comment 17 Balazs Kelemen 2012-05-08 01:41:48 PDT
Comment on attachment 140532 [details]
Patch

Clearing flags on attachment: 140532

Committed r116403: <http://trac.webkit.org/changeset/116403>
Comment 18 Balazs Kelemen 2012-05-08 01:41:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogonác 2012-05-08 13:00:34 PDT
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.)
Comment 20 Balazs Kelemen 2012-05-09 02:25:48 PDT
(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.
Comment 21 Csaba Osztrogonác 2012-05-10 08:08:32 PDT
Are you guys working on fixing these crashes?
Comment 22 Balazs Kelemen 2012-05-11 01:25:32 PDT
(In reply to comment #21)
> Are you guys working on fixing these crashes?

I promise I will :)
Comment 23 Balazs Kelemen 2012-05-15 05:00:50 PDT
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.
Comment 24 Balazs Kelemen 2012-05-16 06:40:02 PDT
(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)
Comment 25 Balazs Kelemen 2012-05-16 06:58:07 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=86620
Comment 26 Balazs Kelemen 2012-05-16 09:45:28 PDT
(In reply to comment #25)
> Filed https://bugs.webkit.org/show_bug.cgi?id=86620

Workaround landed, crashed disappeared.