Bug 134357 – [GTK] Add API to allow blocking plugins

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKitGTK

[Jiří Janoušek]

Hi,

the documentation of WebKitPlugin class[1] states "This object can be used to get more information about a plugin, and enable/disable it, allowing fine-grained control of plugins.", but I can't see any function to disable a particular plugin. I would use such a function to disable all plugins except a preferred Flash plugin implementation (Adobe Flash, Gnash, Lightspark, Pipelight, ...) as requested by users of my app (Nuvola Player).

[1] http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitPlugin.html

Thank you.

[Jiří Janoušek]

When both the NPAPI Adobe Flash plugin 11.2 and the Fresh-player-wrapped PPAPI Adobe Flash plugin 15.0 are installed, WebKit2Gtk uses the old 11.2 Flash plugin and ignores the newer version. A method webkit_plugin_set_enabled () would be really helpful to let user disable the old Flash plugin and make sure the new one is used.

[Carlos Alberto Lopez Perez]

(In reply to comment #1)
> When both the NPAPI Adobe Flash plugin 11.2 and the Fresh-player-wrapped
> PPAPI Adobe Flash plugin 15.0 are installed, WebKit2Gtk uses the old 11.2
> Flash plugin and ignores the newer version. A method
> webkit_plugin_set_enabled () would be really helpful to let user disable the
> old Flash plugin and make sure the new one is used.

Please note that WebKit don't supports PPAPI plugins.

[Martin Robinson]

(In reply to comment #2)
> (In reply to comment #1)
> > When both the NPAPI Adobe Flash plugin 11.2 and the Fresh-player-wrapped
> > PPAPI Adobe Flash plugin 15.0 are installed, WebKit2Gtk uses the old 11.2
> > Flash plugin and ignores the newer version. A method
> > webkit_plugin_set_enabled () would be really helpful to let user disable the
> > old Flash plugin and make sure the new one is used.
> 
> Please note that WebKit don't supports PPAPI plugins.

Fresh-player appears to be an NPAPI wrapper for PPAPI plugins: https://github.com/i-rinat/freshplayerplugin.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 242994 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 242994 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitPlugin.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPlugin.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPlugin.cpp:180:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:125:  all_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:125:  loaded_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:142:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:154:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 242994 [details]
Patch

I don't see how this can work. To have a WebKitPlugin object, you need to the plugin to be loaded, since it's the way to have a PluginModuleInfo, so it's imposible to disable a plugin before being loaded using WebKitPlugin API. What we want is not to disable a plugin load (in most of the cases the plugin cache will be used), but to block those plugin instances so that the plugin process is not even spawned. So, maybe webkit_plugin_block/unblock or set_blocked is_blocked would be better names. Please, provide a unit test to check things are working as expected. You should implement pluginLoadPolicy loader client callback and return PluginModuleBlocked, for the blocked plugins and PluginModuleLoadNormally for the others

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 243189 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 243189 [details]
Patch

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

After thinking about this again, I think we could expose this in a different way, giving more control to the application. We could expose the pluginLoadPolicy callback as a signal of WebKitWebView, where the user can connect to decide what to do about the plugin. And we should also expose the unavailablePluginButtonClicked so that the application can ask the user whether to enable the particular plugin or not and reload the page. This way, it's the application the one keeping the list of blocking plugins, and we keep the WebKitPlugin object as read-only.

> Source/WebKit2/Shared/Plugins/PluginModuleInfo.h:62
> +    bool isBlocked;

I don't think we need to keep this here. This is for the information that the plugin provides, not for the plugin state.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:123
> +    bool blocked = toImpl(pluginInfoDictionary)->get<API::Boolean>(pluginInformationIsBlockedKey())->value();
> +    return blocked ? PluginModuleBlocked : PluginModuleLoadNormally;

We should provide the unavailabilityDescription, I guess. I think we should keep the list of plugins blocked in GTK specific code, and check here if the given plugin is blocked or not.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:168
>          0, // didLayout
>          0, // pluginLoadPolicy_deprecatedForUseWithV2
>          0, // pluginDidFail
> -        0, // pluginLoadPolicy
> +        pluginLoadPolicy // pluginLoadPolicy

What happens when a plugin is blocked and the user clicks on the unavailable button? I think we should also implement unavailablePluginButtonClicked in the UI client.

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:71
> +#if PLATFORM(GTK)
> +        if (PluginInfoStore::m_loadedPluginListIsUpToDate)
> +#endif

I don't understand why this is needed. The pluginLoadPolicy will be called every time the page is reloaded and there are plugins.

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:96
> +#if PLATFORM(GTK)
> +    PluginInfoStore::m_loadedPluginListIsUpToDate = true;
> +#endif

Ditto.

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:65
> +#if PLATFORM(GTK)
> +    static void setBlockedPlugin(PluginModuleInfo&, bool);
> +#endif

This could be handled in the GTK+ specific code.

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.h:110
> +#if PLATFORM(GTK)
> +    static Vector<String> m_blockedPluginsPaths;
> +    static bool m_loadedPluginListIsUpToDate;
> +#endif

Ditto.

> Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:107
> +    for (const auto& blocked_plugin_path : m_blockedPluginsPaths) {

blocked_plugin_path -> blockedPluginPath

[Carlos Garcia Campos]

We should also consider exposing didBlockInsecurePluginVersion for plugins that don't have a representation in the web page, for example flash playing audio only, so that the app knows there's a plugin blocked and can ask the user.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 248056 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 248058 [details]
Patch

[Sergio Villar Senin]

I think we could use this bug to provide an API to selectively disable plugins even per host based blocking specially after https://bugs.webkit.org/show_bug.cgi?id=142506 has landed.

[Marcos Chavarría Teijeiro (irc: chavaone)]

I have just send an email to webkitgtk+ mailing list proposing a API to implement selective disable plugins.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 250424 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

I have beed working last weeks on modifying my patch to support declarative set of plugin load policies.

In spite of the fact that it do work know, this is a WIP still but any review/idea are really wellcome.

Appart from the previous created signals load-plugin, blocked-plugin and unavailable-plugin-activated, I have implement two methods to set and clear plugin load policies in a declarative way. So, webkit_plugin_set_client_load_policy can be used for set the load policy for a certain host and plugin and webkit_context_clear_policies could be used for clear all established policies. We have to take into account that there is no persistence at all in this policies so when we restart our application, the previous established policies will be gone. Thats why the application should store a list of policies established for plugins and the API does not provide a webkit_plugin_get_client_load_policy.

Both implemented methods expose the WebProccessPool methods created by r181562 (http://trac.webkit.org/changeset/181562). These methods update a local hashmap that contains all created policies and then they send a message to the WebProcess which also updates a local hashmap.

The problem is that these methods use BundleId and Version to identify each plugin and we dont have that in GTK+ so, I have use the filename instead of the bundleId and I have hardcoded the versionId to "*".

On the other hand the solution to get the established plugin policy for a certain plugin and host in load-plugin is a litle hacky... I have implement a method getPluginLoadClientPolicy in WebProcessPool. This method gets the established policy for a plugin finding the key in the WebProccessPool local array. Mac port seems to be doing this using other ways but I have not found any alternative.

The plugin information provided to load-plugin is a dictionary created using a PluginModuleInfo data obtained from PluginInfoStore. In GTK we use a cache to store the PluginModuleInfo for each plugin and this cache is only updated when the file changed.

In addition the created test has a race condition so It it timeouts if we dont have the wait instruction on line 900 of the TestWebKitWebView.cpp file.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 250508 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

After the email send by Carlos Garcia to the mailing list, we have conclude that block plugins and disable plugins are different things and they should be implemented on different bugs. So I re-upload the work I had done about blocking plugins with some tiny fixes. Any review would be really appreciated.

[Carlos Garcia Campos]

Plugins are no longer supported.