Block NPAPI plugins by default

Block NPAPI plugins by default

Add --enable-npapi flag in chrome://flags to support re-enable.

BUG=295137
TEST=browser_tests, unit_tests, content_browsertests, blink layout tests

Committed: https://crrev.com/0539f2f5214fc17fdd5d1964be0ceedf98fdb9ce
Cr-Commit-Position: refs/heads/master@{#312374}

[Will Harris, 2014-10-10 22:08:56]

wfh@chromium.org changed reviewers:
+ jam@chromium.org

[Will Harris, 2014-10-10 22:09:27]

wfh@chromium.org changed reviewers:
+ komoroske@chromium.org

[Will Harris, 2014-10-10 22:09:58]

PTAL

komoroske for sanity check on strings.

[Will Harris, 2014-10-10 23:18:58]

Patchset #2 (id:150001) has been deleted

[Will Harris, 2014-10-11 00:34:39]

Patchset #2 (id:220001) has been deleted

[Will Harris, 2014-10-13 03:47:52]

looks like there's quite a few tests I need to fix up still.

[Will Harris, 2014-10-16 15:58:15]

jam@ PTAL - This appears to be passing on all the tests I can find, including
blink layout tests, so ready for review.

[Will Harris, 2014-10-16 18:45:01]

wfh@chromium.org changed reviewers:
+ sky@chromium.org

[Will Harris, 2014-10-16 18:45:45]

sky@ can you take a look at content/browser, where the main changes are made,
most of the other changes are to fix/add tests.

[sky, 2014-10-16 19:53:09]

John is an owner for all this code, he should review.

[jam, 2014-10-16 22:10:57]

https://codereview.chromium.org/645203002/diff/780001/chrome/test/data/load_n...
File chrome/test/data/load_npapi_plugin.html (right):

https://codereview.chromium.org/645203002/diff/780001/chrome/test/data/load_n...
chrome/test/data/load_npapi_plugin.html:21: setTimeout(function(){notloaded()},
2000);
this seems flaky? what if it takes the plugin process time to start and this
times out first?

https://codereview.chromium.org/645203002/diff/780001/content/browser/plugin_...
File content/browser/plugin_service_impl.h (right):

https://codereview.chromium.org/645203002/diff/780001/content/browser/plugin_...
content/browser/plugin_service_impl.h:111: #if defined(OS_WIN) ||
defined(OS_MACOSX)
do we really need these ifdefs everywhere? why not just take them out and
default to false elsewhere?

[Will Harris, 2014-10-17 18:57:09]

PTAL

https://codereview.chromium.org/645203002/diff/780001/chrome/test/data/load_n...
File chrome/test/data/load_npapi_plugin.html (right):

https://codereview.chromium.org/645203002/diff/780001/chrome/test/data/load_n...
chrome/test/data/load_npapi_plugin.html:21: setTimeout(function(){notloaded()},
2000);
On 2014/10/16 22:10:57, jam wrote:
> this seems flaky? what if it takes the plugin process time to start and this
> times out first?

I changed this to call into the plugin directly using npruntime, so it is
accurately determined whether it's loaded or not.

https://codereview.chromium.org/645203002/diff/780001/content/browser/plugin_...
File content/browser/plugin_service_impl.h (right):

https://codereview.chromium.org/645203002/diff/780001/content/browser/plugin_...
content/browser/plugin_service_impl.h:111: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2014/10/16 22:10:57, jam wrote:
> do we really need these ifdefs everywhere? why not just take them out and
> default to false elsewhere?

Done.

https://codereview.chromium.org/645203002/diff/800001/chrome/test/data/load_n...
File chrome/test/data/load_npapi_plugin.html (right):

https://codereview.chromium.org/645203002/diff/800001/chrome/test/data/load_n...
chrome/test/data/load_npapi_plugin.html:11: child.innerHTML =
'<embed	type="application/vnd.npapi-test" src="foo"' +
nit: tab here.

[jam, 2014-10-20 15:50:32]

lgtm

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_browsertest.cc:328: #if
defined(OS_MACOSX) || defined(OS_WIN)
ditto

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs_unittest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs_unittest.cc:213: #if defined(OS_WIN) ||
defined(OS_MACOSX)
ditto?

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1091: #if defined(OS_WIN) ||
defined(OS_MACOSX)
ditto

https://codereview.chromium.org/645203002/diff/840001/content/browser/plugin_...
File content/browser/plugin_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/content/browser/plugin_...
content/browser/plugin_browsertest.cc:56: #if defined(OS_WIN) ||
defined(OS_MACOSX)
dont need ifdef?

https://codereview.chromium.org/645203002/diff/840001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/645203002/diff/840001/content/public/common/c...
content/public/common/content_switches.cc:959: #if defined(OS_WIN) ||
defined(OS_MACOSX)
can we just leave this for all to get rid of ifdefs elsewhere, and just have the
place that sets a flag based on this be the only place that's behind ifdef?

[Will Harris, 2014-10-20 17:59:18]

wfh@chromium.org changed reviewers:
+ isherman@chromium.org

[Will Harris, 2014-10-20 17:59:56]

+isherman@ for OWNERS on histograms.xml

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_browsertest.cc:328: #if
defined(OS_MACOSX) || defined(OS_WIN)
On 2014/10/20 15:50:32, jam wrote:
> ditto

Done.

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs_unittest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs_unittest.cc:213: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2014/10/20 15:50:32, jam wrote:
> ditto?

Done.

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1091: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2014/10/20 15:50:32, jam wrote:
> ditto

Done.

https://codereview.chromium.org/645203002/diff/840001/content/browser/plugin_...
File content/browser/plugin_browsertest.cc (right):

https://codereview.chromium.org/645203002/diff/840001/content/browser/plugin_...
content/browser/plugin_browsertest.cc:56: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2014/10/20 15:50:32, jam wrote:
> dont need ifdef?

Done.

https://codereview.chromium.org/645203002/diff/840001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/645203002/diff/840001/content/public/common/c...
content/public/common/content_switches.cc:959: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2014/10/20 15:50:32, jam wrote:
> can we just leave this for all to get rid of ifdefs elsewhere, and just have
the
> place that sets a flag based on this be the only place that's behind ifdef?

Done.

[Ilya Sherman, 2014-10-21 01:34:57]

histograms.xml lgtm

[jschuh, 2014-12-09 17:10:50]

Just a reminder that we want to have UMA data for what users run Chrome with the
enable flag, rather than just when a user toggles it.

[Will Harris, 2015-01-16 22:32:55]

Patchset #16 (id:900001) has been deleted

[Will Harris, 2015-01-17 01:47:09]

Patchset #16 (id:920001) has been deleted

[Will Harris, 2015-01-20 02:45:12]

Rebased, and added the NPAPI histogram. isherman@ can you take a quick look over
please?  Thanks.

[Ilya Sherman, 2015-01-20 20:45:18]

Histograms LGTM % a nit:

https://codereview.chromium.org/645203002/diff/960001/content/browser/plugin_...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/645203002/diff/960001/content/browser/plugin_...
content/browser/plugin_service_impl.cc:201: NPAPI_STATUS_ENUM_COUNT);
nit: To reduce the risk of typos creeping in to one of the two parallel
codepaths, I'd recommend writing this as:

#if defined(OS_WIN) || defined(OS_MACOSX)
NPAPIPluginStatus status =
    npapi_plugins_enabled_ ? NPAPI_STATUS_ENABLED : NPAPI_STATUS_DISABLED;
#else
NPAPIPluginStatus status = NPAPI_STATUS_UNSUPPORTED;
#endif
UMA_HISTOGRAM_ENUMERATION("Plugin.NPAPIStatus", status,
                          NPAPI_STATUS_ENUM_COUNT);

[Will Harris, 2015-01-20 22:35:12]

thanks for all the reviews.

https://codereview.chromium.org/645203002/diff/960001/content/browser/plugin_...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/645203002/diff/960001/content/browser/plugin_...
content/browser/plugin_service_impl.cc:201: NPAPI_STATUS_ENUM_COUNT);
On 2015/01/20 20:45:18, Ilya Sherman wrote:
> nit: To reduce the risk of typos creeping in to one of the two parallel
> codepaths, I'd recommend writing this as:
> 
> #if defined(OS_WIN) || defined(OS_MACOSX)
> NPAPIPluginStatus status =
>     npapi_plugins_enabled_ ? NPAPI_STATUS_ENABLED : NPAPI_STATUS_DISABLED;
> #else
> NPAPIPluginStatus status = NPAPI_STATUS_UNSUPPORTED;
> #endif
> UMA_HISTOGRAM_ENUMERATION("Plugin.NPAPIStatus", status,
>                           NPAPI_STATUS_ENUM_COUNT);

Done.

[Will Harris, 2015-01-20 22:35:18]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-01-20 22:36:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645203002/980001

[commit-bot: I haz the power, 2015-01-21 00:08:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:08:19]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Will Harris, 2015-01-21 01:07:56]

On 2015/01/21 00:08:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

this looks like a flake since the same code was passing in ps16... re-try.

[Will Harris, 2015-01-21 01:08:07]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-01-21 01:14:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645203002/980001

[commit-bot: I haz the power, 2015-01-21 01:47:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 01:47:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Will Harris, 2015-01-21 03:17:55]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-01-21 03:19:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645203002/980001

[commit-bot: I haz the power, 2015-01-21 14:37:16]

Committed patchset #18 (id:980001)

[commit-bot: I haz the power, 2015-01-21 14:38:53]

Patchset 18 (id:??) landed as
https://crrev.com/0539f2f5214fc17fdd5d1964be0ceedf98fdb9ce
Cr-Commit-Position: refs/heads/master@{#312374}