731047 - Clean up old profile after Firefox profile reset

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Matthew N. [:MattN]]

We need

Comment 1

[Matthew N. [:MattN]]

We need to clean up the old profile after a reset so that we don't have a user's old profile lying around on their hard drive.

Comment 2

[Matthew N. [:MattN]]

(Quoting Richard Newman [:rnewman] from bug 725904 comment #1)
> Note that if you don't migrate Sync, and you consider the old profile
> disposed of, you should deactivate Sync on that profile before
> decommissioning it. That'll avoid this device showing up in Tabs From Other
> Computers, amongst other things.

Comment 3

[Matthew N. [:MattN]]

Some options:
1) Instant deletion of old profile
2) Move profile folder to user's Desktop or Trash with name like "Old Firefox Data".
3) Leave profile in place and delete it if it's unused for N days. 

On platforms where we use two profile folders (local and roaming), we should probably delete the local/cache one immediately.

I prefer option 3 because with #1 the reset may not have actually fixed the user's problems but they would still have dataloss.  With #2, the user won't necessarily know they can delete the folder depending on its name.

Comment 4

[Kadir Topal [:atopal]]

Yeah, option 3 seems like the best way to go.

Comment 5

[Matthew N. [:MattN]]

Attachment: WIP v.1 Backup to desktop, delete local profile, delete old profile

This patch implements option #2 from comment 3 because we can't guarantee that we'll be able to delete a profile later for option #3.

The long term goal after more testing in the wild is to not keep a backup.

bsmedberg, could you (or someone else) do a feedback pass on this? It works in my manual testing on OS X.

How do you recommend I test profile reset? It would be easy to do basic tests with a shell script but I'm not sure how to integrate that with our test suites.

Thanks

Comment 6

[Benjamin Smedberg]

Comment on attachment 607879 [details] [diff] [review]
WIP v.1 Backup to desktop, delete local profile, delete old profile

I think that the general approach is sane, but I'm concerned about the performance effects of copying all this data on the main thread. There could be a sizeable pause while this is happening...

Comment 7

[Matthew N. [:MattN]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I think that the general approach is sane, but I'm concerned about the
> performance effects of copying all this data on the main thread. There could
> be a sizeable pause while this is happening...

Good point.  Is nsILocalFile::CopyTo safe to run on a new thread?  Can I just make a new thread for the copy and spin the event loop in the meantime while the main thread shows a progress meter?

A simpler workaround would be to iterate the directory entries and copy each file separately.  Is this sufficient?

Is there a better solution?  If we don't make backups for the case where --reset-profile is used without --migration then I can instead do this work in migration.js but I don't think that makes this any easier.

Comment 8

[Benjamin Smedberg]

Iterate them and copy separately asynchronously? If you're doing it synchronously, that's just exactly what CopyTo does internally.

nsIFile is apartment-threaded, so it should be threadsafe to use one from a worker thread. This is what the cache-deletion code does, for example. Note that you'll have the same problem but probably worse deleting the local-profile, since that contains the network cache and can take upwards of a minute to delete in some cases.

Comment 9

[Matthew N. [:MattN]]

Attachment: v.1 Separate thread for copy/delete.

Switched to using a separate thread for the heavy I/O.

Should the new cleanup classes be moved to a new or an existing header file?
Is there a way to do a move following symlinks on the destination (rather than CopyToFollowingLinks + Delete)?

The UI still needs some polish but can you review the nsAppRunner portion?

Comment 10

[Matthew N. [:MattN]]

bsmedberg, when do you think you'll get a chance to review this? I'd like to get a decent amount of time baking this on Nightly. Thanks in advance.

Comment 11

[Benjamin Smedberg]

Comment on attachment 610481 [details] [diff] [review]
v.1 Separate thread for copy/delete.

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+static nsresult
>+ProfileResetCleanupAsync(nsILocalFile* aProfileDir, nsILocalFile* aProfileLocalDir,
>+                         nsIFile* aBackupDest, const nsAString& aLeafName)

I followed the trail of this "nsresult": it gets returned to ProfileResetCleanupAsyncTask::Run and from there to ProfileResetCleanupResultTask::mResult, but nobody ever really *uses* that result for anything except issuing a warning. Since you're already issuing NS_WARNINGs here, I think we can just make this return null.

Secondly in terms of code organization, we just talked on IRC that this is a fair bit of new code which isn't directly related to nsAppRunner.cpp. Can you move this set of functions into a new file toolkit/xre/ProfileReset.cpp so that the files aren't so large?

Thirdly, I don't think this separate function is really necessary. Can you just do all this work in ProfileResetCleanupAsyncTask::Run?

Fourthly, I think you can just set profileResetCleanupCompleted at the end of this method, so that the only thing ProfileResetCleanupResultTask needs to do is shut down the thread.

>+static bool profileResetCleanupCompleted = false;

This should be "gProfileResetCleanupCompleted;".

>+static nsresult
>+ProfileResetCleanup(nsIToolkitProfile* aOldProfile)

>+  // Try to get a unique backup directory name.
>+  backupDest->Clone(getter_AddRefs(uniqueDest));
>+  uniqueDest->Append(resetBackupDirectoryName);
>+  rv = uniqueDest->CreateUnique(nsIFile::DIRECTORY_TYPE, 0700);
>+  NS_ENSURE_SUCCESS(rv, rv);

I wonder if this name shouldn't have the date in it, e.g. "Old Firefox Data 10-Apr-2012". We'd still need CreateUnique.

>+  // Show a progress window while the cleanup happens since the disk I/O can take time.
>+  nsCOMPtr<nsIWindowWatcher> windowWatcher(do_GetService(NS_WINDOWWATCHER_CONTRACTID));
>+  nsCOMPtr<nsIDialogParamBlock> ioParamBlock(do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID));
>+  nsCOMPtr<nsIMutableArray> dlgArray (do_CreateInstance(NS_ARRAY_CONTRACTID));

Why do you need the dialog param block *or* the mutable array here? It doesn't look like you're using either one, and I'm pretty sure you can just pass null instead.

I think with those changes that this is correct. I hate the amount of boilerplate required to accomplish this, but I can't think of a better way to accomplish it.

Comment 12

[Matthew N. [:MattN]]

Attachment: v.2 move to toolkit/xre/ProfileReset

I didn't move GetCurrentProfileIsDefault because it isn't really reset specific and it isn't new in this patch.  If you feel strongly about it, I can move it.

Comment 13

[Matthew N. [:MattN]]

Attachment: Screenshot of profile cleanup progress for ui-review

Very simple dialog which is very similar to the application update progress bar.

Comment 14

[Matthew N. [:MattN]]

Try results are good: https://tbpl.mozilla.org/?tree=Try&rev=1ae774bb5312

Comment 15

[Benjamin Smedberg]

Comment on attachment 616462 [details] [diff] [review]
v.2 move to toolkit/xre/ProfileReset

>diff --git a/toolkit/xre/ProfileReset.cpp b/toolkit/xre/ProfileReset.cpp

>+/**
>+ * Creates a new profile with a timestamp in the name to use for profile reset.
>+ */
>+nsresult
>+CreateResetProfile(nsIToolkitProfileService* aProfileSvc, nsIToolkitProfile* *aNewProfile)
>+{
>+  NS_ENSURE_ARG_POINTER(aProfileSvc);

1) Please don't use NS_ENSURE_* in my modules
2) In this case we should just assert that it's nonnull, since we're not dealing with untrusted input at all.

>+
>+  nsCOMPtr<nsIToolkitProfile> newProfile;
...
>+  NS_IF_ADDREF(*aNewProfile = newProfile);

make this newProfile.swap(*aNewProfile)

>+/**
>+ * Delete the profile directory being reset after a backup and delete the local profile directory.
>+ */
>+nsresult
>+ProfileResetCleanup(nsIToolkitProfile* aOldProfile)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsILocalFile> profileDir;
>+  rv = aOldProfile->GetRootDir(getter_AddRefs(profileDir));
>+  NS_ENSURE_SUCCESS(rv, rv);

excise all the NS_ENSURE_* macros please. Just if (NS_FAILED(rv)) return rv;

r=me with those changes

Comment 16

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Screenshot: Are you sure reset window

The attached window should really be an arrow notification on the page rather than a modal dialog - is this possible?

Comment 17

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #16)
> Created attachment 619154 [details]
> Screenshot: Are you sure reset window
> 
> The attached window should really be an arrow notification on the page
> rather than a modal dialog - is this possible?

MattN brought up the good point that about:support really wouldn't have anywhere for an arrow-panel notification to be based from.  Would a page-modal dialog be possible?  Matt points out that it isn't XUL - how hard would the implementation be?

Comment 18

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 616463 [details]
Screenshot of profile cleanup progress for ui-review

This looks fine to me.

Comment 19

[Alex Limi (:limi) — Firefox UX Team]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #17)
> MattN brought up the good point that about:support really wouldn't have
> anywhere for an arrow-panel notification to be based from.  Would a
> page-modal dialog be possible?  Matt points out that it isn't XUL - how hard
> would the implementation be?

I wouldn't worry about it too much. This is deep in our UI, and is a troubleshooting mechanism, so it's probably fine if this is modal. Now, HTTP login boxes, on the other hand… ;)

Comment 20

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51eeffc5a31d

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/51eeffc5a31d

Comment 22

[Francesco Lodolo [:flod]]

Opened bug 758952 for missing l10n comments in profileSelection.properties

+resetBackupDirectory=Old %S Data

What is %S in this label?

Comment 23

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20120611 Firefox/15.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20120611 Firefox/15.0a2
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120611 Firefox/15.0a2
BuildID: 20120611042006

Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/16.0 Firefox/16.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0a1
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/16.0 Firefox/16.0a1
BuildID: 20120611030526

An "Old Firefox Data" folder is added to the Desktop and it contains the old (deleted) profile. The old profile is removed from everywhere else.