Bug 115650 – [GTK][WK2] Blocks when fetching plugins information

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Claudio Saavedra]

bool PluginProcessProxy::scanPlugin() is a synchronous method and it's using g_spawn_sync(), which blocks for a couple of seconds every now and then:

(gdb) back
#0  0x0000003a2c0ec653 in select () at ../sysdeps/unix/syscall-template.S:81
#1  0x0000003a2e489957 in g_spawn_sync (working_directory=<optimized out>,
    argv=<optimized out>, envp=<optimized out>, flags=<optimized out>,
    child_setup=<optimized out>, user_data=<optimized out>,
    standard_output=0x7fffffffd4e8, standard_error=0x0, exit_status=0x7fffffffd4bc,
    error=0x0) at gspawn.c:328
#2  0x0000003a55cd6cbf in WebKit::PluginProcessProxy::scanPlugin ()
   from /lib64/libwebkit2gtk-3.0.so.25
#3  0x0000003a55c481fa in WebKit::NetscapePluginModule::getPluginInfo ()
   from /lib64/libwebkit2gtk-3.0.so.25
#4  0x0000003a55ccbbb7 in WebKit::PluginInfoStore::loadPlugin ()
   from /lib64/libwebkit2gtk-3.0.so.25
#5  0x0000003a55ccc76b in WebKit::PluginInfoStore::loadPluginsIfNecessary ()
   from /lib64/libwebkit2gtk-3.0.so.25
#6  0x0000003a55cccb41 in WebKit::PluginInfoStore::plugins ()
   from /lib64/libwebkit2gtk-3.0.so.25
#7  0x0000003a55d28418 in WebKit::WebProcessProxy::getPlugins ()
   from /lib64/libwebkit2gtk-3.0.so.25
#8  0x0000003a55dddb0a in WebKit::WebProcessProxy::didReceiveSyncWebProcessProxyMessage
    () from /lib64/libwebkit2gtk-3.0.so.25
#9  0x0000003a55c1f9ec in CoreIPC::Connection::dispatchSyncMessage ()
   from /lib64/libwebkit2gtk-3.0.so.25
#10 0x0000003a55c1fb35 in CoreIPC::Connection::dispatchMessage ()
   from /lib64/libwebkit2gtk-3.0.so.25
#11 0x0000003a55c217b4 in CoreIPC::Connection::SyncMessageState::dispatchMessages ()
   from /lib64/libwebkit2gtk-3.0.so.25
#12 0x0000003a56c3f99d in WebCore::RunLoop::performWork ()
   from /lib64/libwebkit2gtk-3.0.so.25
#13 0x0000003a570ff039 in WebCore::RunLoop::queueWork ()
   from /lib64/libwebkit2gtk-3.0.so.25
#14 0x0000003a2e447f56 in g_main_dispatch (context=0x70b8d0) at gmain.c:3054
#15 g_main_context_dispatch (context=context@entry=0x70b8d0) at gmain.c:3630
#16 0x0000003a2e4482a8 in g_main_context_iterate (context=context@entry=0x70b8d0,
    block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
    at gmain.c:3701
#17 0x0000003a2e44834c in g_main_context_iteration (context=0x70b8d0,
    context@entry=0x0, may_block=may_block@entry=1) at gmain.c:3762
#18 0x0000003a30496564 in g_application_run (application=0x8cc060 [EphyShell],
    argc=argc@entry=1, argv=argv@entry=0x7fffffffdd48) at gapplication.c:1623
#19 0x000000000042ebbe in main (argc=1, argv=0x7fffffffdd48) at ephy-main.c:472

[Carlos Garcia Campos]

It should only happen once, unless plugins need to be refreshed. The thing is that GetPluginInfo message is sync, so there's little we can do I guess. Maybe we can get plugins info in a thread at startup to make sure plugins info are cached in the UI process asap. I wonder if other ports do something like that or GetPluginInfo is just faster.

[Martin Robinson]

(In reply to comment #1)
> It should only happen once, unless plugins need to be refreshed. The thing is that GetPluginInfo message is sync, so there's little we can do I guess. Maybe we can get plugins info in a thread at startup to make sure plugins info are cached in the UI process asap. I wonder if other ports do something like that or GetPluginInfo is just faster.

Another idea I had was to cache plugin information based on path, so that when you start the process again it doesn't block.

[Anders Carlsson]

(In reply to comment #2)
> (In reply to comment #1)
> > It should only happen once, unless plugins need to be refreshed. The thing is that GetPluginInfo message is sync, so there's little we can do I guess. Maybe we can get plugins info in a thread at startup to make sure plugins info are cached in the UI process asap. I wonder if other ports do something like that or GetPluginInfo is just faster.
> 
> Another idea I had was to cache plugin information based on path, so that when you start the process again it doesn't block.

I’d rather not do any caching in the web process since the UI process dictates whether plug-ins are blocked or not and that can change at any point.

[Martin Robinson]

(In reply to comment #3)

> I’d rather not do any caching in the web process since the UI process dictates whether plug-ins are blocked or not and that can change at any point.

We should be okay, since it looks like scanPlugin is running from the UI process here. I'd prefer to keep caching there as well.

[Carlos Garcia Campos]

(In reply to comment #2)
> (In reply to comment #1)
> > It should only happen once, unless plugins need to be refreshed. The thing is that GetPluginInfo message is sync, so there's little we can do I guess. Maybe we can get plugins info in a thread at startup to make sure plugins info are cached in the UI process asap. I wonder if other ports do something like that or GetPluginInfo is just faster.
> 
> Another idea I had was to cache plugin information based on path, so that when you start the process again it doesn't block.

The problem then is when do we update the cache? Maybe we could save also the md5sum of the plugin in the cache or something like that, so that when a new version of the plugin is installed we update the metadata.

[Carlos Garcia Campos]

Created attachment 225136 [details]
Patch

This patch uses a simple GKeyFile as persistent cache for the plugins metadata.

[Carlos Garcia Campos]

I'm using the mtime for now, to decide when to update the cache, if you think that's not enough we could also check the file size, or maybe use the md5sum.

[Carlos Garcia Campos]

Created attachment 225140 [details]
Try to fix mac build

[Carlos Garcia Campos]

Created attachment 225356 [details]
Patch

Updated patch to include a schema version so that if we add more metadata in the future, we don't use old files.

[Gustavo Noronha (kov)]

Comment on attachment 225356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225356&action=review

LGTM!

> Source/WebKit2/UIProcess/Plugins/gtk/PluginInfoCache.cpp:87
> +    g_file_set_contents(m_cachePath.get(), data.get(), dataLength, nullptr);

I guess we don't have to be concerned about multiple webkit clients trying to write to the file at the same time since this is atomic, right?

> Source/WebKit2/UIProcess/Plugins/gtk/PluginInfoCache.cpp:135
> +    // Save the cache file in an idle to make sure it happens in the main thread and
> +    // it's done only once when this is called multiple times in a very short time.
> +    if (m_saveToFileIdleId)
> +        return;

I assume this is called from a single thread, so we don't need to perform any locking of the m_saveToFileIdleId member.

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 225356 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225356&action=review
> 
> LGTM!

Thanks for the review.

> > Source/WebKit2/UIProcess/Plugins/gtk/PluginInfoCache.cpp:87
> > +    g_file_set_contents(m_cachePath.get(), data.get(), dataLength, nullptr);
> 
> I guess we don't have to be concerned about multiple webkit clients trying to write to the file at the same time since this is atomic, right?

This is only run in the main thread, because other threads "send" the task to the main thread using g_idle (the other threads that can run this use the same main context).

> > Source/WebKit2/UIProcess/Plugins/gtk/PluginInfoCache.cpp:135
> > +    // Save the cache file in an idle to make sure it happens in the main thread and
> > +    // it's done only once when this is called multiple times in a very short time.
> > +    if (m_saveToFileIdleId)
> > +        return;
> 
> I assume this is called from a single thread, so we don't need to perform any locking of the m_saveToFileIdleId member.

In this case we could probably use a mutex to be extra sure, I'll update the patch.

[Martin Robinson]

(In reply to comment #11)

> > I guess we don't have to be concerned about multiple webkit clients trying to write to the file at the same time since this is atomic, right?
> 
> This is only run in the main thread, because other threads "send" the task to the main thread using g_idle (the other threads that can run this use the same main context).

I think he's concerned about multiple applications running that are all using WebKit.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > > I guess we don't have to be concerned about multiple webkit clients trying to write to the file at the same time since this is atomic, right?
> > 
> > This is only run in the main thread, because other threads "send" the task to the main thread using g_idle (the other threads that can run this use the same main context).
> 
> I think he's concerned about multiple applications running that are all using WebKit.

Oh!, good point, we should make sure that the save is atomic, indeed.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > 
> > > > I guess we don't have to be concerned about multiple webkit clients trying to write to the file at the same time since this is atomic, right?
> > > 
> > > This is only run in the main thread, because other threads "send" the task to the main thread using g_idle (the other threads that can run this use the same main context).
> > 
> > I think he's concerned about multiple applications running that are all using WebKit.
> 
> Oh!, good point, we should make sure that the save is atomic, indeed.

g_file_set_contents is atomic.

[Carlos Garcia Campos]

Created attachment 225375 [details]
Updated patch using a mutex

[Carlos Garcia Campos]

Committed r164808: <http://trac.webkit.org/changeset/164808>