Summary: | [GTK][WK2] Blocks when fetching plugins information | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, berto, bugzilla, bunhere, cdumez, cgarcia, commit-queue, csaavedra, gustavo, gyuyoung.kim, mrobinson, rakuco, sergio, svillar, william.jon.mccann, zan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Claudio Saavedra
2013-05-06 09:00:12 PDT
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. (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. (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. (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. (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. Created attachment 225136 [details]
Patch
This patch uses a simple GKeyFile as persistent cache for the plugins metadata.
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. Created attachment 225140 [details]
Try to fix mac build
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.
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. (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. (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. (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. (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. Created attachment 225375 [details]
Updated patch using a mutex
Committed r164808: <http://trac.webkit.org/changeset/164808> |