WKContextGetInfoForInstalledPlugIns() currently is exposed as asynchronous SPI, but it synchronously calls PluginInfoStore::plugins() internally, which can trigger synchronous loading of plug-ins. A corresponding asynchronous function should be introduced on PluginInfoStore and adopted by WKContextGetInfoForInstalledPlugIns().
<rdar://problem/22600972>
Created attachment 281690 [details] Patch
Whoops... thanks EWS!
Created attachment 281693 [details] Patch
Comment on attachment 281693 [details] Patch Attachment 281693 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1539342 Number of test failures exceeded the failure limit.
Created attachment 281700 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 281693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281693&action=review > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:124 > + BinarySemaphore semaphore; I believe the preferred primitive is WTF::Condition these days. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:66 > + void getInstalledPlugins(void(^)(Vector<PluginModuleInfo>)); This should take a std::function<void (Vector<PluginModuleInfo>)>. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:77 > + // Returns the info for the plug-in with the given path. This is non-const only because it takes a lock to read the plug-ins. You don't need the non-const comment, if you want to keep this const, you could make the lock explicitly mutable. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:86 > + // These are non-const only because they take a lock to read the plug-ins. You don't need the non-const comment, if you want to keep this const, you could make the lock explicitly mutable.
Comment on attachment 281693 [details] Patch mac-wk2 EWS failures are somewhat surprising, but chances are that they are actually introduced by the patch.
(In reply to comment #7) > Comment on attachment 281693 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281693&action=review > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:124 > > + BinarySemaphore semaphore; > > I believe the preferred primitive is WTF::Condition these days. Changed. > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:66 > > + void getInstalledPlugins(void(^)(Vector<PluginModuleInfo>)); > > This should take a std::function<void (Vector<PluginModuleInfo>)>. Changed. > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:77 > > + // Returns the info for the plug-in with the given path. This is non-const only because it takes a lock to read the plug-ins. > > You don't need the non-const comment, if you want to keep this const, you > could make the lock explicitly mutable. > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:86 > > + // These are non-const only because they take a lock to read the plug-ins. > > You don't need the non-const comment, if you want to keep this const, you > could make the lock explicitly mutable. Ah, yes, that's much better. Changed. Will also run the tests locally and try to figure out what happened with mac-wk2.
Created attachment 281776 [details] Patch
Comment on attachment 281776 [details] Patch Retracting this patch for the time being for a bit more investigation.
Created attachment 281850 [details] Patch
Comment on attachment 281850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281850&action=review > Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.mm:100 > + dispatch_async(dispatch_get_main_queue(), ^() { > + blockCopy(toAPI(array.get()), 0); > + Block_release(blockCopy); > + toImpl(contextRef)->deref(); This should use a lambda and BlockPtr. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:55 > + m_queue->dispatch([this, directories]() { > + m_additionalPluginsDirectories = directories; > + }); No need for () in the lambda expression. This isn't thread safe - directories can't be copied since that'll ref/deref the Strings. How does this ensure that the plug-in info store isn't destroyed before the lambda runs? > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:64 > + m_queue->dispatch([this]() { > + m_pluginListIsUpToDate = false; > + }); How does this ensure that the plug-in info store isn't destroyed before the lambda runs? > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:93 > + std::lock_guard<Lock> lock(m_pluginsMutex); Why does this need a lock? > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:104 > + m_client->pluginInfoStoreDidLoadPlugins(this); How does this ensure that the plug-in info store isn't destroyed before the lambda runs? > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:131 > + Lock waitingLock; > + Condition pluginsLoaded; > + LockHolder locker(waitingLock); > + m_queue->dispatch([this, &waitingLock, &pluginsLoaded]() { > + LockHolder locker(waitingLock); > + loadPluginsIfNecessary(); > + pluginsLoaded.notifyOne(); > + }); > + pluginsLoaded.wait(waitingLock); I think Sam was wrong when he suggested you use a condition + lock here. This doesn't handle spurious wakes - please change it back to use a binary semaphore. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:137 > +void PluginInfoStore::getInstalledPlugins(std::function<void(Vector<PluginModuleInfo>)> completionHandler) This should use a WTF::Function. Also, put a space after void. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:143 > + m_queue->dispatch([this, completionHandler]() { > + loadPluginsIfNecessary(); > + std::lock_guard<Lock> lock(m_pluginsMutex); > + completionHandler(m_plugins); > + }); How does this ensure that the plug-in info store isn't destroyed before the lambda runs? > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:109 > + mutable Lock m_pluginsMutex; Not sure what this is used for.
(In reply to comment #13) > Comment on attachment 281850 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281850&action=review > > > Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.mm:100 > > + dispatch_async(dispatch_get_main_queue(), ^() { > > + blockCopy(toAPI(array.get()), 0); > > + Block_release(blockCopy); > > + toImpl(contextRef)->deref(); > > This should use a lambda and BlockPtr. Will change. > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:55 > > + m_queue->dispatch([this, directories]() { > > + m_additionalPluginsDirectories = directories; > > + }); > > No need for () in the lambda expression. > This isn't thread safe - directories can't be copied since that'll ref/deref > the Strings. Didn't know this. Will fix. > How does this ensure that the plug-in info store isn't destroyed before the > lambda runs? I'll protect PluginInfoStore for the lambda here and elsewhere. > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:93 > > + std::lock_guard<Lock> lock(m_pluginsMutex); > > Why does this need a lock? m_plugins is the only member accessed from multiple queues, so it needs a lock to mediate access. > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:131 > > + Lock waitingLock; > > + Condition pluginsLoaded; > > + LockHolder locker(waitingLock); > > + m_queue->dispatch([this, &waitingLock, &pluginsLoaded]() { > > + LockHolder locker(waitingLock); > > + loadPluginsIfNecessary(); > > + pluginsLoaded.notifyOne(); > > + }); > > + pluginsLoaded.wait(waitingLock); > > I think Sam was wrong when he suggested you use a condition + lock here. > This doesn't handle spurious wakes - please change it back to use a binary > semaphore. Done. > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:137 > > +void PluginInfoStore::getInstalledPlugins(std::function<void(Vector<PluginModuleInfo>)> completionHandler) > > This should use a WTF::Function. Also, put a space after void. Will fix. > How does this ensure that the plug-in info store isn't destroyed before the > lambda runs? > > > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:109 > > + mutable Lock m_pluginsMutex; > > Not sure what this is used for. See above comment. Revised patch coming.
Created attachment 283231 [details] Patch
Comment on attachment 283231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283231&action=review Not an expert enough on locking for. full review but I have a few small comments. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:63 > + m_queue->dispatch([this, protectedThis = Ref<PluginInfoStore>(*this), directoriesCopy = WTFMove(directoriesCopy)]() mutable { Should use the new makeRef function now that it has landed. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:72 > + m_queue->dispatch([this, protectedThis = Ref<PluginInfoStore>(*this)] { Should use the new makeRef function now that it has landed. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:94 > + for (size_t i = 0; i < m_additionalPluginsDirectories->size(); ++i) > + addFromVector(uniquePluginPaths, pluginPathsInDirectory(m_additionalPluginsDirectories->at(i))); How about writing this? for (auto& directory : *m_additionalPluginsDirectories) addFromVector(uniquePluginPaths, pluginPathsInDirectory(directory)); > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:114 > + callOnMainThread([this, protectedThis = Ref<PluginInfoStore>(*this)] { Should use the new makeRef function now that it has landed. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:136 > + m_queue->dispatch([this, protectedThis = Ref<PluginInfoStore>(*this), &semaphore] { Should use the new makeRef function now that it has landed. > Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:148 > + m_queue->dispatch([this, protectedThis = Ref<PluginInfoStore>(*this), completionHandler = WTFMove(completionHandler)] { Should use the new makeRef function now that it has landed.
Anders, Sam, could one of you review this? I’d love to see Conrad able to land this before the next branch Apple makes.
Comment on attachment 283231 [details] Patch r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.