New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 803643 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocked on: View detail
issue 88586
issue 812636
issue 820057



Sign in to add a comment

Deleting a Chrome profile doesn't delete its user data (such as bookmarks)

Reported by 93m4qau...@gmail.com, Jan 18 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce the problem:
1. Open Chrome with just one profile.
2. Go to chrome://settings and add a scrap profile (be sure to create a desktop shortcut for the new profile).
3. Go to Google Maps and build up history.
4. Bookmark several pages, such as google.com, chromium.org, bugs.chromium.org, maps.google.com, and keep.google.com.
5. Right-click the desktop shortcut for the scrap profile and click Properties.
6. Select the shortcut target and press Ctrl+C to copy it to the clipboard.
7. Go to chrome://settings > Manage people and delete the scrap profile.
8. Right-click the desktop, click New, and click Shortcut.
9. Specify the target you copied to the clipboard as the new shortcut's target.
8. Double-click the new shortcut and open Chrome to find that the bookmarks and history were actually never deleted. (If you are using Chrome stable, it will actually crash when you do this, but this is fixed in Chrome canary.)

What is the expected behavior?
Since the scrap profile has been "deleted", its bookmarks, history, and other user data cannot be recovered simply by creating and clicking on a shortcut pointing to it.

What went wrong?
Simply by creating and clicking on a shortcut pointing to the "deleted" scrap profile, you can instantly pull it back up and see bookmarks, history, and other user data - perhaps even passwords and credit card numbers.

Did this work before? No 

Chrome version: 63.0.3239.132  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: 

When you "delete" a Chrome profile, it should be deleted, not hidden.
 
Components: UI>Browser>Profiles
Labels: Needs-Triage-M63
Cc: dullweber@chromium.org
+dullweber@, FYI.
The Privacy label should be added to this bug.
Cc: msramek@chromium.org
Components: Privacy
Thanks for reporting this bug.

When you delete a profile, it is marked for deletion, all windows are closed and the profile directory will be deleted when you close Chrome. There are technical reasons that prevent deleting these files earlier.
Of course it shouldn't be possible to load a deleted profile again. 
There is code that should prevent loading a deleted profile but it seems that your approach circumvents this somehow. I will investigate why this is happening.

https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.cc?l=549&rcl=f6d7a66d709fe816bbe0619456c01e3c867d9551
Blockedon: 88586
When a profile is deleted we delete everything that is covered by Clear Browsing Data like history, cookies, passwords and autofill but not bookmarks. https://crbug.com/88586 seems to be the reason that we can't remove the profile earlier

We should probably remove bookmarks as well.
The check in CreateProfileAsnyc doesn't apply because  and prevent that a windows for the profile can be opened.
forgot to complete the last sentence:

The check in CreateProfileAsnyc doesn't apply because the Profile object is not removed before closing Chrome. We should prevent that a windows for the profile can be opened.

Cc: -dullweber@chromium.org
Owner: dullweber@chromium.org
Status: Started (was: Unconfirmed)
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 29 2018

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

commit 33b07d1373210013a53523516bf86cb4277861e4
Author: Christian Dullweber <dullweber@chromium.org>
Date: Mon Jan 29 10:05:41 2018

Prevent opening a deleted profile

When deleting a profile, the profile will be marked as deleted and all
browsing data is removed but the profile folder will remain until shutdown
and it still contains some data like bookmarks and settings. Using command
line arguments it is possible to open a new window for this profile.
This CL changes the profile manager to not return profiles that have
been deleted. Instead of opening a window for a deleted profile,
Chrome will refuse to open the window.

Bug: 803643
Change-Id: Iae3caf93b2895d23b90aeb6550197a81038a1ec6
Reviewed-on: https://chromium-review.googlesource.com/883342
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532378}
[modify] https://crrev.com/33b07d1373210013a53523516bf86cb4277861e4/chrome/browser/profiles/profile_manager.cc

Status: Available (was: Started)
It should not be possible to open a Chrome Window for a deleted profile anymore. 

We should still think about cleaning up more data when the profile is deleted as e.g. bookmarks are only delted when Chrome is closed completely. Marking as available as this doesn't need to be done immediately.
Tested this issue on Windows 10 on the latest Chrome Build 66.0.3335.0 by following the steps mentioned in the original comment.

Not able to open the deleted chrome profile by clicking on shortcut icon after following the steps mentioned in the original comment.
Attached is the screen cast for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..
803643_CL.webm
20.2 MB Download
Tested this issue on Windows 10 on the latest Chrome Build 66.0.3335.0 by following the steps mentioned in the original comment.

Unable to open the deleted Chrome profile on clicking on the shortcut icon after following the steps mentioned in the original comment.
Attached is the screen cast for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..
803643_CL.mp4
10.1 MB View Download
Reverting this change as it causes crashes when sending feedback reports: https://crbug.com/807745
Status: Started (was: Available)
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 1 2018

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

commit 94557c5bcc01a075e89ba629e4a5a20445456589
Author: Christian Dullweber <dullweber@chromium.org>
Date: Thu Feb 01 09:57:37 2018

Revert "Prevent opening a deleted profile"

This reverts commit 33b07d1373210013a53523516bf86cb4277861e4.

Reason for revert: This change leads to crashes when sending feedback reports

Original change's description:
> Prevent opening a deleted profile
> 
> When deleting a profile, the profile will be marked as deleted and all
> browsing data is removed but the profile folder will remain until shutdown
> and it still contains some data like bookmarks and settings. Using command
> line arguments it is possible to open a new window for this profile.
> This CL changes the profile manager to not return profiles that have
> been deleted. Instead of opening a window for a deleted profile,
> Chrome will refuse to open the window.
> 
> Bug: 803643
> Change-Id: Iae3caf93b2895d23b90aeb6550197a81038a1ec6
> Reviewed-on: https://chromium-review.googlesource.com/883342
> Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
> Commit-Queue: Christian Dullweber <dullweber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#532378}

TBR=skuhne@chromium.org,dullweber@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 803643, 807745
Change-Id: I44ad772e3d1b7fc21ec9e414cd92c1d4b915a7af
Reviewed-on: https://chromium-review.googlesource.com/897522
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533615}
[modify] https://crrev.com/94557c5bcc01a075e89ba629e4a5a20445456589/chrome/browser/profiles/profile_manager.cc

I found a way to reproduce the issue: 
 - Open Chrome Stable 66.0.3335. 
 - Delete your default profile (the one in the "default" profile directory). 
 - Try to send feedback. 

This will call ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath. This method tries to open the "default" profile. The default profile is deleted, so ProfileManager::GetProfile calls CreateAndInitializeProfile for the still existing profile directory, which is not a good idea.
Cc: skuhne@chromium.org
skuhne@: My recent change to prevent loading deleted profiles caused a crash when sending feedback reports after deleting the "default" profile. (see last comments)

Do you have a suggestion on how to best fix this? One way would be to move the deletion check outside profile manager to browser startup.

The issue with profile manager is that some code expects that there is a default profile or a default profile can be created. When the default profile is deleted but Chrome didn't restart yet, both aren't true.

The issue has a TODO (from 2011): 

base::FilePath ProfileManager::GetInitialProfileDir() {
  ...
  // TODO(mirandac): should not automatically be default profile.
  return relative_profile_dir.AppendASCII(chrome::kInitialProfile);
}
(https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.cc?l=624&rcl=3724a2a9d3e065522c18cc39757e0cf4756ddc90)

"Initial profile" doesn't really have a meaning outside ChromeOS, maybe we could return the LastUsedProfile instead?
Could this also cause  Issue 807857 ?
Blockedon: 812636
ping skuhne@: 
I created a bug for the issue that sending feedback reports is always accessing the Default profile but I think there is a general problem that ProfileManager::GetPrimaryUserProfile and ProfileManager::GetActiveUserProfile are available for Linux, Windows or Mac builds. Do you know why they are there and why they return the "Default" profile? 
Both functions have a todo from you that they should be moved to ash but the todo is from 2014 :)
Cc: xiy...@chromium.org
I was travelling in China and Japan - as such I assume that this fell through the cracks somehow.

Yeah - that was part of the Multiprofile work. Back then there was a "getDefaultProfile" function which makes no sense in multi profile scenarios (what is the default then?) but it was called from over 250 locations. Back then we checked out each of the invocations and eliminated them one by one. The profile team was targeting a re-write of the profile manager, but their office got closed for development back in 2014 and the team members got re-assigned to new projects. As such the effort of the re-write got stopped.

We could do a delayed delete like this: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc?rcl=b0e6afca6430cd3aa67504b4453ec10e15e37dd9&l=117

Xiyuan (cc'ed) was suggesting that we might want to extend this into a more generic function.
Ah, this is windows. The code I suggested in #20 only works on ChromeOS.

How would feedback UI pick the profile to use? GetFeedbackProfile() [1]? Does this function pick up the deleted "Default" profile? Is there any profile available when it is called in the repro?

[1]: https://cs.chromium.org/chromium/src/chrome/browser/feedback/feedback_dialog_utils.cc?rcl=b0e6afca6430cd3aa67504b4453ec10e15e37dd9&l=40
Thanks! 
The feedback code that is crashing uses GetPrimaryProfile(). Maybe I can just switch it to the GetFeedbackProfile method? 
https://cs.chromium.org/chromium/src/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc?type=cs&q=ChromeInternalLogSource::PopulateSyncLogs&sq=package:chromium&l=168

This bug is not caused by GetPrimaryUserProfile() or GetActiveUserprofile() but my attempt at fixing it caused crashes because some code just assumes that a Default profile exists or can be created. As you said, it doesn't make sense on Windows to assume that the Default profile is the right thing to use.

One other place that looks problematic is the AutomationEventRouter. 
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/automation_internal/automation_event_router.cc?q=GetPrimaryUserProfile%7CGetActiveUserprofile+-f:chromeos+-f:test+-f:ash+-f:android&sq=package:chromium&l=39&dr=C

It seems easier and safer to fix this bug by just checking for deleted profiles when a Browser is created and not in ProfileManager in general, I think I will create a CL to do this. (https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browser_creator.cc?type=cs&sq=package:chromium&l=947)
For the crash in ChromeInternalLogSource::PopulateSyncLogs, we should probably pass down the profile when creating the system log fetcher instead of going through the ProfileManager::GetPrimaryProfile to get one.
Blockedon: 820057
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 15 2018

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

commit 8d297d6d7a3ea57b49fec20a4ab0c7419ca529be
Author: Christian Dullweber <dullweber@chromium.org>
Date: Thu Mar 15 13:39:21 2018

Delete bookmarks on profile deletion

When a profile is deleted, all browsing_data is removed and the folder
is marked for deletion on shutdown. Bookmarks are currently not deleted
until the folder is deleted. As bookmarks can contains sensitive data
they should also be removed immediately.
This also changes SigninManagerAndroidTest to use
ChromeBrowsingDataRemoverDelegate.

Bug: 803643, 748484
Change-Id: Icebfcec44d61d8b38939a8cee8dc4a92b6f9e258
Reviewed-on: https://chromium-review.googlesource.com/957040
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543358}
[modify] https://crrev.com/8d297d6d7a3ea57b49fec20a4ab0c7419ca529be/chrome/browser/android/signin/signin_manager_android.cc
[modify] https://crrev.com/8d297d6d7a3ea57b49fec20a4ab0c7419ca529be/chrome/browser/android/signin/signin_manager_android_unittest.cc
[modify] https://crrev.com/8d297d6d7a3ea57b49fec20a4ab0c7419ca529be/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
[modify] https://crrev.com/8d297d6d7a3ea57b49fec20a4ab0c7419ca529be/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
[modify] https://crrev.com/8d297d6d7a3ea57b49fec20a4ab0c7419ca529be/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc

Sign in to add a comment