Bug 106140 – [Qt] Major performance improvement in Qt's PluginDatabase implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[David Faure]

load() calls NP_Initialize, which is very slow and even leads to nppdf.so
showing the acrobat reader license dialog.

fetchInfo() doesn't need to call all that code, it only needs to load the QLibrary.

[David Faure]

Created attachment 181434 [details]
the patch

Now with the patch, oops.

I'm wondering if my approach is valid: googling for NP_Initialize and NP_GetValue API docs seems to indicate that the browser is supposed to call NP_Initialize before NP_GetValue? It appears to work fine, though, all plugins I have here return correct stuff (description and mimetypes) from NP_GetValue even without calling NP_Initialize.


Note that I had to remove "m_infoIsFromCache = false" so that ensurePluginLoaded() doesn't return m_isLoaded (which is false at that point, when called from PluginDatabase::pluginForMIMEType). Basically this is what PluginPackageWin.cpp does as well (not calling load from fetchInfo, and not setting m_infoIsFromCache to false), while PluginPackageMac.cpp calls load from fetchInfo and sets that bool to false (like the Qt code was doing before this patch). So I presume this is correct (it does fix the actual loading of plugins), even though I don't fully understand the m_infoIsFromCache logic.

[Alexis Menard (darktears)]

Hi David,

You need to run the script in Tools/Script/prepare-ChangeLog -b 106140 to generate the ChangeLog.

And then you can request the review and commit queue flag to ?

[Simon Hausmann]

Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?

[David Faure]

Created attachment 181456 [details]
the patch with changelog added

[David Faure]

(In reply to comment #3)
> Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?

The Windows implementation already does this.
The Mac implementation could do pretty much the same as this patch (call CFBundleCreate directly in fetchInfo), but I can't test that, so better if a Mac guy does that. As you can see, no change is required in the shared code, so implementations can do this optimization independently from each other.

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #3)
> > Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?
> 
> The Windows implementation already does this.
> The Mac implementation could do pretty much the same as this patch (call CFBundleCreate directly in fetchInfo), but I can't test that, so better if a Mac guy does that. As you can see, no change is required in the shared code, so implementations can do this optimization independently from each other.

I was thinking of Gtk and EFL :)

[David Faure]

Ah. Same issue: I can make a very similar patch, but I can't test it.


Also, I just realized that with this patch, m_infoIsFromCache is now completely unused (it's always true, for all implementations). I can add the cleanup of that bool to the patch.

[Simon Hausmann]

Comment on attachment 181456 [details]
the patch with changelog added

r=me

Yeah, sounds like the cleanup of m_infoIsFromCache should be a separate cleanup patch.

Other suggestions:

    * Move the QLibrary initializing code into a separate shared help method instead of duplicating the code?
    * Make PluginPackage::fetchInfo() shared code between Qt, Gtk and EFL. There's little reason I think for this code to be duplicated and if shared the other platforms could benefit from the same optimization.

[WebKit Review Bot]

Comment on attachment 181456 [details]
the patch with changelog added

Clearing flags on attachment: 181456

Committed r138944: <http://trac.webkit.org/changeset/138944>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #9)
> (From update of attachment 181456 [details])
> Clearing flags on attachment: 181456
> 
> Committed r138944: <http://trac.webkit.org/changeset/138944>

It made 28 tests crash - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20NRWT/builds/26195

Could you check it?

[WebKit Review Bot]

Re-opened since this is blocked by bug 106223

[Csaba Osztrogonác]

(In reply to comment #12)
> Re-opened since this is blocked by bug 106223

rollout landed in http://trac.webkit.org/changeset/138948

[Simon Hausmann]

Thanks for the rollout, Ossy

[David Faure]

I can't reproduce the crashes.
http://paste.kde.org/641462/
http://paste.kde.org/641486/
I tried a number of others (from the list at https://gist.github.com/4483529), but they all run fine (except LayoutTests/http/tests/plugins/get-url.html because I don't have apache installed).


I suspect that the CI machine has different plugins installed than I do. How can I find more information about which plugins it has... and/or how can I get a full backtrace in debug mode, from the CI machine?
Or, maybe someone else can try the patch locally to see if he manages to get a crash?


Simon: of course I thought about factorizing the code, but that's a bit messy with the header being shared with other implementations. I'd have to add the method declaration in the .h inside some sort ifdef QT, or I would have to make a file-static helper function but then it can't access members, making the code ugly.

For the sharing with other impls, that's rather unrelated to this patch (could have been done before, too). Let's come back to that once the basic patch is in...

[Csaba Osztrogonác]

Tests pass if you run only one, but many of them crash if you run all plugins
directory. I tried r138944 in debug mode and I got crash after the 4. plugins
test:

LayoutTests/platform/qt/plugins/qt-qwidget-plugin.html
LayoutTests/plugins/access-after-page-destroyed.html
LayoutTests/plugins/attach-during-destroy.html
LayoutTests/plugins/change-widget-and-click-crash.html

And here is the GDB bactracke with all necessary information:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4545d98 in WebCore::PluginPackage::freeLibraryTimerFired (this=0x6a44a0) at /home/oszi/WebKit/Source/WebCore/plugins/PluginPackage.cpp:70
70          ASSERT(!m_loadCount);
(gdb) bt
#0  0x00007ffff4545d98 in WebCore::PluginPackage::freeLibraryTimerFired (this=0x6a44a0) at /home/oszi/WebKit/Source/WebCore/plugins/PluginPackage.cpp:70
#1  0x00007ffff4547256 in WebCore::Timer<WebCore::PluginPackage>::fired (this=0x6a4790) at /home/oszi/WebKit/Source/WebCore/platform/Timer.h:106
#2  0x00007ffff4531fe9 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x6bbbd0) at /home/oszi/WebKit/Source/WebCore/platform/ThreadTimers.cpp:116
#3  0x00007ffff4531f13 in WebCore::ThreadTimers::sharedTimerFired () at /home/oszi/WebKit/Source/WebCore/platform/ThreadTimers.cpp:93
#4  0x00007ffff488fd80 in WebCore::SharedTimerQt::timerEvent (this=0x6bbc00, ev=0x7fffffffdd50) at /home/oszi/WebKit/Source/WebCore/platform/qt/SharedTimerQt.cpp:113
#5  0x00007fffe8d49439 in QObject::event(QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#6  0x00007fffea3db8cc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5
#7  0x00007fffea3e1beb in QApplication::notify(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5
#8  0x00007fffe8d24c04 in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#9  0x00007fffe8d700f2 in QTimerInfoList::activateTimers() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#10 0x00007fffe8d70b5d in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#11 0x00007fffec1ee6f2 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#12 0x00007fffec1f2568 in ?? () from /lib/libglib-2.0.so.0
#13 0x00007fffec1f271c in g_main_context_iteration () from /lib/libglib-2.0.so.0
#14 0x00007fffe8d7081b in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#15 0x00007fffe8d23e4b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#16 0x00007fffe8d29e4d in QCoreApplication::exec() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#17 0x0000000000431d54 in main (argc=2, argv=0x7fffffffe3b8) at /home/oszi/WebKit/Tools/DumpRenderTree/qt/DumpRenderTreeMain.cpp:203

[David Faure]

Created attachment 182463 [details]
new patch, with refcounting unchanged

The assert has been fixed in bug 106463. There was another issue, the unloading of flash actually crashes. This new patch solves this.

[David Faure]

Alexis, Simon: ping?

(Patch is ready for review)

[WebKit Review Bot]

Comment on attachment 182463 [details]
new patch, with refcounting unchanged

Clearing flags on attachment: 182463

Committed r141240: <http://trac.webkit.org/changeset/141240>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.