BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629
Committed: https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99
Cr-Commit-Position: refs/heads/master@{#426852}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[ncarter (slow), 2016-10-20 21:52:52]

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

[commit-bot: I haz the power, 2016-10-20 21:53:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-10-20 21:57:42]

Description was changed from

==========
browser_process_impl.cc: Move IsIsolateExtensionsEnabled() to after field trial
init.

BUG=657629
==========

to

==========
browser_process_impl.cc: Move IsIsolateExtensionsEnabled() to after field trial
init.

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so this call returned true regardless
of the finch experiment state.

PreCreateThreads() is the earliest place in BrowserProcessImpl where we can do
this registration.

BUG=657629
==========

[ncarter (slow), 2016-10-20 22:00:14]

Description was changed from

==========
browser_process_impl.cc: Move IsIsolateExtensionsEnabled() to after field trial
init.

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so this call returned true regardless
of the finch experiment state.

PreCreateThreads() is the earliest place in BrowserProcessImpl where we can do
this registration.

BUG=657629
==========

to

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629
==========

[ncarter (slow), 2016-10-20 22:12:03]

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

[ncarter (slow), 2016-10-20 22:12:04]

thestig@: Please take a look; and let me know if you know a better spot. I
looked at many possible places to do this, but nothing seemed particularly
fitting.

Two alternatives I considered:

 - chrome_extensions::EnsureBrowserContextKeyedServiceFactoriesBuilt(): this is
the moment where extension service factories are installed (driven by
ExtraPartsProfile). However, it both feels inappropriate (since it's not
installing a factory instance) and also, the security state is global, not
per-profile configuration. We would actually DCHECK if we tried to do it for a
second profile.

- ChromeBrowserMainParts::SetupFieldTrials() or the bottom of
ChromeBrowserMainParts::PreCreateThreadsImpl(), just after the call to
SetupFieldTrials() -- seemed too specific, and these are basically the same time
as BrowserProcessImpl::PreCreateThreads(), so I figured it's better to leave all
the RegisterWebSafeScheme calls in the same process.

Soon (in M57 maybe) we will remove extensions::IsIsolateExtensionsEnabled().
When that happens we can move this code back to the original spot.

[ncarter (slow), 2016-10-20 22:12:51]

On 2016/10/20 22:12:04, ncarter wrote:
> thestig@: Please take a look; and let me know if you know a better spot. I
> looked at many possible places to do this, but nothing seemed particularly
> fitting.
> 
> Two alternatives I considered:
> 
>  - chrome_extensions::EnsureBrowserContextKeyedServiceFactoriesBuilt(): this
is
> the moment where extension service factories are installed (driven by
> ExtraPartsProfile). However, it both feels inappropriate (since it's not
> installing a factory instance) and also, the security state is global, not
> per-profile configuration. We would actually DCHECK if we tried to do it for a
> second profile.
> 
> - ChromeBrowserMainParts::SetupFieldTrials() or the bottom of
> ChromeBrowserMainParts::PreCreateThreadsImpl(), just after the call to
> SetupFieldTrials() -- seemed too specific, and these are basically the same
time
> as BrowserProcessImpl::PreCreateThreads(), so I figured it's better to leave
all
> the RegisterWebSafeScheme calls in the same process.
> 
> Soon (in M57 maybe) we will remove extensions::IsIsolateExtensionsEnabled().
> When that happens we can move this code back to the original spot.

s/"in the same process"/"in the same class (BrowserProcessImpl)"/

[Lei Zhang, 2016-10-20 22:21:27]

So we are checking is a field trial is enabled before the field trial had a
chance to initialize? Is that the type of bug that should trigger a DCHECK
failure?

It is probably better if you ask someone who works on metrics and someone who
works on extensions to review this instead. For instance, I don't know if
RegisterWebSafeScheme() needs to come before extensions::ExtensionsClient::Set()
and friends.

[ncarter (slow), 2016-10-20 22:46:43]

On 2016/10/20 22:21:27, Lei Zhang wrote:
> So we are checking is a field trial is enabled before the field trial had a
> chance to initialize? Is that the type of bug that should trigger a DCHECK
> failure?

Agree, surprised there wasn't a DCHECK. I'm working on that, and already found
another violation (657939).

> It is probably better if you ask someone who works on metrics and someone who
> works on extensions to review this instead. For instance, I don't know if
> RegisterWebSafeScheme() needs to come before
extensions::ExtensionsClient::Set()
> and friends.

I'd be happy to involve someone else. Main reason I included you, is because I
thought that you might have opinions about whether putting any code in
BrowserProcessImpl::PreCreateThreads() was distasteful. FWIW, I had already
looked at extensions::ExtensionsClient::Set, but that's also too early (happens
before field trial params are read).

Really, anytime "after field trial init, but before the first profile is
created" ought to suffice for this. The policy state needs to be configured
before the first navigation occurs.

[ncarter (slow), 2016-10-20 22:55:13]

nick@chromium.org changed reviewers:
+ nasko@chromium.org, rdevlin.cronin@chromium.org, robliao@chromium.org

[ncarter (slow), 2016-10-20 22:55:13]

nasko, robliao, devlin: this is a tricking initialization ordering question. We
want to setup the security treatment for the chrome-extension scheme early, but
since we depend on a field trial, we were originally doing it too early, before
ChromeBrowserMainParts::SetupFieldTrials() had run.

Curious if you see a better spot for this (i.e., could we / should we do it in
ChromeBrowserMainParts::SetupFieldTrials() directly ? )

[ncarter (slow), 2016-10-20 22:55:29]

On 2016/10/20 22:55:13, ncarter wrote:
> nasko, robliao, devlin: this is a tricking initialization ordering question.
We
> want to setup the security treatment for the chrome-extension scheme early,
but
> since we depend on a field trial, we were originally doing it too early,
before
> ChromeBrowserMainParts::SetupFieldTrials() had run.
> 
> Curious if you see a better spot for this (i.e., could we / should we do it in
> ChromeBrowserMainParts::SetupFieldTrials() directly ? )

s/tricking/tricky/

[robliao, 2016-10-20 23:05:21]

On 2016/10/20 22:55:29, ncarter wrote:
> On 2016/10/20 22:55:13, ncarter wrote:
> > nasko, robliao, devlin: this is a tricking initialization ordering question.
> We
> > want to setup the security treatment for the chrome-extension scheme early,
> but
> > since we depend on a field trial, we were originally doing it too early,
> before
> > ChromeBrowserMainParts::SetupFieldTrials() had run.
> > 
> > Curious if you see a better spot for this (i.e., could we / should we do it
in
> > ChromeBrowserMainParts::SetupFieldTrials() directly ? )
> 
> s/tricking/tricky/

[Lei Zhang, 2016-10-20 23:06:09]

On 2016/10/20 22:46:43, ncarter wrote:
> I'd be happy to involve someone else. Main reason I included you, is because I
> thought that you might have opinions about whether putting any code in
> BrowserProcessImpl::PreCreateThreads() was distasteful. FWIW, I had already
> looked at extensions::ExtensionsClient::Set, but that's also too early
(happens
> before field trial params are read).
> 
> Really, anytime "after field trial init, but before the first profile is
> created" ought to suffice for this. The policy state needs to be configured
> before the first navigation occurs.

PreCreateThreads() is certainly a possible spot, and I don't have a strong
opinion until we work out when the other extensions initialization have to take
place.

In some cases, we want to push things that are not critical as far towards the
end as possible to get them out of the critical path during startup, but this is
not one of those.

[robliao, 2016-10-20 23:07:34]

On 2016/10/20 23:05:21, robliao wrote:
> On 2016/10/20 22:55:29, ncarter wrote:
> > On 2016/10/20 22:55:13, ncarter wrote:
> > > nasko, robliao, devlin: this is a tricking initialization ordering
question.
> > We
> > > want to setup the security treatment for the chrome-extension scheme
early,
> > but
> > > since we depend on a field trial, we were originally doing it too early,
> > before
> > > ChromeBrowserMainParts::SetupFieldTrials() had run.
> > > 
> > > Curious if you see a better spot for this (i.e., could we / should we do
it
> in
> > > ChromeBrowserMainParts::SetupFieldTrials() directly ? )
> > 
> > s/tricking/tricky/

Adding some interested parties. Yep, this is pretty much the soonest you can do
this from the browser process's perspective.

However, this initialization doesn't seem browser process specific. Maybe it
would fit better in ChromeBrowserMainParts?

[commit-bot: I haz the power, 2016-10-20 23:21:23]

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

[commit-bot: I haz the power, 2016-10-20 23:21:24]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-20 23:39:40]

On 2016/10/20 23:07:34, robliao wrote:
> On 2016/10/20 23:05:21, robliao wrote:
> > On 2016/10/20 22:55:29, ncarter wrote:
> > > On 2016/10/20 22:55:13, ncarter wrote:
> > > > nasko, robliao, devlin: this is a tricking initialization ordering
> question.
> > > We
> > > > want to setup the security treatment for the chrome-extension scheme
> early,
> > > but
> > > > since we depend on a field trial, we were originally doing it too early,
> > > before
> > > > ChromeBrowserMainParts::SetupFieldTrials() had run.
> > > > 
> > > > Curious if you see a better spot for this (i.e., could we / should we do
> it
> > in
> > > > ChromeBrowserMainParts::SetupFieldTrials() directly ? )
> > > 
> > > s/tricking/tricky/
> 
> Adding some interested parties. Yep, this is pretty much the soonest you can
do
> this from the browser process's perspective.
> 
> However, this initialization doesn't seem browser process specific. Maybe it
> would fit better in ChromeBrowserMainParts?

If there were an ExtraPartsExtensions, that might be an ideal place for this.
But I don't see anything quite like that.

Is there a rule for what belongs in BrowserProcessImpl vs
ChromeBrowserMainParts?  Anyhow, I'd be happy to have this in
ChromeBrowserMainParts -- any preferences where? somewhere near
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?sq...

FWIW0, I left it in BrowserProcessImpl because that's where I found it. the
kChromeSearchScheme is also registered as web-safe by BrowserProcessImpl.

FWIW1, In extensions shell, it happens here:
https://cs.chromium.org/chromium/src/extensions/shell/browser/shell_browser_m...

FWIW2, fixing this bug is somewhat urgent: some extensions on M55 are broken as
a result of this (for 10% of dev&beta).

FWIW3, whatever we settle on, it may likely be temporary code; I'm hoping to
push --isolate-extensions down into //extensions this quarter, though that's
somewhat at risk if these blob security issues keep percolating. When that
happens there may be a better place in //extensions that we can hook. Right now
this has to live in //chrome.

[robliao, 2016-10-20 23:59:19]

On 2016/10/20 23:39:40, ncarter wrote:
> On 2016/10/20 23:07:34, robliao wrote:
> > On 2016/10/20 23:05:21, robliao wrote:
> > > On 2016/10/20 22:55:29, ncarter wrote:
> > > > On 2016/10/20 22:55:13, ncarter wrote:
> > > > > nasko, robliao, devlin: this is a tricking initialization ordering
> > question.
> > > > We
> > > > > want to setup the security treatment for the chrome-extension scheme
> > early,
> > > > but
> > > > > since we depend on a field trial, we were originally doing it too
early,
> > > > before
> > > > > ChromeBrowserMainParts::SetupFieldTrials() had run.
> > > > > 
> > > > > Curious if you see a better spot for this (i.e., could we / should we
do
> > it
> > > in
> > > > > ChromeBrowserMainParts::SetupFieldTrials() directly ? )
> > > > 
> > > > s/tricking/tricky/
> > 
> > Adding some interested parties. Yep, this is pretty much the soonest you can
> do
> > this from the browser process's perspective.
> > 
> > However, this initialization doesn't seem browser process specific. Maybe it
> > would fit better in ChromeBrowserMainParts?
> 
> If there were an ExtraPartsExtensions, that might be an ideal place for this.
> But I don't see anything quite like that.
> 
> Is there a rule for what belongs in BrowserProcessImpl vs
> ChromeBrowserMainParts?  Anyhow, I'd be happy to have this in
> ChromeBrowserMainParts -- any preferences where? somewhere near
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?sq...
> 
> FWIW0, I left it in BrowserProcessImpl because that's where I found it. the
> kChromeSearchScheme is also registered as web-safe by BrowserProcessImpl.
> 
> FWIW1, In extensions shell, it happens here:
>
https://cs.chromium.org/chromium/src/extensions/shell/browser/shell_browser_m...
> 
> FWIW2, fixing this bug is somewhat urgent: some extensions on M55 are broken
as
> a result of this (for 10% of dev&beta).
> 
> FWIW3, whatever we settle on, it may likely be temporary code; I'm hoping to
> push --isolate-extensions down into //extensions this quarter, though that's
> somewhat at risk if these blob security issues keep percolating. When that
> happens there may be a better place in //extensions that we can hook. Right
now
> this has to live in //chrome.

The simple intrafile move is fine quick fix to me.

As for what goes where, I've always imagined that BrowserProcessImpl contains
initialization for services obtained through it.
For everything else, it would go in one of the main parts (for Chrome) or in
content startup (for startup actually owned by content.

[nasko, 2016-10-21 16:51:01]

Move within the file LGTM, since we want this to be merged back to M55. I agree
finding a better spot is desirable, but done at a later point.

[ncarter (slow), 2016-10-21 17:22:27]

lei: I need OWNERS approval

[Lei Zhang, 2016-10-21 18:15:49]

lgtm

[ncarter (slow), 2016-10-21 18:19:17]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-10-21 18:20:09]

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, 2016-10-21 18:39:46]

Description was changed from

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629
==========

to

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629
==========

[commit-bot: I haz the power, 2016-10-21 18:39:47]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-21 18:42:53]

Description was changed from

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629
==========

to

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629

Committed: https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99
Cr-Commit-Position: refs/heads/master@{#426852}
==========

[commit-bot: I haz the power, 2016-10-21 18:42:54]

Patchset 1 (id:??) landed as
https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99
Cr-Commit-Position: refs/heads/master@{#426852}

[ncarter (slow), 2016-10-26 19:19:52]

Description was changed from

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629

Committed: https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99
Cr-Commit-Position: refs/heads/master@{#426852}
==========

to

==========
BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early

This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the
field trials actually being initialized, so the call returned true regardless
of the finch experiment state, resulting in a possible misconfiguration of
security state.

PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we
can do this registration.

BUG=657629

Committed: https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99
Cr-Commit-Position: refs/heads/master@{#426852}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2016-10-26 20:56:32]

Patchset #2 (id:20001) has been deleted