Disable outdated plugins, block non-sandboxed plugins.

Add command line flags to disable outdated plugins and block non-sandboxed plugins.

BUG=47730, 47731
TEST=Run Chrome with --disable-outdated-plugins --block-nonsandboxed-plugins

[Bernhard Bauer, 2010-07-14 13:28:56]

Could you review this while I wait for the final LGTM on the click-to-play CL?
I'm trying to increase CL throughput by pipelining ;-)

[Pam (message me for reviews), 2010-07-15 16:45:09]

This looks OK to me apart from the minor stuff below, but I'm not an expert in
this stuff. Please have someone more familiar with plugins take a look too.
John, would you, or could you suggest someone?

- Pam

http://codereview.chromium.org/2967007/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2967007/diff/2001/3001#newcode3728
chrome/app/generated_resources.grd:3728: Update plugin
plug-in

http://codereview.chromium.org/2967007/diff/2001/3002
File chrome/browser/blocked_plugin_manager.cc (right):

http://codereview.chromium.org/2967007/diff/2001/3002#newcode42
chrome/browser/blocked_plugin_manager.cc:42: // TODO
Please expand this TODO by adding your name and when it will be ready to be done
(if applicable). For example:

// TODO(bauerb): Implement this once the design is settled.

http://codereview.chromium.org/2967007/diff/2001/3004
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/2967007/diff/2001/3004#newcode67
chrome/browser/plugin_service.cc:67: // Disable outdated plugins.
This isn't a very useful comment. Please either describe what's happening in
more detail or just take it out.

http://codereview.chromium.org/2967007/diff/2001/3005
File chrome/browser/plugin_updater.cc (right):

http://codereview.chromium.org/2967007/diff/2001/3005#newcode219
chrome/browser/plugin_updater.cc:219: switches::kDisableOutdatedPlugins) ||
!(*it)->IsVulnerable()) {
Continuation line should have 4-space indent. But for clarity, I'd reformat it
something like this:

if (!CommandLine::ForCurrentProcess()->HasSwitch(
        switches::kDisableOutdatedPlugins) ||
    !(*it)->IsVulnerable()) {

http://codereview.chromium.org/2967007/diff/2001/3006
File chrome/browser/plugin_updater.h (right):

http://codereview.chromium.org/2967007/diff/2001/3006#newcode32
chrome/browser/plugin_updater.h:32: // Disable all plugins groups that are
outdated.
This is also not a very useful comment. Can you explain more about what the
method does? When would it typically be called, how does it decide what's
outdated, what does disabling mean exactly?

http://codereview.chromium.org/2967007/diff/2001/3020
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/2967007/diff/2001/3020#newcode1234
chrome/common/render_messages_internal.h:1234: // is no plugin, |found| is set
to false.
No matching plugin, to be completely clear.

http://codereview.chromium.org/2967007/diff/2001/3022
File chrome/renderer/blocked_plugin.h (right):

http://codereview.chromium.org/2967007/diff/2001/3022#newcode32
chrome/renderer/blocked_plugin.h:32: void Load(const CppArgumentList& args,
CppVariant* result);
Please add brief comments to these methods. How is Load() different from
LoadPlugin()? What does Update() expect in its args?  What does OpenURL() do?

http://codereview.chromium.org/2967007/diff/2001/3023
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2967007/diff/2001/3023#newcode3723
chrome/renderer/render_view.cc:3723: 
Please remove extraneous line

[Bernhard Bauer, 2010-07-16 16:48:24]

All done (I hope).

[Pam (message me for reviews), 2010-07-19 15:16:34]

LGTM with below nits picked. Please still get someone who knows plug-ins better
to review too.

- Pam

http://codereview.chromium.org/2967007/diff/18001/19001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2967007/diff/18001/19001#newcode3687
chrome/app/generated_resources.grd:3687: Cannot enable plugins that are disabled
by policy
plug-ins

http://codereview.chromium.org/2967007/diff/18001/19020
File chrome/common/plugin_group.h (right):

http://codereview.chromium.org/2967007/diff/18001/19020#newcode34
chrome/common/plugin_group.h:34: // that is needed in order not to exhibit known
security vulnerabilities.
Thanks for expanding this comment. Please also re-wrap it.

http://codereview.chromium.org/2967007/diff/18001/19024
File chrome/renderer/blocked_plugin.h (right):

http://codereview.chromium.org/2967007/diff/18001/19024#newcode50
chrome/renderer/blocked_plugin.h:50: // the plugin there.
missing )

http://codereview.chromium.org/2967007/diff/18001/19024#newcode53
chrome/renderer/blocked_plugin.h:53: // Load the blocked plugin.
Just to avoid confusion, please note that this is identical to Load(), just not
coming from JS. (Or the other way around, adding to the comment on Load().)

http://codereview.chromium.org/2967007/diff/18001/19025
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2967007/diff/18001/19025#newcode2312
chrome/renderer/render_view.cc:2312: didNotAllowPlugins(frame);
indent

http://codereview.chromium.org/2967007/diff/18001/19025#newcode2314
chrome/renderer/render_view.cc:2314: }
indent

[Bernhard Bauer, 2010-07-19 15:41:55]

On 2010/07/19 15:16:34, Pam wrote:
> LGTM with below nits picked. Please still get someone who knows plug-ins
better
> to review too.

Nits picked. I think John is on vacation since today :-(

[Pam (message me for reviews), 2010-07-19 15:51:17]

Stuart, do you feel comfortable reviewing this, or can you suggest someone?

- Pam

[Bernhard Bauer, 2010-07-20 08:40:44]

On 2010/07/19 15:51:17, Pam wrote:
> Stuart, do you feel comfortable reviewing this, or can you suggest someone?
> 
> - Pam

Would it make reviewing easier if I split the CL up? I could split it between
disable outdated/block nonsandboxed plugins, or between renderer and browser
(infobar) changes, or even both.

[stuartmorgan, 2010-07-20 17:17:19]

Please update the CL description; it doesn't do the things in the title, it just
adds flags for them.

http://codereview.chromium.org/2967007/diff/50001/51005
File chrome/browser/plugin_updater.cc (right):

http://codereview.chromium.org/2967007/diff/50001/51005#newcode253
chrome/browser/plugin_updater.cc:253: !(*it)->IsVulnerable()) {
What is currently using IsVulnerable? Are we sure that everything using it
should be keyed off of this new flag? Since it predates the flag that seems
surprising.

http://codereview.chromium.org/2967007/diff/50001/51025
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/2967007/diff/50001/51025#oldcode3730
chrome/renderer/render_view.cc:3730: WebPlugin*
RenderView::CreatePluginInternal(WebFrame* frame,
It seems weird that CreatePluginInternal no longer creates pepper plug-ins.
Maybe rename this to CreateNPAPIPlugin, and make a parallel CreatePepperPlugin
so that there's less logic directly embedded in createPlugin.

http://codereview.chromium.org/2967007/diff/50001/51025
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2967007/diff/50001/51025#newcode2317
chrome/renderer/render_view.cc:2317: switches::kDisableOutdatedPlugins)) {
It seems like this line should be indented more, since it has a higher level of
nesting than its current alignment suggests.

http://codereview.chromium.org/2967007/diff/50001/51025#newcode2329
chrome/renderer/render_view.cc:2329: }
Fix indentation

http://codereview.chromium.org/2967007/diff/50001/51025#newcode2342
chrome/renderer/render_view.cc:2342: switches::kBlockNonSandboxedPlugins) &&
Same alignment concern here.

[darin (slow to review), 2010-07-20 17:43:44]

http://codereview.chromium.org/2967007/diff/50001/51014
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2967007/diff/50001/51014#newcode1961
chrome/browser/tab_contents/tab_contents.cc:1961: class DisabledPluginInfoBar :
public ConfirmInfoBarDelegate {
nit: please move this to the top of the file and place it in an anonymous
namespace, or move it to its own file (tab_contents.cc is already big enough!!).
 it is bad form to interrupt the flow of class method implementations with
something that is not a class method implementation :)

[Bernhard Bauer, 2010-07-26 17:39:32]

On 2010/07/20 17:17:19, stuartmorgan wrote:
> Please update the CL description; it doesn't do the things in the title, it
just
> adds flags for them.

done.

> http://codereview.chromium.org/2967007/diff/50001/51005
> File chrome/browser/plugin_updater.cc (right):
> 
> http://codereview.chromium.org/2967007/diff/50001/51005#newcode253
> chrome/browser/plugin_updater.cc:253: !(*it)->IsVulnerable()) {
> What is currently using IsVulnerable? Are we sure that everything using it
> should be keyed off of this new flag? Since it predates the flag that seems
> surprising.

IsVulnerable() is used to warn the user in about:plugins. That we don't save
preferences for vulnerable plugins without the --disable-outdated-plugins flag
is actually a bug (it slipped through another CL).

> http://codereview.chromium.org/2967007/diff/50001/51025
> File chrome/renderer/render_view.cc (left):
> 
> http://codereview.chromium.org/2967007/diff/50001/51025#oldcode3730
> chrome/renderer/render_view.cc:3730: WebPlugin*
> RenderView::CreatePluginInternal(WebFrame* frame,
> It seems weird that CreatePluginInternal no longer creates pepper plug-ins.
> Maybe rename this to CreateNPAPIPlugin, and make a parallel CreatePepperPlugin
> so that there's less logic directly embedded in createPlugin.

sounds good, done.

> http://codereview.chromium.org/2967007/diff/50001/51025
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/2967007/diff/50001/51025#newcode2317
> chrome/renderer/render_view.cc:2317: switches::kDisableOutdatedPlugins)) {
> It seems like this line should be indented more, since it has a higher level
of
> nesting than its current alignment suggests.

It's indented four spaces more than the previous line, because it's a
continuation thereof. Is that incorrect?

> http://codereview.chromium.org/2967007/diff/50001/51025#newcode2329
> chrome/renderer/render_view.cc:2329: }
> Fix indentation

done.

> http://codereview.chromium.org/2967007/diff/50001/51025#newcode2342
> chrome/renderer/render_view.cc:2342: switches::kBlockNonSandboxedPlugins) &&
> Same alignment concern here.

(see above)

On 2010/07/20 17:43:44, darin wrote:
> http://codereview.chromium.org/2967007/diff/50001/51014
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
> http://codereview.chromium.org/2967007/diff/50001/51014#newcode1961
> chrome/browser/tab_contents/tab_contents.cc:1961: class DisabledPluginInfoBar
:
> public ConfirmInfoBarDelegate {
> nit: please move this to the top of the file and place it in an anonymous
> namespace, or move it to its own file (tab_contents.cc is already big
enough!!).
>  it is bad form to interrupt the flow of class method implementations with
> something that is not a class method implementation :)

done (moved to top of file).

BTW, I'm splitting http://codereview.chromium.org/3015029 out of this CL (and
maybe some other smallish stuff, so it's easier to review).

[stuartmorgan, 2010-07-26 18:04:12]

On 2010/07/26 17:39:32, Bernhard Bauer wrote:
> It's indented four spaces more than the previous line, because it's a
> continuation thereof. Is that incorrect?

There's no style rule specifically covering continuation within a complex
conditional AFAIK, but it impairs readability, so it seems wrong in spirit. The
point of indenting continuations is to make them easily identifiable as you
read, and the current code has the same level of indentation a completely
unrelated extra clause in the conditional would have.

[Pam (message me for reviews), 2010-07-26 21:21:13]

On 2010/07/26 18:04:12, stuartmorgan wrote:
> There's no style rule specifically covering continuation within a complex
> conditional AFAIK, but it impairs readability, so it seems wrong in spirit. 

For readability, I'd probably switch the conditions and indent like this:

if ( CommandLine::ForCurrentProcess()->HasSwitch(
        switches::kDisableOutdatedPlugins) &&
    group->IsVulnerable()) {

and likewise

if (CommandLine::ForCurrentProcess()->HasSwitch(
        switches::kBlockNonSandboxedPlugins) &&
    path.value() != kDefaultPluginLibraryName) {

unless short-circuiting the first HasSwitch() call has significant performance
impact, in which case I'd reverse them both so they still match.

Painting the bikeshed blue,
- Pam

[Pam (message me for reviews), 2010-08-04 18:15:07]

If this has been committed, could you close the issue please?

- Pam