Closed Bug 68827 Opened 24 years ago Closed 24 years ago

Crash in XPCOM migrating profiles

Categories

(Core Graveyard :: Profile: BackEnd, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: mkaply, Assigned: ccarlen)

Details

(Keywords: crash)

Attachments

(2 files)

Using current nightly on Windows (also recreated on OS/2, so believe to be XP) Delete moz*.dat and and the MOZILLA directory in the WINDOWS directory. Start Mozilla. When the profile migration screen appears, click Manage Profiles. Then click Exit on the Profile Manager. Start Mozilla again. On the migration screen, click Convert Profile. Trap in XPCOM. It appears to be caused by having some information about the profile in the registry but not all of it.
> Delete moz*.dat and and the MOZILLA directory in the WINDOWS directory. Can you clarify? For "MOZILLA directory in the WINDOWS directory" do you mean (if running NT4) Winnt\Profiles\you\Application Data\Mozilla ?
Yes, that's the directory on NT. Basically you need to remove any remnants that Mozilla had been started before. Mozilla directory and *.dat This crash happens with a "clean" start of Mozilla.
The problem is that during the first start of mozilla, registry entries will be made saying that there is a 4x profile to migrate. This entry remains even if the user presses Exit on the Manage Profiles dialog. During the second start of mozilla, it will call nsProfileAccess::Get4xProfileInfo as per usual. This will enumerate the subtrees in the registry. For each key it will get the profile name. If the call the ProfileExists() returns non-zero, it will skip on to process the next key. The code that it is skipping is where profileItem's are created and appended to the m4xProfiles list. The migration code enters into nsProfile::AutoMigrate() who calls nsProfile::MigrateAllProfiles(). It expects m4xProfiles to have at least one item. In the failing case, this isn't correct. So, is the fix to remove the profile registry entries if "Exit" is ever encountered? Would it be better to change MigrateAllProfiles() so that if there aren't any m4xProfiles to create them and THEN process them? I figure this is the least potentially problematic solution since I don't know who expects these entries to be there in other situations. But removing registry entries when "Exit" button is pushed would be more intuitive. Or maybe wait and not even put in the registry entries until after the migration is done. Any suggestions? I'll do the fix if you want, but I need to know what the best fix would be. I'm not that familiar with other considerations that might need to be taken into account (like what if the user does some profile renaming and deletion prior to hitting Exit, do we honor that, etc.).
Also contributing to this problem is that inside nsProfileAccess::FillProfileInfo(), mNumOldProfiles can be incremented (as it is in this case) WITHOUT an item being added to the m4xProfiles list. If this is the counter the represents the list, this seems like a pretty risky thing to be doing. Seems like maybe m4xCount would be the better counter to use in the for loop in MigrateAllProfiles(). But in this case that would mean that no profiles would be migrated since m4xCount is zero.
I see the problem: Basically the nsProfileAccess class (despite some earlier cleanup) is a mess. There are a number of instance variables in it which are used to keep count of the items in lists. There is both m4xCount and mNumOldProfiles, and worse, mNumOldProfiles is public - sheesh. Looking at nsProfileAccess::GetNum4xProfiles(), it does the right thing by enumerating the list and looking at whether or not the profile element has been migrated or not. When profile info is migrated by nsProfileAccess::MigrateProfileInfo(), it is immediately stuffed into the current list by UpdateProfileArray(). Once that happens, m4xCount and nNumOldProfiles should be irrelevant. The fact that nsProfile::MigrateAllProfiles() is looking directly at instance variables in nsProfileAccess which are themselves questionable is wrong. I've gotta clean this up once and for all.
Attached patch Patch which fixes this (deleted) — Splinter Review
The patch above fixes this by not looking at instance variables which it shouldn't be. Sent to reporter and tested.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
-> 0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
The 2nd patch fixes the problem but also cleans up nsProfileAccess. There were a number of instance variables which were used to keep track of the items in an array. The patch just uses Count() and avoids synchronization problems. Patch also removes a redundant list and makes nearly all member vars and meny methods private.
CC'ing Bhuvan and Seth for review/sr.
Keywords: crash, patch
Whiteboard: awaiting reviews
Bhuvan, Seth - any time to review?
r=valeski.
racham knows this code best. waiting for a review from him.
Conrad, Changes look good. As you are cleaning up the count variables, it will be best if you can run some regular profilemanager basic tests (you may have already done). Tests like migrate 1/more profiles, create profile & quit & launch, delete profiles. r=racham.
sr=sspitzer but please do a lot of testing. racham, is http://www.mozilla.org/profilemanager/checklist.txt still the correct checklist?
* One can delete unmigrated profile from the list now. It just takes it out of the registry.dat file. * -CreateProfile, creates profile and quits the app. Those are the 2 things I noticed being changed since that document is posted. I have requested for an account on gila server a while ago and it never materialized. I will follow up on that.
racham: send me an email and I'll fix that documentation for you tomorrow.
Seth, I have a gila account and should probably do that. In addition to the updates Bhuvan mentioned, there's another change: The document mentions mozregistry. It's now registry.dat. The file mozregistry will never be looked at by profile mgr.
ccarlen, you're a much better person to fix up the checklist since you're hacking in that code. thanks for taking care of it. after updating checklist.txt and running through them, please update this bug with the results. a successful run of those tests will put all our minds at ease.
OK - ran it through the checklist. No problems were found with the code being checked in for this bug. Two others were: (1) from my checkin for bug 60182 - deleting a profile and choosing *not* to delete the files causes nothing to happen - an uninitialized rv was being returned when in fact everything was NS_OK. I'll sneak that fix in with this one since it's a one-liner. (2) When renaming a profile, the name does not update in the profile mgr UI even though the profile is in fact renamed and NS_OK is returned to frontend. A build of NS6 from a month ago has the same problem. I'll file a new bug on that one. (3) This bit from the checklist: 2. mozilla case 1 : CreateProfileWizard is launched. I don't think is accurate. If launched with no args, the profile wizard does not come up. There are several possibilities that can happen, depending on the existence of 4x profiles, current profiles, etc. But AFAIK, launching mozilla with no args should not show the CreateProfileWizard.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: awaiting reviews
Conrad, you are right. CreateProfile Wizard will not come up when launched with mozilla with no args. Infact, the case in the question there was with no profiles (in the past we launched the wizard when no 6.0 profiles are found...that is really distant past..we need to definitely change the doc now). Seth at later point implemented force migration which when launched with no 6.0 profiles forces migration (all those who delete their registry and don't know how to remigrate are taken care of). If no 4.x profile(s) then, we silently create a default 6.0 profile and launch the browser. you know all these details... Anyway, I had a skeleton document covering various sections of the ProfileManager. I will pass it on to you. You can add to/modify the document with latest and greatest details and post it on gila server. Thanks for the help.
verified on branch 20010600713 adding vtrunk
Keywords: vtrunk
trunk build 2001061304
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: