User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130612084701 Steps to reproduce: (I have 3 versions of Firefox installed, each has a different profile which I select on startup for the corresponding version.) So I just reset a profile which I named "UX" (the other two are "default-xxx" and "Aurora", where "xxx" is the random number generated by the reset profile feature). Actual results: The profile manager showed 2 "default-xxx" and one "Aurora" profiles (where "xxx" is the random number generated by the reset profile feature). Expected results: There should be one "default-xxx", one "Aurora" and one "UX-xxx" profile (where "xxx" is the random number generated by the reset profile feature). So basically, resetting a profile should preserve the (custom) name of a profile but just append (or replace) the random(?) numbers it adds now.

We don't really support multiple profiles. If you know how to create multiple profiles then a) you probably don't need to reset your profile because you can do your own troubleshooting and/or b) you can figure out how to rename the new profile.

Even for me it's confusing to have two different "default" profiles ;)

This should be easier to fix after bug 1265368 as the old profile name is being passed to the migrator.

bug 1122124 will fix the name in the profile manager dialog. I'm morphing this bug per the review comment in bug 1122124 to adjust the directory name of the new profile.

Hi, I am interested in working on this, if this is still an issue and/or something we should change. If so, is there any other information I should know of? Please let me know, thanks.

(In reply to Gregory Moore from comment #5) > Hi, I am interested in working on this, if this is still an issue and/or > something we should change. If so, is there any other information I should > know of? Please let me know, thanks. Hi Gregory, sorry for the slow response. Some notes: - the goal will be updating CreateResetProfile to use the existing profile name for the name of the newly created profile, in here: https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/toolkit/xre/ProfileReset.cpp#42 We should continue to use a timestamp suffix, but we can prefix with the old profile name followed by a dash, instead of hardcoding "default-" as the prefix. - to do this, we'll need to add the old profile name as an argument (probably as a const nsACString). - at the callsites in nsAppRunner.cpp, we tend to store the profile name in gResetOldProfileName. For a few callsites, you'll need to move the fetching of that name forward a little bit (so we do it before we create the reset profile) so that the name of the old profile is available beforehand. - in some cases, no name will be available, in which case the code in CreateResetProfile should still create the profile with the "default-" prefix. - finally, there is 1 automated test for this, which we should ensure gets updated to continue passing. It's at https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py . You can run it with ./mach test path/to/test. I expect it will need teaching about the changed path. Ideally, we should verify that the logic for the path works in that test. Does that help? Please feel free to ping me or Matt if you have more questions.

Hi Gjis, thanks for responding and for all of the information, and sorry I took so long to respond as well. This looks like enough to get started. I am currently trying to finish work on another bug, but I will start work on this once I'm done with that one. I'll let you know if I have any other questions. Thanks again!

Alright, here is a patch with what I have so far. I have some questions about it as well. (In reply to :Gijs from comment #6) > - at the callsites in nsAppRunner.cpp, we tend to store the profile name in > gResetOldProfileName. For a few callsites, you'll need to move the fetching > of that name forward a little bit (so we do it before we create the reset > profile) so that the name of the old profile is available beforehand. At which callsites would we need to move the fetching forward? It seems like the only time we set gResetOldProfileName within SelectProfile() is here: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2251 But all 3 callsites are after this point, so it seems like gResetOldProfileName should already be set. After creating the reset profile at these 2 points: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2411 https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2519 we do update gResetOldProfileName based on the new profile we created, but besides that I don't see anywhere we set it. Oh wait, I guess when we set gResetOldProfileName is only one case, so it won't always be set. So I guess we need to set it in the other 2 cases. Should we set gResetOldProfileName the same way in the other cases, by getting "XRE_PROFILE_NAME"?: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2247 Or do it some other way? > - in some cases, no name will be available, in which case the code in > CreateResetProfile should still create the profile with the "default-" > prefix. In these cases, would we just pass in an empty string? (e.g. nsAutoCString("")). Or is there a clean way to do a default argument for aOldProfileName? Or do we just pass in gResetOldProfileName, but it will be empty? > - finally, there is 1 automated test for this, which we should ensure gets > updated to continue passing. It's at > https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/ > tests/marionette/test_refresh_firefox.py . You can run it with ./mach test > path/to/test. I expect it will need teaching about the changed path. > Ideally, we should verify that the logic for the path works in that test. Alright. Do you have any suggestions for how to do this? I don't have much experience with Python. The test seems to be passing on my local build, even with the changes made though. Thanks.

(In reply to Gregory Moore from comment #8) > Created attachment 8835772 [details] [diff] [review] > Initial version of patch. > > Alright, here is a patch with what I have so far. I have some questions > about it as well. This patch is definitely going in the right direction. > (In reply to :Gijs from comment #6) > > - at the callsites in nsAppRunner.cpp, we tend to store the profile name in > > gResetOldProfileName. For a few callsites, you'll need to move the fetching > > of that name forward a little bit (so we do it before we create the reset > > profile) so that the name of the old profile is available beforehand. > > At which callsites would we need to move the fetching forward? It seems like > the only time we set gResetOldProfileName within SelectProfile() is here: > https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner. > cpp#2251 > But all 3 callsites are after this point, so it seems like > gResetOldProfileName should already be set. Ah, I think this is not quite true. This code is hairy (multi-100-line-methods = sad), but effectively I think we only enter the path you reference here: https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/toolkit/xre/nsAppRunner.cpp#2239-2240 so if the XRE_PROFILE_PATH environment variable is set. We then early return at the end of that if() block, here: https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/toolkit/xre/nsAppRunner.cpp#2284 The net result is that if we don't enter that block (the profile wasn't passed in as an environment variable) then we will hit one of the other 2 cases (if we're resetting at all, that is). > After creating the reset profile > at these 2 points: > https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner. > cpp#2411 > https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner. > cpp#2519 > we do update gResetOldProfileName based on the new profile we created, but > besides that I don't see anywhere we set it. We update it based on the *old* profile, not the new (reset) profile: 2416 nsresult gotName = profile->GetName(gResetOldProfileName); (ditto on line 2524) https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/toolkit/xre/nsAppRunner.cpp#2524 This is exactly the problem - we should fetch that information *before* calling CreateResetProfile. Effectively, that code just needs to move up a few lines. If we don't manage to get a name from the profile object, we can just set gDoProfileReset to false (so we won't reset at all) and not bother even calling createresetprofile (which fixes another bug, in fact - looks like right now it's possible for us to create the reset profile and then not reset...). > > - in some cases, no name will be available, in which case the code in > > CreateResetProfile should still create the profile with the "default-" > > prefix. > In these cases, would we just pass in an empty string? (e.g. > nsAutoCString("")). Or is there a clean way to do a default argument for > aOldProfileName? Or do we just pass in gResetOldProfileName, but it will be > empty? From looking at other code, I think you can define the argument as: const nsACString& message = EmptyCString() in the header file, which will define the default argument as an empty string. But looking at the code again, I'm not sure we're ever hitting that case. It's possible I missed something, though. > > - finally, there is 1 automated test for this, which we should ensure gets > > updated to continue passing. It's at > > https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/ > > tests/marionette/test_refresh_firefox.py . You can run it with ./mach test > > path/to/test. I expect it will need teaching about the changed path. > > Ideally, we should verify that the logic for the path works in that test. > > Alright. Do you have any suggestions for how to do this? I don't have much > experience with Python. The test seems to be passing on my local build, even > with the changes made though. Thanks. If it's still passing I guess we're good. It looks like we already determine the new profile path dynamically: https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/browser/components/migration/tests/marionette/test_refresh_firefox.py#395-399 so that should be OK. We could potentially add a test assertion about the profile path, but let's get the code changes worked out first. :-)

(In reply to :Gijs from comment #9) > We update it based on the *old* profile, not the new (reset) profile: Oh I see, you're right. My bad. Thanks for all of the information. I made the changes you mentioned in this new patch. It's still passing the test case, but I can't really tell if it's doing what it's supposed to yet.

Comment on attachment 8836417 [details] [diff] [review] Moved fetching old profile name forward and added default argument to CreateResetProfile(). Review of attachment 8836417 [details] [diff] [review]: ----------------------------------------------------------------- Great! I think the code is almost done. One comment below, and then I'll give some comments about tests here. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/components/migration/tests/marionette/test_refresh_firefox.py#364-371 looks like it sets the name of the profile we're resetting. It stores it in the 'profileNameToRemove' member of the test runner/item/thingy object (ie 'self'). It looks like in addition to these asserts: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/components/migration/tests/marionette/test_refresh_firefox.py#416-417 you could add another one that asserts that the string stored in 'profileNameToRemove' is a substring of the path name in 'reset_profile_path'. Does that help? ::: toolkit/xre/ProfileReset.cpp @@ +33,5 @@ > /** > * Creates a new profile with a timestamp in the name to use for profile reset. > */ > nsresult > +CreateResetProfile(nsIToolkitProfileService* aProfileSvc, nsIToolkitProfile* *aNewProfile, const nsACString &aOldProfileName) Er, sorry, I just realized that now all the callers are actually providing this argument, and having it after the **out param is a bit odd... can we add it as a non-optional argument before the out param (aNewProfile) ?

(In reply to :Gijs from comment #11) > you could add another one that asserts that the string stored in > 'profileNameToRemove' is a substring of the path name in > 'reset_profile_path'. > > Does that help? Yeah, definitely, thanks for all of the information. I added an assertion for this case. Also by the way, right now I believe we are doing a case-sensitive substring comparison. I'm pretty sure that is the correct way in this case though. > Er, sorry, I just realized that now all the callers are actually providing > this argument, and having it after the **out param is a bit odd... can we > add it as a non-optional argument before the out param (aNewProfile) ? Sure, that makes more sense. Here is the patch. Let me know if there's any other problems, and thanks for all of the help!

Comment on attachment 8836899 [details] [diff] [review] Removed default argument from CreateResetProfile() and added assertion to doReset(). Review of attachment 8836899 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me - thanks for working on this! :-)

Trypush to check I didn't miss anything: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=551565fd4eaa28788de8c880d0463ab0524e3559

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b895d6a2483e Updated CreateResetProfile() to use the existing profile name for the newly created profile, r=gijs