Skip to content

Commit

Permalink
Ensure only one IdentityManagerFactory instance gets created
Browse files Browse the repository at this point in the history
IdentityManagerFactory is a class meant to be instantiated in the
browser process via base::Singleton, thus it's not expected to have
several instance of them.

However, since PR #7480[1] we're actually creating two instances for
Brave: the BraveIdentityManagerFactory (subclass) instanced from
ProfileSyncServiceFactory as part of a DependsOn() statement, and
another one for every other place in the code (68 callpoints at this
time) using DependsOn(IdentityManagerFactory::GetInstance()) to
initialize different services.

This hasn't rang any alarm until Chromium 92.0.4496.3, but in that
release some changes were made[2] including the introduction of the
IdentityManagerProvider class, which now DCHECKs for having only
one instance of IdentityManagerFactory created in the browser:

  void SetIdentityManagerProvider(const IdentityManagerProvider& provider) {
    IdentityManagerProvider& instance = GetIdentityManagerProvider();
    DCHECK(!instance);
    instance = provider;
  }

...meaning that we will get a crash on startup and all the unit and
browser tests crashing on DCHECK-enabled builds, revealing this issue
that we didn't know we had until now.

This patch, makes changes to ensure we have one IdentityManagerFactory
instance only, by removing a few parts of the infrastructure added with
PR #7480[1] and simply leveraging chromium_src overrides to implement
the changes we need for the IdentityManager: to empty implementations
of GetAccountsInCookieJar() and GetPrimaryAccountMutator().

[1] #7480
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2824129

Resolves brave/brave-browser#15659
  • Loading branch information
mariospr committed May 6, 2021
1 parent 06042dc commit a9becf3
Show file tree
Hide file tree
Showing 17 changed files with 42 additions and 328 deletions.
84 changes: 0 additions & 84 deletions browser/signin/brave_identity_manager_factory.cc

This file was deleted.

42 changes: 0 additions & 42 deletions browser/signin/brave_identity_manager_factory.h

This file was deleted.

13 changes: 0 additions & 13 deletions browser/signin/sources.gni

This file was deleted.

3 changes: 0 additions & 3 deletions browser/sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import("//brave/browser/ipfs/sources.gni")
import("//brave/browser/new_tab/sources.gni")
import("//brave/browser/permissions/sources.gni")
import("//brave/browser/search_engines/sources.gni")
import("//brave/browser/signin/sources.gni")
import("//brave/browser/speedreader/sources.gni")
import("//brave/browser/themes/sources.gni")
import("//brave/chromium_src/chrome/browser/prefs/sources.gni")
Expand Down Expand Up @@ -319,7 +318,6 @@ brave_chrome_browser_sources += brave_browser_ipfs_sources
brave_chrome_browser_sources += brave_browser_new_tab_sources
brave_chrome_browser_sources += brave_browser_permissions_sources
brave_chrome_browser_sources += brave_browser_search_engines_sources
brave_chrome_browser_sources += brave_browser_signin_sources
brave_chrome_browser_sources += brave_browser_speedreader_sources
brave_chrome_browser_sources += brave_browser_themes_sources
brave_chrome_browser_sources += brave_browser_wallet_sources
Expand All @@ -339,7 +337,6 @@ brave_chrome_browser_deps += brave_browser_ipfs_deps
brave_chrome_browser_deps += brave_browser_new_tab_deps
brave_chrome_browser_deps += brave_browser_permissions_deps
brave_chrome_browser_deps += brave_browser_search_engines_deps
brave_chrome_browser_deps += brave_browser_signin_deps
brave_chrome_browser_deps += brave_browser_speedreader_deps
brave_chrome_browser_deps += brave_browser_themes_deps
brave_chrome_browser_deps += brave_browser_wallet_deps
Expand Down
20 changes: 0 additions & 20 deletions chromium_src/chrome/browser/signin/identity_manager_factory.h

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,10 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/signin/brave_identity_manager_factory.h"
#include "brave/browser/sync/brave_profile_sync_service_delegate.h"
#include "brave/components/signin/public/identity_manager/brave_identity_manager.h"
#include "brave/components/sync/driver/brave_sync_profile_sync_service.h"
#include "chrome/browser/sync/device_info_sync_service_factory.h"

#define IdentityManagerFactory BraveIdentityManagerFactory

#define BRAVE_BUILD_SERVICE_INSTANCE_FOR \
std::make_unique<syncer::BraveProfileSyncService>( \
std::move(init_params), \
Expand All @@ -20,5 +16,3 @@
#include "../../../../../chrome/browser/sync/profile_sync_service_factory.cc"

#undef BRAVE_BUILD_SERVICE_INSTANCE_FOR

#undef IdentityManagerFactory
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/chromium_src/components/signin/public/identity_manager/identity_manager.h"

#include <utility>
#include <vector>

#define GetAccountsInCookieJar GetAccountsInCookieJar_Unused
#include "../../../../../../components/signin/public/identity_manager/identity_manager.cc"
#undef GetAccountsInCookieJar

namespace signin {

AccountsInCookieJarInfo IdentityManager::GetAccountsInCookieJar() const {
// accounts_in_cookie_jar_info.accounts_are_fresh must be false,
// see `ProfileSyncService::OnEngineInitialized`
return AccountsInCookieJarInfo(false, std::vector<gaia::ListedAccount>(),
std::vector<gaia::ListedAccount>());
}

} // namespace signin
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_IDENTITY_MANAGER_H_
#define BRAVE_CHROMIUM_SRC_COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_IDENTITY_MANAGER_H_

#define GetAccountsInCookieJar virtual GetAccountsInCookieJar
#define GetAccountsInCookieJar \
GetAccountsInCookieJar_Unused() const; \
AccountsInCookieJarInfo GetAccountsInCookieJar

#include "../../../../../../components/signin/public/identity_manager/identity_manager.h"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,24 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#define BuildIdentityManager BuildIdentityManager_Unused
#define BuildIdentityManagerInitParameters \
BuildIdentityManagerInitParameters_ChromiumImpl
#include "../../../../../../components/signin/public/identity_manager/identity_manager_builder.cc"
#undef BuildIdentityManagerInitParameters
#undef BuildIdentityManager

#include "brave/components/signin/internal/identity_manager/brave_primary_account_mutator_impl.h"
#include "brave/components/signin/public/identity_manager/brave_identity_manager.h"
#include "components/signin/public/identity_manager/identity_manager.h"

namespace signin {

namespace {

IdentityManager::InitParameters BuildBraveIdentityManagerInitParameters(
IdentityManager::InitParameters BuildIdentityManagerInitParameters(
IdentityManagerBuildParams* params) {
IdentityManager::InitParameters init_params =
BuildIdentityManagerInitParameters(params);
BuildIdentityManagerInitParameters_ChromiumImpl(params);

init_params.primary_account_mutator =
std::make_unique<BravePrimaryAccountMutatorImpl>(
Expand All @@ -29,10 +34,10 @@ IdentityManager::InitParameters BuildBraveIdentityManagerInitParameters(

} // namespace

std::unique_ptr<BraveIdentityManager> BuildBraveIdentityManager(
std::unique_ptr<IdentityManager> BuildIdentityManager(
IdentityManagerBuildParams* params) {
return std::make_unique<BraveIdentityManager>(
BuildBraveIdentityManagerInitParameters(params));
return std::make_unique<IdentityManager>(
BuildIdentityManagerInitParameters(params));
}

} // namespace signin

This file was deleted.

This file was deleted.

41 changes: 0 additions & 41 deletions components/signin/public/identity_manager/brave_identity_manager.h

This file was deleted.

Loading

0 comments on commit a9becf3

Please sign in to comment.