Bug 188764 – WKNavigationDelegate needs to allow clients to specify a custom blocked plug-in message

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

[Jeff Miller]

In https://bugs.webkit.org/show_bug.cgi?id=181073, WebKit added the following WKNavigationDelegate SPI:

- (_WKPluginModuleLoadPolicy)_webView:(WKWebView *)webView decidePolicyForPluginLoadWithCurrentPolicy:(_WKPluginModuleLoadPolicy)policy pluginInfo:(NSDictionary *)info unavailabilityDescription:(NSString *)unavailabilityDescription API_AVAILABLE(macosx(10.14));

One intent of this SPI was to allow clients to return a custom unavailabilityDescription, but they can't do that currently because this parameter is an NSString * instead of an NSString **.

[Radar WebKit Bug Importer]

<rdar://problem/43534382>

[Jeff Miller]

The original bug was actually https://bugs.webkit.org/show_bug.cgi?id=181413.

[Jeff Miller]

Created attachment 347586 [details]
Patch

[Alex Christensen]

Comment on attachment 347586 [details]
Patch

Give it a CompletionHandler that has two parameters.

[Jeff Miller]

That makes much more sense than blindly following the old SPI. Will do.

[Jeff Miller]

I'm not sure how to implement your suggestion easily. WebPage::createPlugin() in the web process sends a synchronous IPC message that's handled by WebPageProxy::findPlugin() in the UI process, which ends up calling the navigation delegate synchronously.

If we wanted to make the navigation delegate method asynchronous with a completion handler, I think this would require rearchitecting how plug-ins are created in the web process.

[Jeff Miller]

I followed the chain back from WebPage::createPlugin() a bit more in the WebProcess, and the fact that plugin creation is synchronous seems pretty integral. I don't think I should attempt to change this.

In theory, I guess we could still use a CompletionHandler with the WKNavigationDelegate method, with an implied contract that the client has to call the completion handler before returning from the method so WebPageProxy::findPlugin() can return synchronously, but that seems like a clear antipattern to me.

I'm going to reupload my original patch for review.

[Jeff Miller]

Created attachment 347651 [details]
Patch

[Alex Christensen]

Comment on attachment 347651 [details]
Patch

If you add "Delayed" after FindPlugin(...)->(...) in WebPageProxy.messages.in you should be able to accomplish this the right way.  This way causes problems with arc.

[Jeff Miller]

That's the bit I was missing, thanks. I have a working patch.

[Jeff Miller]

Created attachment 347713 [details]
Patch

[Alex Christensen]

Comment on attachment 347713 [details]
Patch

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

Looks much better!

> Source/WebKit/UIProcess/API/APINavigationClient.h:120
> +    virtual void decidePolicyForPluginLoad(WebKit::WebPageProxy&, WebKit::PluginModuleLoadPolicy currentPluginLoadPolicy, Dictionary&, WTF::Function<void(WebKit::PluginModuleLoadPolicy, const WTF::String&)>&& completionHandler)

WTF::Function -> CompletionHandler here and everywhere.

[Jeff Miller]

Created attachment 347726 [details]
Patch

[Alex Christensen]

Comment on attachment 347726 [details]
Patch

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

> Source/WebKit/UIProcess/API/APINavigationClient.h:122
> +        completionHandler(currentPluginLoadPolicy, WTF::String());

{ } here and elsewhere where we create an empty String.  The type isn't important.  The fact that we have nothing is all we need to represent in the code.

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:374
> +            completionHandler(pluginModuleLoadPolicy(policy), String(unavailabilityDescription));

String has a non-explicit NSString* constructor, so you shouldn't need the "String(" and ")"

> Source/WebKit/UIProcess/WebPageProxy.cpp:2168
> +    auto findPluginCompletion = [processType, &reply, &newMimeType, &plugin] (uint32_t pluginLoadPolicy, const String& unavailabilityDescription) {

reply = WTFMove(reply), newMimeType = WTFMove(newMimeType), plugin = WTFMove(plugin)
It is unsafe to capture pointers to stack variables in a lambda that might be called after this stack frame returns.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2190
> +        m_navigationClient->decidePolicyForPluginLoad(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), [findPluginCompletion = WTFMove(findPluginCompletion)] (PluginModuleLoadPolicy pluginLoadPolicy, const String& unavailabilityDescription) mutable {

You should just be able to WTFMove(findPluginCompletion) into the function call as a parameter instead of creating a lambda does nothing but call it.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2195
>          pluginLoadPolicy = m_loaderClient->pluginLoadPolicy(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), unavailabilityDescription);

It would probably be cleaner if you passed the CompletionHandler to the loader client, too.

[Jeff Miller]

Comment on attachment 347726 [details]
Patch

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

>> Source/WebKit/UIProcess/API/APINavigationClient.h:122
>> +        completionHandler(currentPluginLoadPolicy, WTF::String());
> 
> { } here and elsewhere where we create an empty String.  The type isn't important.  The fact that we have nothing is all we need to represent in the code.

Done.

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:374
>> +            completionHandler(pluginModuleLoadPolicy(policy), String(unavailabilityDescription));
> 
> String has a non-explicit NSString* constructor, so you shouldn't need the "String(" and ")"

I confirmed that works, done.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:2168
>> +    auto findPluginCompletion = [processType, &reply, &newMimeType, &plugin] (uint32_t pluginLoadPolicy, const String& unavailabilityDescription) {
> 
> reply = WTFMove(reply), newMimeType = WTFMove(newMimeType), plugin = WTFMove(plugin)
> It is unsafe to capture pointers to stack variables in a lambda that might be called after this stack frame returns.

Fixed. I had to move the initialization of pluginInformation before the lambda, since it depends on plugin.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:2190
>> +        m_navigationClient->decidePolicyForPluginLoad(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), [findPluginCompletion = WTFMove(findPluginCompletion)] (PluginModuleLoadPolicy pluginLoadPolicy, const String& unavailabilityDescription) mutable {
> 
> You should just be able to WTFMove(findPluginCompletion) into the function call as a parameter instead of creating a lambda does nothing but call it.

Nice optimization, fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:2195
>>          pluginLoadPolicy = m_loaderClient->pluginLoadPolicy(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), unavailabilityDescription);
> 
> It would probably be cleaner if you passed the CompletionHandler to the loader client, too.

Fixed.

[Jeff Miller]

Created attachment 347816 [details]
Patch

[Alex Christensen]

Comment on attachment 347816 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366
> +        completionHandler(currentPluginLoadPolicy, String());

{ }

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:373
> +        [(id <WKNavigationDelegatePrivate>)navigationDelegate _webView:m_navigationState.m_webView decidePolicyForPluginLoadWithCurrentPolicy:wkPluginModuleLoadPolicy(currentPluginLoadPolicy) pluginInfo:wrapper(pluginInformation) completionHandler:^(_WKPluginModuleLoadPolicy policy, NSString *unavailabilityDescription) {

I'm surprised this compiles without using BlockPtr because CompletionHandler is not copyable and blocks are.

[Jeff Miller]

Comment on attachment 347816 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366
>> +        completionHandler(currentPluginLoadPolicy, String());
> 
> { }

Oops, missed this. Fixed.

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:373
>> +        [(id <WKNavigationDelegatePrivate>)navigationDelegate _webView:m_navigationState.m_webView decidePolicyForPluginLoadWithCurrentPolicy:wkPluginModuleLoadPolicy(currentPluginLoadPolicy) pluginInfo:wrapper(pluginInformation) completionHandler:^(_WKPluginModuleLoadPolicy policy, NSString *unavailabilityDescription) {
> 
> I'm surprised this compiles without using BlockPtr because CompletionHandler is not copyable and blocks are.

Interesting. I'll add this.

It also occurred to me that I should use a CompletionHandlerCallChecker here as well.

[Jeff Miller]

https://trac.webkit.org/changeset/235200/webkit

[Chris Dumez]

Comment on attachment 347816 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366
>>> +        completionHandler(currentPluginLoadPolicy, String());
>> 
>> { }
> 
> Oops, missed this. Fixed.

This is missing a return statement..

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:370
> +        completionHandler(currentPluginLoadPolicy, { });

ditto..

-> https://bugs.webkit.org/show_bug.cgi?id=191753