Store plug-in enabled/disabled state in PluginPrefs instead of WebPluginInfo.

Store plug-in enabled/disabled state in PluginPrefs instead of WebPluginInfo, to allow different sets of enabled/disabled plug-ins to be specified per profile.


BUG=80794
TEST=Open two profiles, disable different plugins in them.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101269

[Bernhard Bauer, 2011-09-12 17:39:52]

A'ight, this is The Big One. Please review :)

John: content/ and webkit/plugins
Joao: PluginPrefs, esp. with regards to policies
Carlos: component updater
Miranda: FYI

Thanks!

[cpu_(ooo_6.6-7.5), 2011-09-13 01:12:04]

+trung

[jam, 2011-09-13 01:47:30]

content/webkit lgtm. i only glanced at some of the other code

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:122: plugin_prefs->EnablePluginGroup(
wouldn't these two calls assert since they're not being made on the file thread?

http://codereview.chromium.org/7848025/diff/9001/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:332: DCHECK(group.get());
nit: this dcheck is not necessary. if it's null, we'll get a crash dump from
below. it'll also be as good of a signal for developers using debug builds

[Joao da Silva, 2011-09-13 09:30:47]

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:12: #include
"chrome/browser/chrome_plugin_service_filter.h"
Nit: this line is out of place, please fix

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs.cc
File chrome/browser/plugin_prefs.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:176: base::AutoLock auto_lock(lock_);
The lock is acquired when reading here, but isn't acquired when
|policy_*_patterns_| is written below.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs.h
File chrome/browser/plugin_prefs.h (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:43: // thread.
Please update this comment.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:90: const NotificationDetails& details);
OVERRIDE

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs_...
File chrome/browser/plugin_prefs_unittest.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs_...
chrome/browser/plugin_prefs_unittest.cc:12: virtual void SetUp() {
OVERRIDE

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/ui/webui/plug...
File chrome/browser/ui/webui/plugins_ui.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/ui/webui/plug...
chrome/browser/ui/webui/plugins_ui.cc:407: // TODO(bauerb): Remove
|enabled_controls| & co.
Nit: |enable_controls|

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_group.cc (right):

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_group.cc:245: bool PluginGroup::IsVulnerable(const
WebPluginInfo& plugin) const {
It was decided that force-enabled plugins shouldn't display these warnings
(http://crbug.com/80737). I suggest modifying
ChromeViewHostMsg_GetPluginPolicies to either

1. also retrieve the PolicyStatus for the plugin; if POLICY_ENABLED, don't block
it

or 2. just ask what to do about the plugin, and get back a decision from the
browser (ALLOW, BLOCK or ASK). In this case the renderer doesn't get to know
anything about outdated_policy or authorize_policy; it just passes the plugin
name to the browser and the browser does these checks.

wdyt?

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_group.h (right):

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_group.h:9: #include <map>
Not used

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_group.h:10: #include <set>
Not used anymore

[Bernhard Bauer, 2011-09-13 12:13:00]

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:12: #include
"chrome/browser/chrome_plugin_service_filter.h"
On 2011/09/13 09:30:48, Joao da Silva wrote:
> Nit: this line is out of place, please fix

Done.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:122: plugin_prefs->EnablePluginGroup(
On 2011/09/13 01:47:31, John Abd-El-Malek wrote:
> wouldn't these two calls assert since they're not being made on the file
thread?

Indeed! Thanks for catching that. I changed PluginPrefs::EnablePluginGroup to
re-post itself on the right thread.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs.cc
File chrome/browser/plugin_prefs.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:176: base::AutoLock auto_lock(lock_);
On 2011/09/13 09:30:48, Joao da Silva wrote:
> The lock is acquired when reading here, but isn't acquired when
> |policy_*_patterns_| is written below.

Added a lock in |Observe|.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs.h
File chrome/browser/plugin_prefs.h (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:43: // thread.
On 2011/09/13 09:30:48, Joao da Silva wrote:
> Please update this comment.

Done.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:90: const NotificationDetails& details);
On 2011/09/13 09:30:48, Joao da Silva wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs_...
File chrome/browser/plugin_prefs_unittest.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/plugin_prefs_...
chrome/browser/plugin_prefs_unittest.cc:12: virtual void SetUp() {
On 2011/09/13 09:30:48, Joao da Silva wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/ui/webui/plug...
File chrome/browser/ui/webui/plugins_ui.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/ui/webui/plug...
chrome/browser/ui/webui/plugins_ui.cc:407: // TODO(bauerb): Remove
|enabled_controls| & co.
On 2011/09/13 09:30:48, Joao da Silva wrote:
> Nit: |enable_controls|

Done.

http://codereview.chromium.org/7848025/diff/9001/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:332: DCHECK(group.get());
On 2011/09/13 01:47:31, John Abd-El-Malek wrote:
> nit: this dcheck is not necessary. if it's null, we'll get a crash dump from
> below. it'll also be as good of a signal for developers using debug builds

Done.

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_group.cc (right):

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_group.cc:245: bool PluginGroup::IsVulnerable(const
WebPluginInfo& plugin) const {
On 2011/09/13 09:30:48, Joao da Silva wrote:
> It was decided that force-enabled plugins shouldn't display these warnings
> (http://crbug.com/80737). I suggest modifying
> ChromeViewHostMsg_GetPluginPolicies to either
> 
> 1. also retrieve the PolicyStatus for the plugin; if POLICY_ENABLED, don't
block
> it
> 
> or 2. just ask what to do about the plugin, and get back a decision from the
> browser (ALLOW, BLOCK or ASK). In this case the renderer doesn't get to know
> anything about outdated_policy or authorize_policy; it just passes the plugin
> name to the browser and the browser does these checks.

I agree; there are more possible actions though (show infobars, allow loading
the plug-in or not, allow click-to-play or not, etc). I'll have to think about
what we want to do here exactly (and do it in a follow-up CL).

> wdyt?

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_group.h (right):

http://codereview.chromium.org/7848025/diff/9001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_group.h:9: #include <map>
On 2011/09/13 09:30:48, Joao da Silva wrote:
> Not used

Removed <map> and <set>.

[Joao da Silva, 2011-09-13 13:18:10]

LGTM regarding policy. Let's fix the whitelist/outdated/dangerous plugins
interaction once this is in.

[Pam (message me for reviews), 2011-09-13 13:28:27]

Drive-by review: Please add a clause or two to the CL description explaining why
this change is happening ("...to allow it to be specified on a per-profile
basis.").

[jam, 2011-09-13 15:58:13]

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/7848025/diff/9001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:122: plugin_prefs->EnablePluginGroup(
On 2011/09/13 12:13:01, Bernhard Bauer wrote:
> On 2011/09/13 01:47:31, John Abd-El-Malek wrote:
> > wouldn't these two calls assert since they're not being made on the file
> thread?
> 
> Indeed! Thanks for catching that. I changed PluginPrefs::EnablePluginGroup to
> re-post itself on the right thread.

cool. i was just going to comment that it would be nice if PluginPrefs, as much
as possible, has its methods called on the UI threads (or any thread).
internally it can post to different threads. this makes it much easier for the
callers.

[cpu_(ooo_6.6-7.5), 2011-09-13 22:20:51]

Flapper is experimental and highly prone to breakage, that is when chrome
upgrades the previously working flapper now finds itself incompatible (because
the pepper api is in flux) that is why it is always registered disabled.

So say you enable it and close the browser, upon restart is disabled again.

Does that property hold with this change?

Second, the component updater only registers the flapper with the highest
version, the old versions will be removed (currently still there). Is that going
to cause a problem?

[Bernhard Bauer, 2011-09-14 16:53:46]

On 2011/09/13 22:20:51, cpu wrote:
> Flapper is experimental and highly prone to breakage, that is when chrome
> upgrades the previously working flapper now finds itself incompatible (because
> the pepper api is in flux) that is why it is always registered disabled.
> 
> So say you enable it and close the browser, upon restart is disabled again.
> 
> Does that property hold with this change?

I see. I changed it so that Flapper is disabled again on every browser restart.

> Second, the component updater only registers the flapper with the highest
> version, the old versions will be removed (currently still there). Is that
going
> to cause a problem?

I don't think so. We can store information about plug-ins which aren't in the
PluginList (because the PluginList can be (re)loaded at a later time), but we
don't write it out to disk, so it'll be gone on the next restart.

[cpu_(ooo_6.6-7.5), 2011-09-15 06:27:48]

lgtm