Initialize BrowserPolicyConnector after the IOThread is created.

Initialize BrowserPolicyConnector after the IOThread is created.

The import process on Windows spawns a subprocess and spins a nested message loop waiting for it to complete. If this subprocess takes longer than 5 seconds then the message loop will execute a delayed initialization task of the policy code, that grabs the system request context; however, since the IOThread hasn't been created yet this leads to a crash.

BUG=177843
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184967

[Joao da Silva, 2013-02-25 09:43:22]

Please review. This will likely be merged to 26.

[Mattias Nissler (ping if slow), 2013-02-25 18:44:41]

cloud policy browser_test is broken apparently.

https://codereview.chromium.org/12310100/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12310100/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:148: void
BrowserPolicyConnector::Init() {
Bonus points for passing in local_state and system_request_context here, that
way we'd become entirely independent from globals.

https://codereview.chromium.org/12310100/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:162: GetAppPackUpdater();
Should this really be started before device_cloud_policy_manager_->Init() below?

[Joao da Silva, 2013-02-26 14:07:14]

@Mattias: PTAL

@Jochen: please review the chrome/ changes

[Mattias Nissler (ping if slow), 2013-02-26 14:22:00]

LGTM

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:848: // the main BrowserThreads are
ready. All of that is ready at this stage.
This comment could be clearer. In particular, you could mention that:
* We need to initialize early because local_state initialization needs policy.
* Policy updating (i.e. network fetches, watching the disk) can only be done
once threads are up, justifying the init call here.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:269:
g_browser_process->system_request_context());
Btw. Any plans to rewrite this test as a proper browser_test?

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/asyn...
File chrome/browser/policy/async_policy_provider.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/asyn...
chrome/browser/policy/async_policy_provider.cc:48: DCHECK(post) <<
"AsyncPolicyProvider::Init() called too early!";
Or too late. This could be a CHECK() I guess?

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (left):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:118: if
(!device_management_service_.get()) {
Why the conditional here?

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:109: // assume that
g_browser_process is fully initialized yet, nor the browser
Comment is outdated, we don't reference g_browser_process any longer.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:162: system_request_context_ =
system_request_context;
Rename to |request_context_|? We should be able to use any request context as
long as it can make requests :)

https://codereview.chromium.org/12310100/diff/7001/chrome/test/base/testing_b...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/test/base/testing_b...
chrome/test/base/testing_browser_process.cc:384:
chromeos::DeviceSettingsService::Get()->Shutdown();
Oh the joy of Chrome OS singletons... I don't have a better idea either.

[jochen (gone - plz use gerrit), 2013-02-26 14:23:04]

lgtm

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:269:
g_browser_process->system_request_context());
why are you using g_browser_process and not browser_process_?

[Joao da Silva, 2013-02-26 16:09:00]

Thanks for the comments!

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:848: // the main BrowserThreads are
ready. All of that is ready at this stage.
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> This comment could be clearer. In particular, you could mention that:
> * We need to initialize early because local_state initialization needs policy.
> * Policy updating (i.e. network fetches, watching the disk) can only be done
> once threads are up, justifying the init call here.

Done.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:269:
g_browser_process->system_request_context());
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Btw. Any plans to rewrite this test as a proper browser_test?

MSK? :-)

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:269:
g_browser_process->system_request_context());
On 2013/02/26 14:23:04, jochen wrote:
> why are you using g_browser_process and not browser_process_?

For no good reason, changed.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/asyn...
File chrome/browser/policy/async_policy_provider.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/asyn...
chrome/browser/policy/async_policy_provider.cc:48: DCHECK(post) <<
"AsyncPolicyProvider::Init() called too early!";
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Or too late. This could be a CHECK() I guess?

Rephrased.

The DCHECK is enough to warn us if the initialization is busted (e.g. the
browser_tests running in the debug bots should be enough). If this happens in a
release build then I'd rather not crash the browser; the task will just be
silently discarded, and no policies will be served.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (left):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:118: if
(!device_management_service_.get()) {
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Why the conditional here?

Because of SetDeviceManagementServiceForTesting.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:109: // assume that
g_browser_process is fully initialized yet, nor the browser
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Comment is outdated, we don't reference g_browser_process any longer.

Done.

https://codereview.chromium.org/12310100/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:162: system_request_context_ =
system_request_context;
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Rename to |request_context_|? We should be able to use any request context as
> long as it can make requests :)

Done.

https://codereview.chromium.org/12310100/diff/7001/chrome/test/base/testing_b...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/12310100/diff/7001/chrome/test/base/testing_b...
chrome/test/base/testing_browser_process.cc:384:
chromeos::DeviceSettingsService::Get()->Shutdown();
On 2013/02/26 14:22:00, Mattias Nissler wrote:
> Oh the joy of Chrome OS singletons... I don't have a better idea either.

Yea, I don't like this :-/

I don't know of a better way to avoid breaking unsuspecting tests due to this.

[commit-bot: I haz the power, 2013-02-26 16:09:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12310100/13001

[commit-bot: I haz the power, 2013-02-26 17:07:06]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Joao da Silva, 2013-02-26 18:20:10]

@Mattias: changed chrome/browser/policy/user_policy_signin_service_unittest.cc
to use an IO instead of a UI loop, this fixes a CHECK.

[commit-bot: I haz the power, 2013-02-26 18:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12310100/3004

[commit-bot: I haz the power, 2013-02-26 19:27:44]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-26 19:33:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12310100/3004

[commit-bot: I haz the power, 2013-02-26 22:32:36]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-27 07:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12310100/3004

[commit-bot: I haz the power, 2013-02-27 09:49:42]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-27 12:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12310100/3008

[commit-bot: I haz the power, 2013-02-27 16:15:22]

Change committed as 184967