New issue
Advanced search Search tips

Issue 806538 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Policy providers aren't loaded early enough, resulting in some policies being missed.

Project Member Reported by sky@chromium.org, Jan 27 2018

Issue description

This is a regression from a recent patch attempting to move loading of local state earlier.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/615e458c5a11aa5c351c31875b91b0adf80d8577

commit 615e458c5a11aa5c351c31875b91b0adf80d8577
Author: Scott Violet <sky@chromium.org>
Date: Sat Jan 27 21:53:30 2018

Changes ordering so that ConfigurationPolicyProviders are set earlier

Specifically this converts back to setting the
ConfigurationPolicyProviders *before* the PolicyService is created.

There is a subtle difference between this code and the way the
code was before my patch. Specifically previously the code did
the following:

1. ChromeBrowserPolicyConnector created, which added a bunch of providers
   from constructor.
2. BrowserPolicyConnectorBase::GetPolicyService is called, which creates
   the service and pushes the providers to the PolicyServiceImpl
3. local state/field-trials created.
4. BrowserPolicyConnector::InitInternal was called, which in turn
   called Init() on the providers.

With this patch, order is now:

1. ChromeBrowserPolicyConnector constructor does very little
2. ChromeBrowserPolicyConnector::InitPolicyProviders is called, which
   creates the providers.
3. BrowserPolicyConnectorBase::SetPolicyProviders is called, which
   creates the PolicyService and calls Init() on all the providers and
   sets is_initialized_.
4. BrowserPolicyConnectorBase::GetPolicyService is called, creating
   the PolicyService.
5. local state/field-trials created.
6. BrowserPolicyConnector::InitInternal is called.

Key differences:
. ConfigurationPolicyProvider::Init() is called earlier.
. Had to remove DCHECK in BrowserPolicyConnector::InitInternal() because
  SetProviders sets is_initialized_.


BUG= 806538 
TEST=none

Change-Id: I0981419cb1734208789639cc1713e49ac8e901c7
Reviewed-on: https://chromium-review.googlesource.com/889500
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532253}
[modify] https://crrev.com/615e458c5a11aa5c351c31875b91b0adf80d8577/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/615e458c5a11aa5c351c31875b91b0adf80d8577/chrome/browser/policy/chrome_browser_policy_connector.cc
[modify] https://crrev.com/615e458c5a11aa5c351c31875b91b0adf80d8577/chrome/browser/policy/chrome_browser_policy_connector.h
[modify] https://crrev.com/615e458c5a11aa5c351c31875b91b0adf80d8577/components/policy/core/browser/browser_policy_connector.cc

Comment 2 by sky@chromium.org, Jan 28 2018

Labels: Merge-Request-65
I'm requesting a merge to 65 as this breaks enterprise policies.
Project Member

Comment 3 by sheriffbot@chromium.org, Jan 29 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 29 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e6e8f4bdc7b84c018386185282efa5be4029c02

commit 4e6e8f4bdc7b84c018386185282efa5be4029c02
Author: Scott Violet <sky@chromium.org>
Date: Mon Jan 29 20:12:01 2018

merge: Changes ordering so that ConfigurationPolicyProviders are set earlier

Specifically this converts back to setting the
ConfigurationPolicyProviders *before* the PolicyService is created.

There is a subtle difference between this code and the way the
code was before my patch. Specifically previously the code did
the following:

1. ChromeBrowserPolicyConnector created, which added a bunch of providers
   from constructor.
2. BrowserPolicyConnectorBase::GetPolicyService is called, which creates
   the service and pushes the providers to the PolicyServiceImpl
3. local state/field-trials created.
4. BrowserPolicyConnector::InitInternal was called, which in turn
   called Init() on the providers.

With this patch, order is now:

1. ChromeBrowserPolicyConnector constructor does very little
2. ChromeBrowserPolicyConnector::InitPolicyProviders is called, which
   creates the providers.
3. BrowserPolicyConnectorBase::SetPolicyProviders is called, which
   creates the PolicyService and calls Init() on all the providers and
   sets is_initialized_.
4. BrowserPolicyConnectorBase::GetPolicyService is called, creating
   the PolicyService.
5. local state/field-trials created.
6. BrowserPolicyConnector::InitInternal is called.

Key differences:
. ConfigurationPolicyProvider::Init() is called earlier.
. Had to remove DCHECK in BrowserPolicyConnector::InitInternal() because
  SetProviders sets is_initialized_.


BUG= 806538 
TEST=none

Change-Id: I0981419cb1734208789639cc1713e49ac8e901c7
Reviewed-on: https://chromium-review.googlesource.com/889500
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#532253}(cherry picked from commit 615e458c5a11aa5c351c31875b91b0adf80d8577)
Reviewed-on: https://chromium-review.googlesource.com/891563
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#152}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/4e6e8f4bdc7b84c018386185282efa5be4029c02/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/4e6e8f4bdc7b84c018386185282efa5be4029c02/chrome/browser/policy/chrome_browser_policy_connector.cc
[modify] https://crrev.com/4e6e8f4bdc7b84c018386185282efa5be4029c02/chrome/browser/policy/chrome_browser_policy_connector.h
[modify] https://crrev.com/4e6e8f4bdc7b84c018386185282efa5be4029c02/components/policy/core/browser/browser_policy_connector.cc

Comment 5 by sky@chromium.org, Jan 30 2018

Issue 807367 has been merged into this issue.

Comment 6 by sky@chromium.org, Jan 30 2018

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42947e87ce8c115820aa91d7915dc70ee2821721

commit 42947e87ce8c115820aa91d7915dc70ee2821721
Author: Owen Min <zmin@chromium.org>
Date: Tue Jan 30 23:36:45 2018

Add a browser test to verify the policy value in prefs.

This test verifies the policy value is read and copied to local_state properly in the early
stage of browser launch on Windows.

It's a test for the patch: crrev.com/615e458c5a11aa5c351c31875b91b0adf80d8577.

Bug:  806538 
Change-Id: I5b150f7429f968312692123d5d40525c3ddbd9c6
Reviewed-on: https://chromium-review.googlesource.com/892043
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533083}
[add] https://crrev.com/42947e87ce8c115820aa91d7915dc70ee2821721/chrome/browser/policy/policy_initialization_browsertest.cc
[modify] https://crrev.com/42947e87ce8c115820aa91d7915dc70ee2821721/chrome/test/BUILD.gn

Verified no issue as tested in ChromeOS M67.0.3383.0 10539.0.0 dev paine.

Need to be verified in other platforms.
Labels: cros-verified

Sign in to add a comment