CL here: https://chromium-review.googlesource.com/c/chromium/src/+/1346409

@sdefresne @blundell I'm having some trouble with this migration, could help some clarifications to the 2 questions below, if you have some time.

First of all, it seems that I need to check whether the preference exists before accessing it, otherwise I might sometimes hit an "Trying to read an unregistered pref: google.services.username_pattern" from the unit tests.

Does that sound right to you? Or is it perhaps a symptom of me doing something wrong?


Second, it seems that depending on where the conversion is done, I need to extract the relevant |PrefService| from different places (i.e. the profile or the browser process). Fortunately there are only two places where such conversion needs to happen:

  * chrome/browser/signin/signin_ui_util.cc: I can use profile->GetPrefs() 

  * chrome/browser/ui/webui/signin/signin_utils_desktop.cc: I need to use |g_browser_process->local_state()| here, or I will InlineLoginUIBrowserTest.CanOfferUsernameNotAllowed failing like in https://bit.ly/2OWWbcc because of the preference not being registered with the profile in //c/b/ui/webui/signin/inline_login_ui_browsertest.cc, but with the browser process' local state instead (see [1]).

Could you please double-check whether this is correct?


FWIW, I've uploaded a new patch set to CL1346409 with these considerations in mind that I believe will pass in all the bots, but I'm not 100% sure about the approach, so better safe than sorry.

Thanks!


[1]https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc?type=cs&g=0&l=317
[2]https://chromium-review.googlesource.com/c/chromium/src/+/1346409/4

There is multiple question here.

So, regarding the preference store, you should not use profile->GetPrefs(), but always use g_browser_process->local_state(). This is because they do not return the same store (one is per device, the other is per profile).

Regarding unregistered preference, the implementation in SigninManager explicitly states that during unit tests, the local state may be null when calling SigninManager::Initialize(). In that case, the preference will not be registered. Thus, you are not doing something wrong. However, I think the tests may be wrong there, and the local state preference store should not be null.

I think we should try to fix the tests to always pass a non-null local store preference to SigninManager::Initialize() during tests. This would allow to remove the check in the implementation of SigninManager and to land your CL without checking whether the preference is registered.

I think the files that invoke SigninManagerBase::Initialize() with a nullptr are located in:
- components/signin/core/browser/account_reconcilor_unittest.cc
- chrome/browser/sync/sync_ui_util_unittest.cc
- chrome/browser/ui/sync/one_click_signin_sync_starter_unittest.cc
- chrome/browser/signin/fake_signin_manager_builder.cc

So I think we should try to fix those tests in a preliminary CL (a good way to ensure this is to change the code in SigninManager::Initialize to DCHECK that the local_state parameter is non-null) and then as a followup to unconditionally access the preference.

Hi Mario,

Wrt the first question, we should make sure that the pref is registered in the relevant unittests. How many/which tests are failing?

Wrt the second question, from a quick glance it looks like the pref is intended to be accessed via |local_state|. Can you expand on your issue with signin_ui_util.cc?

Thanks!

(Note: Sylvain's comment answers my first question :).

@blundell @sdefresne Many thanks for your replies here. I've been working on this this morning and I think I've made some progress, although I'm having trouble with some tests such as Test/PeopleHandlerDiceUnifiedConsentTest.* or AdvancedProtectionStatusManagerTest.*, which keep crashing on the |DCHECK(local_state != nullptr)| I added following Sylvain's suggestion, even after using g_browser_process->local_state() when calling SigninManager::Initialize() from BuildFakeSigninManagerForTesting (see [1]):

    [1275:1275:1123/124247.244227:276531029152:FATAL:signin_manager.cc(269)] Check failed: local_state != nullptr. 
    #0 0x7fafed669aef base::debug::StackTrace::StackTrace()
    #1 0x7fafed540859 logging::LogMessage::~LogMessage()
    #2 0x556dd2e423c1 SigninManager::Initialize()
    #3 0x556dd3fbd5c4 BuildFakeSigninManagerForTesting()
    #4 0x556dcfcac052 _ZN4base8internal7InvokerINS0_9BindStateIPFNSt3__110unique_ptrI12KeyedServiceNS3_14default_deleteIS5_EEEEPN7content14BrowserContextEEJEEESC_E3RunEPNS0_13BindStateBaseESB_
    #5 0x7fafe84abfee _ZN4base8internal7InvokerINS0_9BindStateIZN33BrowserContextKeyedServiceFactory17SetTestingFactoryEPN7content14BrowserContextENS_17RepeatingCallbackIFNSt3__110unique_ptrI12KeyedServiceNS8_14default_deleteISA_EEEES6_EEEE3$_0JSF_EEEFSD_PNS_16SupportsUserDataEEE3RunEPNS0_13BindStateBaseESJ_
    #6 0x7fafe8bad288 KeyedServiceFactory::GetServiceForContext()
    #7 0x556dd27e5e8a IdentityManagerWrapper::IdentityManagerWrapper()
    #8 0x556dd27e5e3a IdentityManagerFactory::BuildServiceInstanceFor()
    #9 0x7fafe84abec5 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
    #10 0x7fafe8bad168 KeyedServiceFactory::GetServiceForContext()
    #11 0x556dd27ba869 RendererUpdater::RendererUpdater()
    #12 0x556dd27bbe72 RendererUpdaterFactory::BuildServiceInstanceFor()
    #13 0x7fafe84abec5 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
    #14 0x7fafe8bad168 KeyedServiceFactory::GetServiceForContext()
    #15 0x7fafe8bac199 DependencyManager::CreateContextServices()
    #16 0x7fafe84ab218 BrowserContextDependencyManager::DoCreateBrowserContextServices()
    #17 0x556dd21dba53 TestingProfile::Init()
    #18 0x556dd21dc1d1 TestingProfile::TestingProfile()
    #19 0x556dd21deac6 TestingProfile::Builder::Build()
    #20 0x556dd3fbd843 IdentityTestEnvironmentProfileAdaptor::CreateProfileForIdentityTestEnvironment()
    #21 0x556dd3fbd764 IdentityTestEnvironmentProfileAdaptor::CreateProfileForIdentityTestEnvironment()
    #22 0x556dd3fbd6d4 IdentityTestEnvironmentProfileAdaptor::CreateProfileForIdentityTestEnvironment()
    #23 0x556dd08799a8 settings::PeopleHandlerDiceUnifiedConsentTest_StoredAccountsList_Test::TestBody()
    #24 0x556dd18119c1 testing::Test::Run()
    #25 0x556dd181264f testing::TestInfo::Run()
    #26 0x556dd1812b77 testing::TestCase::Run()
    #27 0x556dd181f357 testing::internal::UnitTestImpl::RunAllTests()
    #28 0x556dd181eecd testing::UnitTest::Run()
    #29 0x556dd21ef471 base::TestSuite::Run()
    #30 0x556dd21f1ded base::(anonymous namespace)::LaunchUnitTestsInternal()
    #31 0x556dd21f1c41 base::LaunchUnitTests()
    #32 0x556dd21e26ca main
    #33 0x7fafdfd3309b __libc_start_main
    #34 0x556dcfc662aa _start


As you can see, those tests use |IdentityTestEnvironmentProfileAdaptor|, so maybe there's something that needs to be done (make sure a PrefsService is registered in the browser process?) there so that all those type of tests work as expected in a world where SigninManager::Initialize() does not accept a null local_state?

FWIW, I've been trying an approach similar to the one in user_policy_signin_service_unittest.cc (whose tests pass with the patch) of calling |TestingBrowserProcess::SetLocalState()| on Setup and TearDown, but that hasn't done the job yet.

Any hints?


> Wrt the second question, from a quick glance it looks like the pref is intended to be accessed via |local_state|. Can you expand on your issue with signin_ui_util.cc?

Replying to this, the problem was that the preference path was not found in some cases, but after Sylvain's comments I think I was hiding a different issue with checking for its existence, as it should be there in any case.

[1]https://cs.chromium.org/chromium/src/chrome/browser/signin/fake_signin_manager_builder.cc?sq=package:chromium&dr=CSs&g=0&l=23

Quick update. It seems I was looking into the wrong place and I managed to fix the situation in Test/PeopleHandlerDiceUnifiedConsentTest.StoredAccountsList/3 by making sure that the local_state for the process is set and removed on Setup / TearDown.

Basically this diff on top of my patch did it:

     #if BUILDFLAG(ENABLE_DICE_SUPPORT)
     class PeopleHandlerDiceUnifiedConsentTest
    -    : public ::testing::TestWithParam<std::tuple<bool, bool>> {};
    +    : public ::testing::TestWithParam<std::tuple<bool, bool>> {
    +
    +  void SetUp() override {
    +    local_state_.reset(new TestingPrefServiceSimple);
    +    RegisterLocalState(local_state_->registry());
    +    TestingBrowserProcess::GetGlobal()->SetLocalState(local_state_.get());
    +  }
    +
    +  void TearDown() override {
    +    TestingBrowserProcess::GetGlobal()->SetLocalState(nullptr);
    +    local_state_.reset();
    +  }
    +
    +std::unique_ptr<TestingPrefServiceSimple> local_state_;
    +};


I'll continue after lunch trying to fix other cases such as AdvancedProtectionStatusManagertest.*...

I think I've might have cracked this one... filed crbug.com/908121 to do those fixes from there, will propose a CL soon.

PS: Today I found out about ScopedTestingLocalState, which seems like a better way to fix these tests

As explained in https://bugs.chromium.org/p/chromium/issues/detail?id=908121#c2, we have to unblock this bug from 908121 because there are simply way too many unit tests (3000+) to fix if we make SigninManager::Initialize() to require a non-null local_state.

Instead, we will implement a "transitional" identity::IsUsernameAllowedByPatternFromPrefs() method that checks whether a preference exists before trying to read it and use that one to finish the CL here, and work in parallel in crbug.com/908121 but without blocking anything.

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

commit 521fe8122747e5c4cbb13c8c977f98153f921363
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Nov 27 18:08:35 2018

New transitional function identity::LegacyIsUsernameAllowedByPatternFromPrefs()

As explained in crbug.com/908121, we can't fix SigninManager::Initialize() to
require a non-null local_state just yet as that would require manually fixing
thousands of tests, so we're adding this transitional function to unblock work
on  crbug.com/906085  allowing to migrate from SigninManager::IsAllowedUsername
right away without depending on those other changes.

So, this patch implements a new helper function that will behave more similar
to SigninManager::IsAllowedUsername on that it will only check the username
against the specified pattern if (1) there's a non-null local state passed
and (2) the pattern is registered and a value for it has been set, returning
true otherwise.

We also include an unit test for this helper function, to help with future
refactorings if they happen.

Bug:  906085 
Change-Id: Idef663d07354fb086a5bc00b1ab447f1811c344d
Reviewed-on: https://chromium-review.googlesource.com/c/1350895
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611190}
[modify] https://crrev.com/521fe8122747e5c4cbb13c8c977f98153f921363/components/signin/core/browser/BUILD.gn
[modify] https://crrev.com/521fe8122747e5c4cbb13c8c977f98153f921363/components/signin/core/browser/identity_utils.cc
[modify] https://crrev.com/521fe8122747e5c4cbb13c8c977f98153f921363/components/signin/core/browser/identity_utils.h
[modify] https://crrev.com/521fe8122747e5c4cbb13c8c977f98153f921363/components/signin/core/browser/identity_utils_unittest.cc

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

commit 198eca95746e419a01cc619e8f063b84a2cbdedc
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Nov 28 15:45:09 2018

Migrate consumers of SigninManager::IsAllowedUsername()

Use identity::IsUsernameAllowedByPattern() instead so that we can migrate
those consumers entirely to the IdentityManager in follow-up patches.

Also, in order to make it explicit that this API should not be used any
more, make SigninManager::IsAllowedUsername() a private method and friend
it with the relevant test cases from SigninManagerTest, so that it can
only be used from there. And while at it, also remove some stale lines
"friending" SigninManager with test cases that no longer exist.

Bug:  890794 ,  890815 ,  906085 
Change-Id: Id8a72b0efd89818e02c6d1e73aac1451207b9625
Reviewed-on: https://chromium-review.googlesource.com/c/1346409
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#611704}
[modify] https://crrev.com/198eca95746e419a01cc619e8f063b84a2cbdedc/chrome/browser/signin/signin_ui_util.cc
[modify] https://crrev.com/198eca95746e419a01cc619e8f063b84a2cbdedc/chrome/browser/ui/webui/signin/signin_utils_desktop.cc
[modify] https://crrev.com/198eca95746e419a01cc619e8f063b84a2cbdedc/components/signin/core/browser/signin_manager.h

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

commit aabacf9ed204501b1766a505753a3a6a41ce503c
Author: Ned Nguyen <nednguyen@google.com>
Date: Wed Nov 28 16:08:37 2018

Revert "Migrate consumers of SigninManager::IsAllowedUsername()"

This reverts commit 198eca95746e419a01cc619e8f063b84a2cbdedc.

Reason for revert: breaking GPU Linux Builder

Original change's description:
> Migrate consumers of SigninManager::IsAllowedUsername()
> 
> Use identity::IsUsernameAllowedByPattern() instead so that we can migrate
> those consumers entirely to the IdentityManager in follow-up patches.
> 
> Also, in order to make it explicit that this API should not be used any
> more, make SigninManager::IsAllowedUsername() a private method and friend
> it with the relevant test cases from SigninManagerTest, so that it can
> only be used from there. And while at it, also remove some stale lines
> "friending" SigninManager with test cases that no longer exist.
> 
> Bug:  890794 ,  890815 ,  906085 
> Change-Id: Id8a72b0efd89818e02c6d1e73aac1451207b9625
> Reviewed-on: https://chromium-review.googlesource.com/c/1346409
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
> Cr-Commit-Position: refs/heads/master@{#611704}

TBR=blundell@chromium.org,msarda@chromium.org,sdefresne@chromium.org,mario@igalia.com

Change-Id: I8172674e32d7671868fa5374a9457abd6413321e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  890794 ,  890815 ,  906085 
Reviewed-on: https://chromium-review.googlesource.com/c/1354121
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#611710}
[modify] https://crrev.com/aabacf9ed204501b1766a505753a3a6a41ce503c/chrome/browser/signin/signin_ui_util.cc
[modify] https://crrev.com/aabacf9ed204501b1766a505753a3a6a41ce503c/chrome/browser/ui/webui/signin/signin_utils_desktop.cc
[modify] https://crrev.com/aabacf9ed204501b1766a505753a3a6a41ce503c/components/signin/core/browser/signin_manager.h

Cl has been reverted.

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

commit 482335f01cab4681a33b8b29bba3d1434b14fd09
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Nov 28 17:48:51 2018

Migrate consumers of SigninManager::IsAllowedUsername()

Use identity::IsUsernameAllowedByPattern() instead so that we can migrate
those consumers entirely to the IdentityManager in follow-up patches.

Also, in order to make it explicit that this API should not be used any
more, make SigninManager::IsAllowedUsername() a private method and friend
it with the relevant test cases from SigninManagerTest, so that it can
only be used from there. And while at it, also remove some stale lines
"friending" SigninManager with test cases that no longer exist.

Bug:  890794 ,  890815 ,  906085 
Change-Id: I2a5be231ff70d3e390aa1718c795aef9d8b255ff
Reviewed-on: https://chromium-review.googlesource.com/c/1353894
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#611764}
[modify] https://crrev.com/482335f01cab4681a33b8b29bba3d1434b14fd09/chrome/browser/signin/signin_ui_util.cc
[modify] https://crrev.com/482335f01cab4681a33b8b29bba3d1434b14fd09/chrome/browser/signin/signin_util.cc
[modify] https://crrev.com/482335f01cab4681a33b8b29bba3d1434b14fd09/chrome/browser/ui/webui/signin/signin_utils_desktop.cc
[modify] https://crrev.com/482335f01cab4681a33b8b29bba3d1434b14fd09/components/signin/core/browser/signin_manager.h

Looks like the newly landed CL is stable now