Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure only one IdentityManagerFactory instance gets created #8731

Merged
merged 1 commit into from
May 12, 2021

Conversation

mariospr
Copy link
Contributor

@mariospr mariospr commented May 6, 2021

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

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

See Test Plan from #7480

@mariospr
Copy link
Contributor Author

@AlexeyBarabash pointed out to me on Slack today that this is not entirely correct because I am disabling IdentityManager::GetAccountsInCookieJar() globally instead of doing it for the Brave Sync use case only, which could break some extensions like Google Keep, as described in this old issue: brave/brave-browser#3650

Still we both agree on that having 2 factories around is not a good idea so perhaps we could add one more commit on top of this PR adding some more code (maybe in the chromium_src overrides) so that we decide when to use upstream's implementation of IdentityManager::GetAccountsInCookieJar() or to disable it, depending on the "Is Signing Allowed" preference, so that would probably be the next step here (instead of coming back to having 2 factories).

However, while discussing this we realized that extensions like Google Keep are currently broken with similar symptoms to the ones described in brave/brave-browser#3650, meaning that we can't really progress on this PR until that problem is fixed first, so I filed a new issue for it: brave/brave-browser#15754

@iefremov As discussed with @AlexeyBarabash , for now I think I'm going to keep the patch from #8731 applied in the cr92 branch, pending to know what to do with brave/brave-browser#15754 since having the 2 factories that we still have in master won't do any good anyway until that is solved.

Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

* 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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be components/signin/public/identity_manager/identity_manager.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment on #include. Otherwise, chromium_src and patches LGTM.

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
@mariospr
Copy link
Contributor Author

Failures unrelated to this PR, merging...

@mariospr mariospr merged commit 0266ab7 into master May 12, 2021
@mariospr mariospr deleted the issues/15659 branch May 12, 2021 23:24
@mariospr mariospr added this to the 1.26.x - Nightly milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure only one IdentityManagerFactory instance gets created
3 participants