Bug 158961 – WKContextGetInfoForInstalledPlugIns() should be asynchronous under the hood

• Component: WebKit API
• Status: ASSIGNED

[Conrad Shultz]

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().

[Conrad Shultz]

<rdar://problem/22600972>

[Conrad Shultz]

Created attachment 281690 [details]
Patch

[Conrad Shultz]

Whoops... thanks EWS!

[Conrad Shultz]

Created attachment 281693 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Sam Weinig]

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.

[Alexey Proskuryakov]

Comment on attachment 281693 [details]
Patch

mac-wk2 EWS failures are somewhat surprising, but chances are that they are actually introduced by the patch.

[Conrad Shultz]

(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.

[Conrad Shultz]

Created attachment 281776 [details]
Patch

[Conrad Shultz]

Comment on attachment 281776 [details]
Patch

Retracting this patch for the time being for a bit more investigation.

[Conrad Shultz]

Created attachment 281850 [details]
Patch

[Anders Carlsson]

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.

[Conrad Shultz]

(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.

[Conrad Shultz]

Created attachment 283231 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Anders, Sam, could one of you review this? I’d love to see Conrad able to land this before the next branch Apple makes.

[Brady Eidson]

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.