Fixed redundant creation of Default profile #13929

spylogsster · 2022-06-23T15:04:38Z

Fixed redundant creation of default profile. GetPrimaryUserProfile returns Default browser profile, but not the profile which is currently used by user. If user has deleted Default profile it will be re-created in this case. Better to use LastUsedProfile which returns current profile for the user.

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
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
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:

Start a clean profile
Click on people icon and click on Manage people
Add a new person in Manage people window
Click on Person 2 window and click on people icon, lists Person 1 in the list
In manage people window, click on 3 dots of Person 1 and delete it
Open Person 2 window and click on people icon, still lists Person 1 there. Clicking that will create a new Person 1 window

simonhong · 2022-06-24T02:00:31Z

browser/brave_stats/brave_stats_updater.cc

@@ -120,7 +120,7 @@ BraveStatsUpdater::BraveStatsUpdater(PrefService* pref_service)
 BraveStatsUpdater::~BraveStatsUpdater() {}

 void BraveStatsUpdater::OnProfileAdded(Profile* profile) {
-  if (profile == ProfileManager::GetPrimaryUserProfile()) {


Wow, there is a comment that this method should not be used in desktop.

// WARNING: do not use this function on Desktop platforms (Windows, Mac, // Linux). See https://crbug.com/1264436 for more info. // TODO(https://crbug.com/1264436): restrict this function to Android and // ChromeOS.

Is there any possibility to introduce regressions from this change? cc @bridiver

Wow- nice find, @spylogsster @simonhong. So we were using a bad method this whole time?

I think the re-creation of the deleted folder was the regression 😄

The fundamental problem here is that we're assuming a single profile and there's really no good way to do this. We're comparing it against the P3A data to see if we can drop this entirely, but if we want to collect a few more months of data first I'm hesitant to make any changes that will introduce another variable into the equation

// WARNING: if the profile does not exist, this function creates it // synchronously, causing blocking file I/O. Use GetLastUsedProfileIfLoaded() // to avoid creating the profile synchronously. static Profile* GetLastUsedProfile();

false alarm, it does not recreate the default profile

Changed to use GetLastUsedProfileIfLoaded

it will recreate the default profile if all profiles have been deleted so probably GetLastUsedProfileIfLoaded is the way to go because it should not have any side effects

bsclifton

Changes LGTM! ++

@DJAndries how do things look for you? This would be a great fix to land if everything is OK

DJAndries · 2022-06-29T16:11:09Z

Changes LGTM! ++

@DJAndries how do things look for you? This would be a great fix to land if everything is OK

looks good! will do a smoke test today

DJAndries

The stats updater works OK with the suggested change above.

Also, has anyone seen this stack trace when exiting the browser?

Received signal 11 SEGV_MAPERR 000000000008
#0 0x557394fa16c9 base::debug::CollectStackTrace()
#1 0x557394ebaa93 base::debug::StackTrace::StackTrace()
#2 0x557394fa1181 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f69cddf88e0 (/usr/lib/libc.so.6+0x3e8df)
#4 0x55738fba85e9 base::ObserverList<>::RemoveObserver()
#5 0x557394bfa064 (anonymous namespace)::AdBlockSubscriptionDownloadManagerGetterImpl::OnProfileManagerDestroying()
#6 0x557394b74ad7 ProfileManager::~ProfileManager()
#7 0x557392a77a22 BraveProfileManager::~BraveProfileManager()
#8 0x557392a770ae BraveProfileManager::~BraveProfileManager()
#9 0x5573949e3c68 BrowserProcessImpl::StartTearDown()
#10 0x5573949e272a ChromeBrowserMainParts::PostMainMessageLoopRun()
#11 0x557391680ac1 content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#12 0x55739168299a content::BrowserMainRunnerImpl::Shutdown()
#13 0x55739167e307 content::BrowserMain()
#14 0x55739496d7f5 content::RunBrowserProcessMain()
#15 0x55739496f184 content::ContentMainRunnerImpl::RunBrowser()
#16 0x55739496ec00 content::ContentMainRunnerImpl::Run()
#17 0x55739496c34c content::RunContentProcess()
#18 0x55739496c43e content::ContentMain()
#19 0x55738f9b4233 ChromeMain
#20 0x7f69cdde3290 (/usr/lib/libc.so.6+0x2928f)
#21 0x7f69cdde334a __libc_start_main
#22 0x55738f9b402a _start
  r8: 000030e4005e4120  r9: 0000000000000000 r10: 00007ffd314ed080 r11: 00000000008328cc
 r12: 00007ffd3147e730 r13: 00007ffd3147e750 r14: 0000000000000008 r15: 000030e400b19860
  di: 0000000000000008  si: 000030e400b19860  bp: 00007ffd3147e6a0  bx: 000030e400b19860
  dx: 0000000000000000  ax: 0000000000000000  cx: 0000000000000001  sp: 00007ffd3147e660
  ip: 000055738fba85e9 efl: 0000000000010206 cgf: 002b000000000033 erf: 0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000008
[end of stack trace]

Now, if I open the wallet, and close the browser in less than 30 seconds, the updated last usage time stored in the profile prefs is not persisted.

DJAndries · 2022-06-29T20:46:03Z

browser/brave_stats/brave_stats_updater.cc

@@ -120,7 +120,7 @@ BraveStatsUpdater::BraveStatsUpdater(PrefService* pref_service)
 BraveStatsUpdater::~BraveStatsUpdater() {}

 void BraveStatsUpdater::OnProfileAdded(Profile* profile) {
-  if (profile == ProfileManager::GetPrimaryUserProfile()) {
+  if (profile == ProfileManager::GetLastUsedProfileIfLoaded()) {


Can we change this instance to GetLastUsedProfile instead? GetLastUsedProfileIfLoaded seems to always return a null pointer when called from OnProfileAdded.

@bridiver wdyt?

That's still going to create side effects. Can we compare profile->GetPath() == GetLastUsedProfileDir()

Just smoke tested, there's a bug with this approach. Steps to reproduce (assumes you have two profiles, and a log within the if block):

Open browser, open first profile, close

Open again, open first profile, see the log, close

Open again, open second profile, the log is missing, close

Open again, open second profile, the log now exists

This doesn't happen with GetLastUsedProfile.

DJAndries · 2022-06-29T21:09:44Z

components/brave_referrals/browser/brave_referrals_service.cc

@@ -214,7 +214,7 @@ BraveReferralsService::BraveReferralsService(PrefService* pref_service,
 BraveReferralsService::~BraveReferralsService() {}

 void BraveReferralsService::OnProfileAdded(Profile* profile) {
-  if (profile == ProfileManager::GetPrimaryUserProfile()) {
+  if (profile == ProfileManager::GetLastUsedProfileIfLoaded()) {


The same may apply here.

DJAndries · 2022-06-29T22:00:53Z

Also, has anyone seen this stack trace when exiting the browser?

The crash goes away if I remove OnProfileManagerDestroying from AdBlockSubscriptionDownloadManagerGetter. This does not occur on master. Interesting side effect.

spylogsster · 2022-07-04T18:56:25Z

browser/brave_shields/ad_block_subscription_download_manager_getter.cc

@@ -102,10 +104,11 @@ class AdBlockSubscriptionDownloadManagerGetterImpl
  }

  void OnProfileManagerDestroying() override {
-    g_browser_process->profile_manager()->RemoveObserver(this);
+    profile_manager_->RemoveObserver(this);


this code is called from ProfileManager destructor when g_browser_process->profile_manager() is null already.
cc @antonok-edm

spylogsster · 2022-07-04T19:27:35Z

Also, has anyone seen this stack trace when exiting the browser?

The crash goes away if I remove OnProfileManagerDestroying from AdBlockSubscriptionDownloadManagerGetter. This does not occur on master. Interesting side effect.

fixed

spylogsster requested a review from simonhong as a code owner June 23, 2022 15:04

github-actions bot assigned spylogsster Jun 23, 2022

spylogsster requested review from bridiver and bsclifton June 23, 2022 15:09

spylogsster added this to the 1.42.x - Nightly milestone Jun 23, 2022

simonhong reviewed Jun 24, 2022

View reviewed changes

spylogsster force-pushed the brave-4599 branch from a2bbb20 to 6310d79 Compare June 28, 2022 19:11

bridiver approved these changes Jun 28, 2022

View reviewed changes

simonhong approved these changes Jun 29, 2022

View reviewed changes

spylogsster force-pushed the brave-4599 branch from 0b4b767 to de81077 Compare June 29, 2022 07:12

bsclifton mentioned this pull request Jun 29, 2022

Person 1 profile keeps coming back after being deleted - Cannot delete the 'Default' user profile brave/brave-browser#4599

Closed

bsclifton approved these changes Jun 29, 2022

View reviewed changes

DJAndries requested changes Jun 29, 2022

View reviewed changes

DJAndries reviewed Jun 29, 2022

View reviewed changes

spylogsster force-pushed the brave-4599 branch 6 times, most recently from 2d27d6c to 2ef3f5a Compare July 4, 2022 17:44

spylogsster commented Jul 4, 2022

View reviewed changes

spylogsster force-pushed the brave-4599 branch from bdf63bd to 39be3b2 Compare July 4, 2022 19:03

spylogsster force-pushed the brave-4599 branch 4 times, most recently from be6a5a4 to 9779fba Compare July 11, 2022 10:18

spylogsster force-pushed the brave-4599 branch from 9779fba to 3271756 Compare July 17, 2022 04:27

spylogsster force-pushed the brave-4599 branch from 3271756 to e73b6ab Compare August 18, 2022 17:24

spylogsster force-pushed the brave-4599 branch 2 times, most recently from 64a91f2 to 7106b3b Compare August 31, 2022 05:23

spylogsster force-pushed the brave-4599 branch 5 times, most recently from 6d084ab to fec2cc1 Compare September 13, 2022 15:47

spylogsster force-pushed the brave-4599 branch 2 times, most recently from 5230f52 to d2f164e Compare September 16, 2022 05:59

spylogsster force-pushed the brave-4599 branch 4 times, most recently from 8d327da to 31c05bd Compare October 15, 2022 09:37

spylogsster force-pushed the brave-4599 branch 3 times, most recently from 8929d8e to c3aa619 Compare October 27, 2022 19:26

spylogsster force-pushed the brave-4599 branch from c3aa619 to cceed7f Compare October 28, 2022 12:46

spylogsster added 3 commits November 1, 2022 14:04

Fixed redundant creation of Default profile

df652a7

Using GetLastUsedProfileIfLoaded

a77a4fc

Fix crash in AdBlockSubscriptionDownloadManagerGetterImpl

1f8b1ce

spylogsster force-pushed the brave-4599 branch from cceed7f to 1f8b1ce Compare November 1, 2022 11:04

spylogsster closed this Nov 17, 2022

spylogsster deleted the brave-4599 branch November 17, 2022 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed redundant creation of Default profile #13929

Fixed redundant creation of Default profile #13929

spylogsster commented Jun 23, 2022 •

edited by bsclifton

Loading

simonhong Jun 24, 2022 •

edited

Loading

bsclifton Jun 24, 2022

bridiver Jun 28, 2022

bridiver Jun 28, 2022 •

edited

Loading

DJAndries Jun 28, 2022

spylogsster Jun 28, 2022

bridiver Jun 28, 2022

bsclifton left a comment

DJAndries commented Jun 29, 2022

DJAndries left a comment •

edited

Loading

DJAndries Jun 29, 2022

spylogsster Jun 29, 2022

bridiver Jun 30, 2022

spylogsster Jun 30, 2022

DJAndries Jun 30, 2022

DJAndries Jun 29, 2022

DJAndries commented Jun 29, 2022 •

edited

Loading

spylogsster Jul 4, 2022

spylogsster commented Jul 4, 2022

Fixed redundant creation of Default profile #13929

Fixed redundant creation of Default profile #13929

Conversation

spylogsster commented Jun 23, 2022 • edited by bsclifton Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simonhong Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

DJAndries commented Jun 29, 2022

DJAndries left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

spylogsster commented Jul 4, 2022

spylogsster commented Jun 23, 2022 •

edited by bsclifton

Loading

simonhong Jun 24, 2022 •

edited

Loading

bridiver Jun 28, 2022 •

edited

Loading

DJAndries left a comment •

edited

Loading

DJAndries commented Jun 29, 2022 •

edited

Loading