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.

Sign up for GitHub

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

Jump to bottom

Ensure only one IdentityManagerFactory instance gets created #15659

Closed
mariospr opened this issue May 6, 2021 · 1 comment · Fixed by brave/brave-core#8731
Closed

Ensure only one IdentityManagerFactory instance gets created #15659

mariospr opened this issue May 6, 2021 · 1 comment · Fixed by brave/brave-core#8731
Assignees
@mariospr
Labels
dev-concern OS/Desktop QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/exclude
Milestone
1.26.x - Release

Comments

@mariospr
Copy link
Contributor

mariospr commented May 6, 2021

Description

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 brave/brave-core#7480 we're actually creating two instances for Brave: the BraveIdentityManagerFactory (subclass of IdentityManagerFactory) 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 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.

Therefore, we need to ensure we have one IdentityManagerFactory instance only, by removing a few parts of the infrastructure added with PR brave/brave-core#7480 and simply leveraging chromium_src overrides to implement the changes we need for the IdentityManager: to empty implementations of GetAccountsInCookieJar() and GetPrimaryAccountMutator().

Brave version (brave://version info)

This has been detected via in Chromium 92.0.4496.3 because of a DCHECK introduced there that uncovered the situation, but the code has been like that since Jan 11, without causing any visible problem so far.

Miscellaneous Information:

For QA-related purposes, please use the Test Plan from brave/brave-core#7480
@mariospr mariospr self-assigned this May 6, 2021
@mariospr mariospr changed the title Ensure that only one IdentityManagerFactory instance gets created Ensure only one IdentityManagerFactory instance gets created May 6, 2021
@mariospr mariospr mentioned this issue May 6, 2021
Merged
24 tasks
mariospr added a commit to brave/brave-core that referenced this issue May 6, 2021
@mariospr
Ensure only one IdentityManagerFactory instance gets created
c6adb01 
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 added a commit to brave/brave-core that referenced this issue May 6, 2021
@mariospr
Ensure only one IdentityManagerFactory instance gets created
a9becf3 
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 mariospr mentioned this issue May 12, 2021
Closed
mariospr added a commit to brave/brave-core that referenced this issue May 12, 2021
@mariospr
Ensure only one IdentityManagerFactory instance gets created
d356d1f 
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 mariospr added this to the 1.26.x - Nightly milestone May 14, 2021
@stephendonner
Copy link

stephendonner commented May 18, 2021 •
edited by btlechowski
Loading

Verified PASSED using the testplan from brave/brave-core#7480 with build

Brave 1.26.22 Chromium: 91.0.4472.57 (Official Build) nightly (x86_64)
Revision e3443317fa07f1e9997e4a9c738eddfefc3c0292-refs/branch-heads/4472_54@{#6}
OS macOS Version 11.3.1 (Build 20E241)

Steps:

  1. opened a new tab with brave://net-export/
  2. started network logging with high available option: ( Include raw bytes (will include cookies and credentials))
  3. enabled Sync, ensured my device showed up
  4. stopped network logging
  5. analyzed the log (suggested https://netlog-viewer.appspot.com/#import)
  6. ensured there was no activity to accounts.google.com
example example example example example example
Screen Shot 2021-05-17 at 5 05 24 PM Screen Shot 2021-05-17 at 5 05 55 PM Screen Shot 2021-05-17 at 5 06 49 PM Screen Shot 2021-05-17 at 5 07 27 PM Screen Shot 2021-05-17 at 5 07 32 PM Screen Shot 2021-05-17 at 5 07 37 PM

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.26.47 Chromium: 91.0.4472.77 (Official Build) beta (64-bit)
--- | ---
Revision | 1cecd5c8a856bc2a5adda436e7b84d8d21b339b6-refs/branch-heads/4472@{#1246}
OS | Windows 10 OS Version 2009 (Build 19042.1023)
Example Example Example
Screenshot 2021-06-01 150739 image image

Verification passed on

Brave 1.26.45 Chromium: 91.0.4472.77 (Official Build) beta (64-bit)
Revision 1cecd5c8a856bc2a5adda436e7b84d8d21b339b6-refs/branch-heads/4472@{#1246}
OS Ubuntu 18.04 LTS
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mariospr mariospr

Labels
dev-concern OS/Desktop QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/exclude
Projects
None yet
Milestone
1.26.x - Release
Development

Successfully merging a pull request may close this issue.

Ensure only one IdentityManagerFactory instance gets created brave/brave-core
5 participants
@stephendonner @mariospr @kjozwiak @LaurenWags @btlechowski