Bug 134357

Summary: [GTK] Add API to allow blocking plugins
Product: WebKit Reporter: Jiří Janoušek <janousek.jiri>
Component: WebKit GtkAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: agomez, berto, bugs-noreply, cgarcia, chavarria1991, clopez, commit-queue, gns, mrobinson, pnormand, svillar
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jiří Janoušek 2014-06-26 12:56:52 PDT
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.
Comment 1 Jiří Janoušek 2014-10-20 13:40:17 PDT
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.
Comment 2 Carlos Alberto Lopez Perez 2014-12-01 04:23:03 PST
(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.
Comment 3 Martin Robinson 2014-12-01 04:36:59 PST
(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.
Comment 4 Marcos Chavarría Teijeiro (irc: chavaone) 2014-12-10 00:38:43 PST
Created attachment 242994 [details]
Patch
Comment 5 WebKit Commit Bot 2014-12-10 00:40:00 PST
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
Comment 6 WebKit Commit Bot 2014-12-10 00:40:10 PST
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.
Comment 7 Carlos Garcia Campos 2014-12-10 00:56:44 PST
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
Comment 8 Marcos Chavarría Teijeiro (irc: chavaone) 2014-12-12 02:27:45 PST
Created attachment 243189 [details]
Patch
Comment 9 Carlos Garcia Campos 2014-12-12 05:51:42 PST
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
Comment 10 Carlos Garcia Campos 2014-12-12 05:56:16 PST
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.
Comment 11 Marcos Chavarría Teijeiro (irc: chavaone) 2015-03-06 02:42:06 PST
Created attachment 248056 [details]
Patch
Comment 12 Marcos Chavarría Teijeiro (irc: chavaone) 2015-03-06 03:25:23 PST
Created attachment 248058 [details]
Patch
Comment 13 Sergio Villar Senin 2015-03-17 01:50:28 PDT
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.
Comment 14 Marcos Chavarría Teijeiro (irc: chavaone) 2015-03-19 02:56:30 PDT
I have just send an email to webkitgtk+ mailing list proposing a API to implement selective disable plugins.
Comment 15 Marcos Chavarría Teijeiro (irc: chavaone) 2015-04-09 00:38:40 PDT
Created attachment 250424 [details]
Patch
Comment 16 Marcos Chavarría Teijeiro (irc: chavaone) 2015-04-09 02:09:14 PDT
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.
Comment 17 Marcos Chavarría Teijeiro (irc: chavaone) 2015-04-10 04:25:20 PDT
Created attachment 250508 [details]
Patch
Comment 18 Marcos Chavarría Teijeiro (irc: chavaone) 2015-04-10 04:29:13 PDT
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.