Remove command line/field trial support and configs for Isolate Extensions.

Remove command line/field trial support for disabling Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter,
and tests that are no longer relevant.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2850793005
Cr-Commit-Position: refs/heads/master@{#468898}
Committed: https://chromium.googlesource.com/chromium/src/+/abed2a56f122e7d6ca95e37a42c0c14b73a85a35

[nasko, 2017-04-29 00:34:45]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:35:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-04-29 00:40:50]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:40:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-04-29 00:41:15]

Patchset #2 (id:20001) has been deleted

[nasko, 2017-04-29 00:42:59]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:43:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 01:26:34]

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

[commit-bot: I haz the power, 2017-04-29 01:26:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-04-29 14:56:49]

Description was changed from

==========
Remove command line support/field trial configs for Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter.

BUG=545200
==========

to

==========
Remove command line/field trial support and configs for Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter.

BUG=545200
==========

[nasko, 2017-04-29 14:56:57]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 14:57:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 16:12:28]

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

[commit-bot: I haz the power, 2017-04-29 16:12:29]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-05-01 16:23:45]

Description was changed from

==========
Remove command line/field trial support and configs for Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter.

BUG=545200
==========

to

==========
Remove command line/field trial support and configs for Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-05-01 16:24:14]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 16:24:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-05-01 16:25:21]

Description was changed from

==========
Remove command line/field trial support and configs for Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove command line/field trial support for disabling Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter,
and tests that are no longer relevant.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-05-01 16:26:23]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2017-05-01 16:26:24]

Hey Nick,
Can you review this CL for me? It removes "Isolate Extensions" as a configurable
knob and makes it universally the default behavior.
Thanks in advance!
Nasko

[commit-bot: I haz the power, 2017-05-01 17:28:47]

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

[commit-bot: I haz the power, 2017-05-01 17:28:48]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-05-01 20:15:19]

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:735: //
the iframe runs in a separate process.  See https://crbug.com/546267.
"This primarily targets ..." -> "The iframe is expected to run in a separate
process."

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:336:
// Extensions should be isolated, except for hosted  apps. Isolating hosted
remove extra space before "apps."

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_management_browsertest.cc:367:
content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
Remove |crash_observer|

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details.cc (left):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details.cc:43: case ISOLATE_EXTENSIONS: {
I don't think it's necessary to remove this?  If anything we should remove
ISOLATE_NOTHING and keep ISOLATE_EXTENSIONS, since it's the new baseline.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:121: case ISOLATE_NOTHING:
ISOLATE_NOTHING ought to be the unreachable case now (except on Android)

https://codereview.chromium.org/2850793005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:703: // protect it from
outsiders :).
Removing the mention of 'extension' from this comment obscures the point
somewhat: which is that we're supposed to pretend that "*.is" is a stand-in for
extensions.

[nasko, 2017-05-01 21:25:37]

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:735: //
the iframe runs in a separate process.  See https://crbug.com/546267.
On 2017/05/01 20:15:19, ncarter wrote:
> "This primarily targets ..." -> "The iframe is expected to run in a separate
> process."

Done.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:336:
// Extensions should be isolated, except for hosted  apps. Isolating hosted
On 2017/05/01 20:15:19, ncarter wrote:
> remove extra space before "apps."

Done.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_management_browsertest.cc:367:
content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
On 2017/05/01 20:15:19, ncarter wrote:
> Remove |crash_observer|

Done.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details.cc (left):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details.cc:43: case ISOLATE_EXTENSIONS: {
On 2017/05/01 20:15:19, ncarter wrote:
> I don't think it's necessary to remove this?  If anything we should remove
> ISOLATE_NOTHING and keep ISOLATE_EXTENSIONS, since it's the new baseline.

The way I read this code is "What should our numbers look if we assume
non-default process model?". Since the default process model is now to isolate
extensions, it is definitely the new baseline and I assumed ISOLATE_NOTHING to
be the value indicating baseline. We were already isolating WebUI, actual
extensions background pages, so ISOLATE_NOTHING wasn't really a great name in
the past either. Would changing it to ISOLATE_DEFAULT be a better naming?

Irrespective of the naming, would we need this code below? It seems the default
process model already does it, so calculating expected values doesn't seem
necessary, when they are the actual values.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:121: case ISOLATE_NOTHING:
On 2017/05/01 20:15:19, ncarter wrote:
> ISOLATE_NOTHING ought to be the unreachable case now (except on Android)

Are you suggesting it should be #ifdef'd for Android only?

https://codereview.chromium.org/2850793005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:703: // protect it from
outsiders :).
On 2017/05/01 20:15:19, ncarter wrote:
> Removing the mention of 'extension' from this comment obscures the point
> somewhat: which is that we're supposed to pretend that "*.is" is a stand-in
for
> extensions.

Done.

[ncarter (slow), 2017-05-01 21:37:44]

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details.cc (left):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details.cc:43: case ISOLATE_EXTENSIONS: {
On 2017/05/01 21:25:37, nasko wrote:
> On 2017/05/01 20:15:19, ncarter wrote:
> > I don't think it's necessary to remove this?  If anything we should remove
> > ISOLATE_NOTHING and keep ISOLATE_EXTENSIONS, since it's the new baseline.
> 
> The way I read this code is "What should our numbers look if we assume
> non-default process model?". Since the default process model is now to isolate
> extensions, it is definitely the new baseline and I assumed ISOLATE_NOTHING to
> be the value indicating baseline. We were already isolating WebUI, actual
> extensions background pages, so ISOLATE_NOTHING wasn't really a great name in
> the past either. Would changing it to ISOLATE_DEFAULT be a better naming?
> 
> Irrespective of the naming, would we need this code below? It seems the
default
> process model already does it, so calculating expected values doesn't seem
> necessary, when they are the actual values.

The value of computing the simulated ISOLATE_EXTENSIONS is mainly in being able
to cross validate the simulation (which is imperfect) with the actual.

It's alsoe potentially useful to compare ISOLATE_HTTPS_SITES to the simulated
ISOLATE_EXTENSIONS metric, since the NoLimit/LowerBound/Estimate/CountEstimate
metrics are computed the same way, and are directly comparable. Converting from
the actual process count is requires quite a few more assumptions.

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:121: case ISOLATE_NOTHING:
On 2017/05/01 21:25:37, nasko wrote:
> On 2017/05/01 20:15:19, ncarter wrote:
> > ISOLATE_NOTHING ought to be the unreachable case now (except on Android)
> 
> Are you suggesting it should be #ifdef'd for Android only?

Hmm, probably not necessary to ifdef on android, since we won't actually
encounter chrome extension URLs there.

[nasko, 2017-05-01 22:12:43]

Per offline discussion, removing most of the changes in site_details* files, as
computed metrics aren't relevant to the actual code running.

[ncarter (slow), 2017-05-01 22:19:50]

lgtm

[nasko, 2017-05-01 22:25:48]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-05-01 22:26:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:31:33]

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

[commit-bot: I haz the power, 2017-05-01 22:31:34]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2017-05-02 00:32:39]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-02 00:32:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2850793005/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-05-02 00:33:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 00:44:10]

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

[commit-bot: I haz the power, 2017-05-02 00:44:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2017-05-02 01:02:26]

nasko@chromium.org changed reviewers:
+ isherman@chromium.org, rdevlin.cronin@chromium.org, thestig@chromium.org

[nasko, 2017-05-02 01:02:27]

Hello, would you mind doing an OWNERS review for me? It is mostly code removal.

rdevlin.cronin@ - extensions code
thestig@ - remainder of chrome/
isherman@ - testing/variations/fieldtrial_testing_config.json

[Devlin, 2017-05-02 15:20:54]

Yay!

extensions lgtm

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:7: #include
"base/command_line.h"
This is no longer needed

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:8: #include
"base/metrics/field_trial.h"
ditto

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:11: #include
"content/public/common/content_switches.h"
ditto

[nasko, 2017-05-02 15:50:06]

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:7: #include
"base/command_line.h"
On 2017/05/02 15:20:54, Devlin wrote:
> This is no longer needed

Done.

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:8: #include
"base/metrics/field_trial.h"
On 2017/05/02 15:20:54, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2850793005/diff/140001/chrome/common/extensio...
chrome/common/extensions/extension_process_policy.cc:11: #include
"content/public/common/content_switches.h"
On 2017/05/02 15:20:54, Devlin wrote:
> ditto

Done.

[Ilya Sherman, 2017-05-02 21:15:18]

testing/variations/fieldtrial_testing_config.json lgtm

[Lei Zhang, 2017-05-02 21:32:01]

lgtm

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:24: #include "base/metrics/field_trial.h"
Not used?

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_security_exploit_browsertest.cc:13: #include
"chrome/common/extensions/extension_process_policy.h"
No longer used.

https://codereview.chromium.org/2850793005/diff/160001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:74: #include
"chrome/common/extensions/extension_process_policy.h"
No longer used.

[nasko, 2017-05-03 03:50:43]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-03 03:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
rdevlin.cronin@chromium.org, thestig@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2850793005/#ps180001
(title: "Remove unused includes.")

[commit-bot: I haz the power, 2017-05-03 03:51:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 03:54:40]

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

[commit-bot: I haz the power, 2017-05-03 03:54:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-05-03 04:04:19]

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:24: #include "base/metrics/field_trial.h"
On 2017/05/02 21:32:00, Lei Zhang wrote:
> Not used?

Done.

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_security_exploit_browsertest.cc:13: #include
"chrome/common/extensions/extension_process_policy.h"
On 2017/05/02 21:32:01, Lei Zhang wrote:
> No longer used.

Done.

https://codereview.chromium.org/2850793005/diff/160001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2850793005/diff/160001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:74: #include
"chrome/common/extensions/extension_process_policy.h"
On 2017/05/02 21:32:01, Lei Zhang wrote:
> No longer used.

Done.

[nasko, 2017-05-03 04:09:15]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-03 04:09:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, nick@chromium.org, isherman@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2850793005/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-05-03 04:09:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 05:10:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493784555985050,
"parent_rev": "9d0f39951bc1fb088380dbde65e8188a75194cdd", "commit_rev":
"abed2a56f122e7d6ca95e37a42c0c14b73a85a35"}

[commit-bot: I haz the power, 2017-05-03 05:11:16]

Description was changed from

==========
Remove command line/field trial support for disabling Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter,
and tests that are no longer relevant.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove command line/field trial support for disabling Isolate Extensions.

The Isolate Extensions project/feature shipped on by default in M56.
This CL removes the support for disabling Isolate Extensions, which
includes field trial config, supporting code, command line parameter,
and tests that are no longer relevant.

BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2850793005
Cr-Commit-Position: refs/heads/master@{#468898}
Committed:
https://chromium.googlesource.com/chromium/src/+/abed2a56f122e7d6ca95e37a42c0...
==========

[commit-bot: I haz the power, 2017-05-03 05:11:17]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/abed2a56f122e7d6ca95e37a42c0...