add support for re-migration of defunct previously migrated profiles

VERIFIED FIXED in mozilla1.0.1

Status

Core Graveyard
Profile: BackEnd
P1
critical
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: selmer (gone), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

Trunk
mozilla1.0.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1],[hitlist] [ETA 5/2] [m5+] [Needs a=][fixed on trunk])

Attachments

(9 attachments, 13 obsolete attachments)

19.50 KB, image/gif
Details
18.72 KB, image/gif
Details
65.45 KB, image/gif
Details
971 bytes, patch
racham
: review+
Details | Diff | Splinter Review
15.84 KB, image/jpeg
Details
43.80 KB, patch
Conrad Carlen (not reading bugmail)
: review+
Details | Diff | Splinter Review
832 bytes, patch
Alec Flett
: superreview+
Details | Diff | Splinter Review
1.65 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
42.92 KB, patch
chris hofmann
: approval+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
The 6.0 release got a lot of people to try the product and then they decided
they didn't like it.  We're starting to get more people coming back to try the
product again and these folks are having problems because the old 6.0 profile
doesn't have all their recent mail etc.  We need to detect these defunct
profiles and "make them go away" in the majority of cases.

(I thought there was a bug like this already, but I couldn't find it after
*much* searching.)

IIRC, we had said that the 6.0 profiles would be unique in some characteristics
like being in the users50 directory and not having a .slt directory and maybe
some other characteristics.  Adding the time element, if such a profile hasn't
been used in over a year I'd suggest removing it from the profile list (leaving
the files in place) and either creating a new profile or migrating over again. 
If any version since 6.1 has been installed it's probably too late to do the
right thing, but perhaps it's worth throwing up a dialog if the profile hasn't
been used for more than 3 months.
(Reporter)

Comment 1

16 years ago
Adding Bhuvan for historical perspective, Ben for any related FE changes, Dan
for installer tie-ins, Kevin for marketing involvement and Scott for ADT
driving.  Since this is such a critical bug, I'm being presumptuous in adding
nsbeta1+ and [adt1] to it.

Everyone, please add any information we already have in this bug and any ideas
on how to proceed.  (Does an older bug actually exist somewhere?)

Dan, it's possible the user experience is improved by solving this in the
installer.  What does it take to do that?  Would we require the entire registry
library?  Is it easier/faster/better to implement an install solution than a
profile manager solution?

We need to move fast on this!
Severity: normal → blocker
Keywords: nsbeta1+
Whiteboard: [adt1]
(Reporter)

Comment 2

16 years ago
Presumably, Mozilla cares about this same issue WRT the milestones corresponding
to the 6.0 timeframe.  The fix for this should work equally for both products.
(Reporter)

Comment 3

16 years ago
Adding Lori to get UE input going.  We need a clean way to handle this in cases
where user interaction is required.
I verified that the app writes out the prefs file on quitting and it's written
whether any change has happened or not.So, the mod date of prefs.js is
essentially the mod date of the profile.

I think the following would work:
Determine the "mod date" of the chosen profile.
If not used in 3 months
  See if there is a 4.x profile of the same name
  If the 4.x mod date is more recent (I need historical help here on how to know)
    Ask the user if they want to re-migrate it
Status: NEW → ASSIGNED
Most of this should be implemented here:
http://lxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsAppShellService.cpp#232
This behavior should not be in the profile mgr backend because that is used by
embedding apps as well and this behavior is a mozilla/ns application behavior.
We shouldn't put such code into a component which is used by embedding apps
which have no use for it.

  The profile mgr back end can have a new method GetProfileLastModifiedTime()
because that's generic enough. Given that, here's how I think it should work:

(1) nsAppShellService::DoProfileStartup() starts up the profile mgr.
(2) If a profile is chosen - it will be unless we're doing initial startup in
QuickLanuch mode with multiple profiles or the user canceled from the profile
picker dialog.
(3) Now that a profile is chosen, DoProfileStartup() checks the mod date of the
profile.
(4) If it is old, DoProfileStartup() uses Srilatha's code for getting a list of
4.x profiles, checks if the name of the chosen profile matches the name of a 4.x
profile and, if so, asks the user if they want to remigrate.
(5) If they confirm, existing profile mgr backend methods should be called to do
the work. If exactly what we need to do this isn't exposed, I'll do so.

(3) 

Comment 6

16 years ago
Updating QA Contact.  Grace will lead the QA on this matter.
QA Contact: ktrina → gbush

Comment 7

16 years ago
Conrad - do you believe 3 months is a better number than 6 months for the last
modified date?
> Conrad - do you believe 3 months is a better number than 6 months for the last
modified date?

I used 3 months for example because that's the number Steve used. I'm not sure
how we determine the "stale" threshold. Whatever we decide for that value, it's
easily changeable.

Comment 9

16 years ago
Unless Steve has objections, let's use 6 months. I'll consult a few more
marketing types and see if that number should change.

OT: how do you get quoting to work in bugzilla? I copy pasted from a comment,
and the quoting doesn't show up.
(Reporter)

Comment 10

16 years ago
I think 6 months is the right threshold.  At 6 months, I'm not convinced we
actually need to prompt the user.  It's theoretically safer, but if there's an
unused 6.0 profile and a recently used 4.x profile of the same name then it's
pretty clear they gave up on 6.0 and won't even remember what's in that profile
anyway.  (Leaving the files around allows for recovery and a useful release note.)

What do others think about cleaning up without prompting?

Comment 11

16 years ago
Are we keying off of anything besides prefs.js?  I'd be concerned that we'd hit
6.1 and 6.2 users if we aren't.

Personally, I think it's dangerous to automatically do this without notifying
the user.  Six months means they haven't used it since December.  If they are
using 6.x through last December they are bound to have accumulated some things
they care about.
(Reporter)

Comment 12

16 years ago
What about at 8 or 10 months?  In that range, they probably wouldn't have gotten
6.1 or higher.  It might be safer to check install versions, but I'm trying to
avoid that because it's a pain.  

My main point from the previous comment is that if it's been unused as long as 6
months the user isn't even going to recognize the contents.  Putting UI in front
of people and making them choose things isn't always in their best interests. 
Frequently, we can't describe a choice like this without confusing them and
leading to a worse result than just making the choice for them.  I worry that
any dialog we can come up with would just scare them away again muttering
"they're even telling me not to try it again!"  The other bad scenario would be
them calling for support because of the dialog.
> Are we keying off of anything besides prefs.js?  I'd be concerned that we'd hit
6.1 and 6.2 users if we aren't.

Looking at the code which writes out the prefs on quitting in CVS blame shows
that we have been doing that since Aug 2000. So, using the date of prefs.js is
pretty certain for this purpose.

I think we should ask the user. Because of salting and absolute path names,
backing up (leaving in place) the old profile data won't do much good as a
fallback since we don't offer a way for the user to retore it. See bug 22689 and
bug 17457.

Confusing alerts at startup are no fun, but as long as the text for this is
clear and obvious enough, I think it would be OK.

Comment 14

16 years ago
If we don't show an alert in most cases, then that's fine. The conditions for
the alert to show up should be in accordance with the situation we're trying to
rectify in this bug only. Something like:

"You haven't used your <Netscape 6.0/Mozilla> profile since <xx/xx/xxxx.> Would
you like to use this profile (your profile contains bookmarks, email, address
book records and settings) for <Netscape X/Mozilla> or use the most recently
modified profile <(last modified on xx/xx/xxxx)> located in your <Communicator
4.79> directory?"

Where anything between <> is generated after we examine the modification
particulars of their profiles.

It gets ugly!
Status update: I have a patch close to completion. As of now, there is no prompt
but that can be added easily if we decide we need it.
(Reporter)

Comment 16

16 years ago
Kevin, I think the dialog needs to mention that the old profile probably doesn't
have their most recent email and bookmarks.  That's the whole point of this
exercise after all.

Adding jglick since there's no UE input yet and I'm not the right person to be
making these responses.

Comment 17

16 years ago
No question - I'm open to whatever. Jen, can you read the bug and give me a call
if you have questions about the problem we're trying to solve?  thx

Comment 18

16 years ago
Created attachment 80425 [details]
Proposal

The most current profile would be selected by default (for users who accept all
the defaults when installing). Profiles over X months old could be not
displayed at all? Title and buttons on this dialog would depend on how this is
implemented. Is it a separate dialog vs part of Install wizard.
Created attachment 80440 [details] [diff] [review]
Patch to profile mgr

1. Adds a RemigrateProfile() method which can be used for this purpose.
2. Maintains info in the registry about
  (a) when a profile was created
  (b) when a profile was last used
  (c) when a profile was migrated, *where* it was migrated from. If we had this
info to begin with, so many problems (including, but not limited to this one)
would be so much easier.

I think this info needs to be added before another version goes out the door.
Since it's new data, low risk.

3. Various cleanup - There was code for reading profile locations which was
only used around M10-M11
Created attachment 80441 [details] [diff] [review]
patch to appshell service

Comment 21

16 years ago
Created attachment 80442 [details]
#2

Removed the word "import" from dialog since it isn't correct for 6.x profiles.
Dialog would appear when user first attempts to use the product. If they pick
6.x, use that existing profile, if they pick 4.x, migrate data. If they pick
"Do not use settings..." let them start with a new profile.

Is this close to what you guys are thinking or am I completely off here? Do we
not want to give them the option for 4.x?

Comment 22

16 years ago
That seems right, Jen. I'm not sure where we are though. 

Conrad, is your patch designed to act without user interaction? Is adding the
dialog and acting on it difficult to implement?
talked with ccarlend and jglick over aim.

jglick is working up a slightly different UI, for review.

In parallel, I'm going to apply / review ccarlen's patches, make it so we can
set the interval with a pref (for easier testing / development / last minute
changes) and add confirmEx code to throw up the prompt.
Assignee: ccarlen → sspitzer
Status: ASSIGNED → NEW
OS: Windows NT → All
Hardware: PC → All
Summary: Detect defunct 6.0 profile and re-migrate → Detect defunct previously migrated profiles and re-migrate

Updated

16 years ago
Whiteboard: [adt1] → [adt1],[hitlist]
(Reporter)

Comment 24

16 years ago
Jen, thanks for jumping in here!  I like the idea behind your dialog proposal. 
One thing to add: the choice the user is making is intrisically exclusive.  If
they pick 4.x, then the 6.x goes away.  If they pick 6.x, we won't ever
remigrate the 4.x.  The dialog should somehow convey the concepts of exclusivity
and permanence implied by this action.

Do we really want the "Do not use settings from a previous version checkbox"? 
The implementation of that would be to remove the 6.x profile, avoid any
remigration, and force a new profile.  Is that OK?

Kevin, Conrad is only on the hook to provide us the mechanical guts of the
implementation.  Someone in Apps will do all UI work for this.

Comment 25

16 years ago
cc'ing mcarlson for localization impact of adding a dialog for beta.

Comment 26

16 years ago
Created attachment 80479 [details]
Updated Proposals

After chatting with Seth and Conrad, some updated proposals (in order of
preference :-)

Comment 27

16 years ago
Note: Users will only see this dialog if they select a profile in which the 
6.x profile and 4.x profile have the same name, And the 6.x profile has not been 
used for over 6 months. 

Comment 28

16 years ago
If we can pull off #1, that would be great. But in the interest of time, I think
the clearest dialog is #3 because it conveys in a minimum number of words the
idea that recent profile info is good. The likely scenario is that the user will
simply hit Yes and we do the right thing. 
(Reporter)

Comment 29

16 years ago
I agree with Kevin on #3.  I liked the wording about what's in a profile from
the previous dialog with the picker though.  We know users don't know what the
word profile means, so explaining that it's bookmarks and mail helps them get
the right answer.
Created attachment 80523 [details] [diff] [review]
initial rough draft patch, starting with ccarlen's patches

differences:

1) pref to control interval (useful for dev and qa)
2) added some comments
3) re-arranged some logic, to prevent nested ifs
4) moved out code into private method

the UI right now is just hard codd. Once I move the strings out to a properties
file, I'll implement #3.
Attachment #80440 - Attachment is obsolete: true
Attachment #80441 - Attachment is obsolete: true
here's my todo list:

1)  find out what happens to the existing 6.x profile when we re-migrate
2)  verify that all the PRInt64 code is "mac friendly".  I think win32 allows
you to get away with a lot more than mac does, I think we need to use more of
the NSPR PRInt64 macros.
3)  move the UI strings out to a properties file
4)  test and build on all 3 platforms, and provide QA with the test plan that I
used.
Severity: blocker → critical
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
Created attachment 80530 [details] [diff] [review]
updated patch, strings moved to migration.properties

also, added some comments to all.js
Attachment #80523 - Attachment is obsolete: true
Created attachment 80532 [details]
screen shot, from mozilla build

this is the screen shot of the dialog from a mozilla build.
for ns builds, the only difference would be the title, since both ns and
mozilla migrate from 4.5+.
Created attachment 80538 [details] [diff] [review]
updated patch, use LL_IS_ZERO() and make it so if the pref is -1, profiles never go defunct, and we never see the UI (some vendors may not want this feature)
Attachment #80530 - Attachment is obsolete: true
Created attachment 80539 [details] [diff] [review]
ns tree diff (to set the interval to 6 months)
Created attachment 80540 [details] [diff] [review]
updated mozilla patch
Attachment #80538 - Attachment is obsolete: true
ok, I've tested the latest patch and it's working for me on win2k.  (haven't 
tested linux and mac yet.)

A brief summary of what happens on disk

my profile is sspitzer
so on disk I've got:

Application Data\Mozilla\Profiles\sspitzer\<foobar>.slt\...

After I remigrate, I'd have this on disk:

Application Data\Mozilla\Profiles\sspitzer\<foobar>.slt\... [remigrated files]
Application Data\Mozilla\Profiles\sspitzer\<foobar>.slt-old\... [old defunct 
files]

three concerns that I'll discuss with ccarlen tomorrow:

1)

+  // Now, we know that a matching 4.x profile exists
+  // See if it has any newer files in it than our defunct profile.
+  // XXX todo
+  // should we descend into child directories?

is it possible for the account to be used, but the top level files never change?
(probably not, but it's worth pointing out that using just mail might cause 
this.)

2)

the way the code is now, the user might re-migrate again.  They could decide we 
still stink, go back to 4.x and try again in another six months.  In which 
case, they'd already have a -old directory (or they might have one from a 
failed "remigration")

+    // XXX todo 
+    // what if foo-old exists?  we need to createunique or something
+    nsCAutoString newDirLeafName(origDirLeafName + NS_LITERAL_CSTRING("-old"));
+    rv = profileDir->MoveTo(nsnull, newDirLeafName.get());
+    if (NS_FAILED(rv))
+        return rv;

3)

I need to see if ccarlen saw this, as well.

+    // XXX todo
+    // on win2k [at least for me], after MoveTo(), empty folders are being 
left behind!
+    // without this call to Remove(), the call to Create() fails.  
+    rv = newProfileDir->Remove(PR_TRUE);
+    if (NS_FAILED(rv))
+        return rv;

after we resolve those issues:

1) build test on mac, linux
2) code reviews
3) provide QA with my initial test list
4) bake on the trunk

Comment 39

16 years ago
>the way the code is now, the user might re-migrate again.  They could decide we 
>still stink, go back to 4.x and try again in another six months.  In which 
>case, they'd already have a -old directory (or they might have one from a 
>failed "remigration") 

I figured we'd do this the first time the user used a profile, not every 6
months that they don't use it.  Maybe each release we can do this the first
time.  What do others think?

Comment on attachment 80540 [details] [diff] [review]
updated mozilla patch

>Index: xpfe/appshell/src/nsAppShellService.h
>===================================================================
>+#include "nsIProfileInternal.h"
> 
> class nsAppShellService : public nsIAppShellService,
>+private:
>+  nsresult CheckAndRemigrateDefunctProfile(nsIProfileInternal *aProfileMgr);
> };
>

I would just use a forward declaration of nsIProfileInternal here instead of
increasing the the number of files which now include nsIProfileInternal.h by
including this file. Also, passing it as a param is odd. It's a singleton
service and doing that just saves one getService() call. 


>+NS_IMETHODIMP 
>+nsProfile::RemigrateProfile(const PRUnichar* profileName)
>+{
>+    nsresult rv;
>+    NS_ENSURE_ARG_POINTER(profileName);
>+    
>+    nsCOMPtr<nsIFile> profileDir;
>+    rv = GetProfileDir(profileName, getter_AddRefs(profileDir));
>+    if (NS_FAILED(rv))
>+        return rv;
>+    nsCOMPtr<nsIFile> newProfileDir;
>+    rv = profileDir->Clone(getter_AddRefs(newProfileDir));
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    // The profile list used by GetOriginalProfileDir is the one with ALL 4.x
>+    // profiles - even ones for which there's a moz profile of the same name.
>+    nsCOMPtr<nsILocalFile> oldProfileDir;
>+    rv = GetOriginalProfileDir(profileName, getter_AddRefs(oldProfileDir));
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    // In case of error, we'll restore what we've renamed.
>+    nsXPIDLCString origDirLeafName;
>+    rv = profileDir->GetLeafName(getter_Copies(origDirLeafName));
>+    if (NS_FAILED(rv))
>+        return rv;
>+        
>+    // Backup what we're remigrating by renaming it and leaving in place
>+    // XXX todo 
>+    // what if foo-old exists?  we need to createunique or something

I guess - if we ever do this twice to the same profile. 2 different long
periods of inactivity?
Since we don't have the equiv of tmpnam on nsILocalFile, it would have to be
done with createunique, get the leaf name then, delete the file, etc.

>+    nsCAutoString newDirLeafName(origDirLeafName + NS_LITERAL_CSTRING("-old"));
>+    rv = profileDir->MoveTo(nsnull, newDirLeafName.get());
>+    if (NS_FAILED(rv))
>+        return rv;
>+    
>+    // XXX todo
>+    // on win2k [at least for me], after MoveTo(), empty folders are being left behind!
>+    // without this call to Remove(), the call to Create() fails.  
>+    rv = newProfileDir->Remove(PR_TRUE);
>+    if (NS_FAILED(rv))
>+        return rv;

Sounds like a bug in either Win2k that nsLocalFileWin should take care or or a
bug in nsLocalFileWin - doesn't happen on Mac.

>+
>+    // Create a new directory for the remigrated profile

Comment 41

16 years ago
> I liked the wording about what's in a profile from the previous dialog with 
> the picker though.  We know users don't know what the word profile means, so 
> explaining that it's bookmarks and mail helps them get the right answer.

We could go with:

"A more recent <Netscape 4.5+> version of this profile (which contains
your bookmarks, email settings, address books and preferences) was
found. Would you like to use the more recent profile?"

Comment 42

16 years ago
>I figured we'd do this the first time the user used a profile, not every 6
>months that they don't use it. 

Agree. IF they use the profile AND its > 6 months since the last time the 
ns6.0+/moz profile was used, they get the dialog.
(Reporter)

Comment 43

16 years ago
If we're bought into showing the dialog every time, perhaps the interval doesn't
need to be so long?

The profile code already has a way of handling directory name clashes, that's
how we get all the profname-1, profname-2 directories.  Could we use that same
code to get profname.old, profname.old-1, ... rather than leave that error case
dangling?

If I run netscp.exe -mail, does prefs.js get updated?  I assume that it does,
but it's worth verifying since that file is being used to derive the last used date.

If we're using the "users50" directory as our clue that this is a defunct
profile, then a remigrated profile would never be re-remigrated since we'll have
put it in the "profiles" directory.  What clue are we using to say that it's a
profile created with a version from late 2000?

Comment 44

16 years ago
I'd like to avoid showing the dialog every time. I thought that the conditions
we thought through were sound, but I don't mind discussing them again. It seems
that if the last modified date is judged to be "current" that we shouldn't
bother showing this. 
We don't show the dialog every time.

We only show the dialog:

if your 6.x profile hasn't been used in 6 months
and your 4.x profile (of the same name) has been used more recently than your 
6.x profile.

Note, if you get the dialog, and you say:  "no, don't remigrate", the 6.x 
profile has now been used.  and you won't see the dialog again.
Created attachment 80617 [details] [diff] [review]
updated patch, more comments, fixed some micro -> milli sec issues.
Attachment #80540 - Attachment is obsolete: true
one problem I'm seeing:

we never properly write out the profile's last modified time.

The reason is we are trying to update the registry on shut down, we fail to
create instance of NS_REGISTRY_CONTRACTID, because we are in shutdown.

the problem is we can't call UpdateCurrentProfileModTime(PR_TRUE); from 
nsProfile's dtor.
 
nsProfileAccess::UpdateRegistry(nsIFile * 0x00000000) line 647
nsProfile::UpdateCurrentProfileModTime(int 1) line 2008
nsProfile::~nsProfile() line 332
nsProfile::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsProfile::Release(nsProfile * const 0x01615270) line 359 + 141 bytes
nsSupportsArray::Clear(nsSupportsArray * const 0x002f8758) line 600 + 54 bytes
nsSupportsArray::DeleteArray() line 304
nsSupportsArray::~nsSupportsArray() line 147
nsSupportsArray::`vector deleting destructor'(unsigned int 1) + 81 bytes
nsSupportsArray::Release(nsSupportsArray * const 0x002f8758) line 238 + 139 
bytes
nsCOMPtr<nsISupportsArray>::~nsCOMPtr<nsISupportsArray>() line 491
nsDirectoryService::~nsDirectoryService() line 596 + 11 bytes
nsDirectoryService::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsDirectoryService::Release(nsDirectoryService * const 0x002f78a8) line 598 + 
136 bytes
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 565 + 27 bytes
main(int 3, char * * 0x00304840) line 1773 + 8 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()
talked to ccarlen, we're going to move the call to UpdateCurrentProfileModTime
(PR_TRUE); from nsProfile's dtor to SetCurrentProfile().

testing that change now...
Created attachment 80627 [details] [diff] [review]
updated patch, now we properly update the last modified time in the registry on shutdown.
Attachment #80617 - Attachment is obsolete: true
Created attachment 80629 [details]
updated screenshot
Attachment #80532 - Attachment is obsolete: true

Comment 51

16 years ago
"< >" aren't necessary. Just my way of indicating, "insert correct product name 
here". :-)
Comment on attachment 80627 [details] [diff] [review]
updated patch, now we properly update the last modified time in the registry on shutdown.

>+//pref("profile.seconds_until_defunct", -1);
>+pref("profile.seconds_until_defunct", 10);
>+

This value is good for testing but make sure you put in a real value before
checkin.

>+ 
>+  nsXPIDLString dialogText, button0Text, button1Text;
>+  rv = migrationBundle->GetStringFromName(NS_LITERAL_STRING("confirmRemigration").get(), getter_Copies(dialogText));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  rv = migrationBundle->GetStringFromName(NS_LITERAL_STRING("confirmRemigrationButtonYes").get(), getter_Copies(button0Text));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  rv = migrationBundle->GetStringFromName(NS_LITERAL_STRING("confirmRemigrationButtonNo").get(), getter_Copies(button1Text));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  nsCOMPtr<nsIPromptService> promptService(do_GetService("@mozilla.org/embedcomp/prompt-service;1", &rv));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  PRInt32 buttonPressed;
>+  rv = promptService->ConfirmEx(nsnull, brandName.get(),
>+    dialogText.get(),
>+    (nsIPromptService::BUTTON_POS_0 * 
>+    nsIPromptService::BUTTON_TITLE_IS_STRING) + 
>+    (nsIPromptService::BUTTON_POS_1 * 
>+    nsIPromptService::BUTTON_TITLE_IS_STRING),
>+    button0Text.get(),
>+    button1Text.get(),
>+    nsnull, nsnull, nsnull, &buttonPressed);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+

From the latest screen shot, the button titles are still "Yes" and "No" If
that's the final text for those buttons, you don't need to fetch strings and
use the BUTTON_TITLE_IS_STRING const with confirmEx. confirmEx was designed to
avoid that. For "Yes" and "No", you can pass BUTTON_TITLE_YES, and
BUTTON_TITLE_NO and save code, and save stuff to be localized.

>+
>+  // call ShutDownCurrentProfile() so we update the last modified time of the profile
>+  {
>+    // scoping this in a block to force release
>+    nsCOMPtr<nsIProfileInternal> profileMgr(do_GetService(NS_PROFILE_CONTRACTID, &rv));
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "failed to get profile manager, so unable to update last modified time");
>+    if (NS_SUCCEEDED(rv)) {
>+      // 0 is undefined, we use this secret value so that we don't notify
>+      profileMgr->ShutDownCurrentProfile(0);

ShutDownCurrentProfile() is on plain-jane nsIProfile - no need for
nsIProfileInternal here. I'd like to keep the consumers of nsIProfileInternal
as limited as possible.

>+# LOCALIZATION NOTE: Do not translate Unicode (\uXXXX) characters.
>+confirmRemigration=A more recent <Netscape 4.5+> version of this profile (which contains your bookmarks, email settings, address books and preferences) was found.\u000aWould you like to use the more recent profile?
>+confirmRemigrationButtonYes=Yes
>+confirmRemigrationButtonNo=No

See previous comment about not needing "Yes" and "No" strings
Created attachment 80659 [details] [diff] [review]
updated mozilla patch (ns patch stays the same), addresses ccarlen's comments.
Attachment #80627 - Attachment is obsolete: true
Created attachment 80660 [details]
screen shot, no < or >
Attachment #80629 - Attachment is obsolete: true
Comment on attachment 80659 [details] [diff] [review]
updated mozilla patch (ns patch stays the same), addresses ccarlen's comments.

>+++ profile/src/nsProfile.cpp	23 Apr 2002 21:11:11 -0000
>@@ -327,7 +327,9 @@
> 
>    if (--gInstanceCount == 0) {
>         
>+      if (gProfileDataAccess) {
>       delete gProfileDataAccess;
>+      }
> 

There used to be something else in this block, justifying the "if". Now that
there's not, don't need the "if"
This is a minor nit, I should have noticed it before, and the r= stands.
Nice.
Attachment #80659 - Flags: review+

Comment 56

16 years ago
Seth, Conrad - thanks. You guys rock!
>+      if (gProfileDataAccess) {
> delete gProfileDataAccess;
>+      }
> 


those lines aren't necessary at all, since it is legal to delete null.

I've removed them locally.
status update:

I still need to test the patch on linux and mac, after that, get some reviews
and superreviews.

Just met with gbush to discuss the test plan.
Whiteboard: [adt1],[hitlist] → [adt1],[hitlist] reviewing, testing patch, eta to land on trunk 4/24

Comment 59

16 years ago
r=bhuvan. Looks fine. Given the changes in here, I think we do have to cover the
complete profile manager test suite. 
Whiteboard: [adt1],[hitlist] reviewing, testing patch, eta to land on trunk 4/24 → [adt1],[hitlist]

Comment 60

16 years ago
Putting back Seth's status whiteboard updates..
Whiteboard: [adt1],[hitlist] → [adt1],[hitlist] reviewing,testing patch, eta to land on trunk 4/24
setting as p1.

I've tested the patch on linux.  I needed to fix 
mozilla/profile/src/Makefile.in (to include profile) to get it to build.

Next, I'll test on mac, and then run through the profile manager test plan to 
make sure these changes won't cause regressions.

I still need to figure out why:

+    // XXX todo
+    // on win2k [at least for me], after MoveTo(), empty folders are being 
left behind!
+    // without this call to Remove(), the call to Create() fails.  
+    rv = newProfileDir->Remove(PR_TRUE);
+    if (NS_FAILED(rv))
+        return rv;

is needed for me on win2k, but not linux (and I assume, mac)  
I'll investigate today.
Priority: -- → P1
Wait, I missed something:

+    // without this call to Remove(), the call to Create() fails.  
+    rv = newProfileDir->Remove(PR_TRUE);
+    if (NS_FAILED(rv))
+        return rv;

When I tested this on Mac (in my patch) I didn't need the Remove() because the
folder was successfully renamed with nothing left behind. If you fail on the
error to Remove() on the Mac, it will fail because there will be nothing to remove.

Comment 63

16 years ago
+      if (gProfileDataAccess) {
       delete gProfileDataAccess;
+      }
 
delete checks for null, so you don't have to. I'll look at the patch more tomorrow.
turns out, the MoveTo() issue is a known bug.  see bug #101527

There's a patch for it, I'll try to drive it in with this bug.

new patch coming with the patch for #101527
Depends on: 101527
Created attachment 80915 [details] [diff] [review]
update patch, includes fix to xpcom for moveTo() issue on win2k
Attachment #80659 - Attachment is obsolete: true
just built and tested the latest patch on mac, works fine.

so good on mac, windows, and linux.

what's left before this can land:

1) land fix for bug #101527
2) get final reviews of ns and mozilla patches
3) finish running the profile function tests:
 http://www.mozilla.org/profilemanager/checklist.txt
 [internal] http://slip/projects/mojo/profile/65-profileindex.htm

eta to land, thursday, 4/25
Whiteboard: [adt1],[hitlist] reviewing,testing patch, eta to land on trunk 4/24 → [adt1],[hitlist] need reviews, more testing, eta 4/25
Whiteboard: [adt1],[hitlist] need reviews, more testing, eta 4/25 → [adt1],[hitlist] need reviews, more testing, eta to land on trunk 4/25

Comment 67

16 years ago
whoops, should have read all the comments for that null delete check.

some K&R braces style crept in here, even though the file seems to be mostly
using the One True Braces Style.
+    if (rhs.resolvedLocation) {
+        regLocationData.Truncate(0);
+        rv = rhs.resolvedLocation->Clone(getter_AddRefs(file));
         if (NS_SUCCEEDED(rv))
             resolvedLocation = do_QueryInterface(file);
+        file = nsnull;
+    }
+    else
+        regLocationData = rhs.regLocationData;
+    
+    migratedFrom = nsnull;    
+    if (rhs.migratedFrom) {

Should you use PRTime instead of PRInt64 for vars that are the result of
PR_Now() and the like? Although they are equivalent, it makes the code a little
clearer and perhaps more portable?

Why use an autostring in a class? Shouldn't we just use an nsString?  
nsAutoString mCurrentProfileName;

more later...
> some K&R braces style crept in here, even though the file seems to be mostly
using the One True Braces Style.

Don't even get started on the indentation and whitespace of this code. The whole
profile module is a mish-mash of various styles.  If not for wiping out CVS
blame in whatever the current rev is, I would run it though a formatting tool
and make it consistent. I might anyway.

> Should you use PRTime instead of PRInt64 for vars that are the result of
PR_Now() and the like?

The unit of time for a PRTime is a microsecond. Since we divide the value from
PR_Now to get millisecs, I think it's less confusing as a PRInt64.
> Why use an autostring in a class? Shouldn't we just use an nsString?  
> nsAutoString mCurrentProfileName;

good catch.  yes, we should be using nsString instead.  I'll fix that, look 
into the PRTime / PRInt64 issue and attach a new patch when I'm done testing.
ok, testing has gone well.

this patch introduces one assertion (see #140158).  But I think we can land 
with this assertion.

I've found other assertions when migrating, but they are not related.  (see 
#140156 and #140157)

updated patch coming soon...
Blocks: 140158
Created attachment 81052 [details] [diff] [review]
updated patch
Attachment #80915 - Attachment is obsolete: true

Comment 72

16 years ago
Comment on attachment 81052 [details] [diff] [review]
updated patch

sr=bienvenu
Attachment #81052 - Flags: superreview+
Comment on attachment 81052 [details] [diff] [review]
updated patch

r=sspitzer on the parts of this patch that ccarlen wrote.  I'd like him to
review the parts I wrote, and bhuvan to review the whole enchilada.
Comment on attachment 81052 [details] [diff] [review]
updated patch

r=ccarlen
Attachment #81052 - Flags: superreview+ → review+

Comment 75

16 years ago
Comment on attachment 81052 [details] [diff] [review]
updated patch

r=bhuvan.

Seth, can you also just run few tests on old/new activation models ? Will send
you mail on those basic tests. thanks.

Comment 76

16 years ago
Comment on attachment 80539 [details] [diff] [review]
ns tree diff (to set the interval to 6 months)

r=bhuvan
Attachment #80539 - Flags: review+
note to gbush:

bhuvan asked me to run through these tests:

1. Go through old activation. activate. Browser comes up. Close the browser and 
launch with that profile again. You should not see the activation window again
2. same thing to be repeated by cancelling activation window (cancel + cancel) 
in old activation. Close the browser widnow. Launch with that profile. You 
should not see the activation window

Above steps to be repeated with new activation. Change 
preference "browser.test_new_activation" to true in all-ns.js for new 
activation dialogs.

I did, and it worked fine.

fixed landed on trunk, will seek approval for the branch.
Keywords: adt1.0.0
Summary: Detect defunct previously migrated profiles and re-migrate → add support for re-migration of defunct previously migrated profiles
Whiteboard: [adt1],[hitlist] need reviews, more testing, eta to land on trunk 4/25 → [adt1],[hitlist] [fixed on trunk]
more notes for my pal Grace:

the pref is an int pref, "profile.seconds_until_defunct"

on mozilla, it defaults to -1, which means "never offer to re-migrate."
in ns, it defaults the number of seconds in 6 months.

to testing, I suggest setting it to 10 (in all-ns.js)

pref("profile.seconds_until_defunct", 10);

That way you don't have to mess with your system clock to test.  You will have 
to mess with the system clock once (per platform) to make sure we really do the 
right thing after six months.

Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Whiteboard: [adt1],[hitlist] [fixed on trunk] → [adt1],[hitlist] [fixed on trunk] [m5+]

Comment 79

16 years ago
help!!
I cannot get this to work.
My steps: 
set system date to 7/29/01
launch 6.0 and migrate a 4.x profile, Abby (set with refdesk as home page)
change home page to mozilla
exit and verify prefs.js has correct time stamp
change system date to today 4/29/02
launch MachV
at UI requesting a new migration say Yes 
verify home page is refdesk (4.x profile) 

I am not seeing
1. the correct home page ( I still see the setting I made in 6.0)
2. I do not see a  copy of .slt  dire (.slt-old)

What I do see in my Abby dir is 4 .slt files only 1 of which is accessible 
(other 3 show as deleted and may be result of my multiple tests)

from the registry: after second migration
Common/Profiles
    CurrentProfile (S)=Abby
    OldRegDataMoved (S)=yes
    HavePregInfo (S)=
    Version (S)=1.0
Common/Profiles/Abby
    LastModTime (B)=$\0x87\0x8a\0x82\0xed\0x00\0x00\0x00
    CreationTime (B)=\0x00\0x00\0x00\0x00\0x00\0x00\0x00\0x00
    NCHavePregInfo (S)=yes
    NCEmailAddress (S)=
    NCServiceDenial (S)=1
    NCProfileName (S)=
    migrated (S)=yes
    directory (S)=C:\WINNT\PROFILES\GBUSH\APPLICATION 
DATA\Mozilla\Users50\Abby\leaoycuf.slt
Common/Profiles/gbush
    LastModTime (B)=\0x00\0x00\0x00\0x00\0x00\0x00\0x00\0x00
    CreationTime (B)=\0x00\0x00\0x00\0x00\0x00\0x00\0x00\0x00
    NCHavePregInfo (S)=
    NCEmailAddress (S)=
    NCServiceDenial (S)=
    NCProfileName (S)=
    migrated (S)=no
    directory (S)=C:\Program Files\Netscape\Users\gbush
re-opening, this is not working properly on windows.  [but appears to work on
mac and linux].

I met with grace, and here's a summary of the problems she's seeing:

win98:
on re-migration, *no migration progress dialog*, defunct 6.x profile is moved to
side (she gets a -old), 4.x profile is not re-migrated, *application hangs*.

winnt:
on remigration, *no migration progress dialog*, defunct 6.x profile is moved to
side (she gets a -old), 4.x profile appears to be migrated, but is removed, and
*-old is restored*, (failure would cause this), when app starts we're back to
our 6.x defuct profile.

win2k:
I think grace saw the same problems as winnt

I'm going to:
1) try out an optimized build on my win2k box, as I didn't see these problems on
my win2k debug build.
2) get a special win32 build to use to find out what's going on.

I'll update with my findings.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [adt1],[hitlist] [fixed on trunk] [m5+] → [adt1],[hitlist] [eta 5/1] [m5+]
some info:

I tried the mozilla, optimized trunk bits on my win2k box.

I'm seeing something similar to what gbush saw on her win98:

the app freezes.

Here's the stack, without symbols:

XPCOM! 60e73266()
XPCOM! 60e728ff()
XPCOM! 60e4872a()
XPCOM! 60e4866a()
XPCOM! 60e48e4e()
XPCOM! 60e490e2()
APPSHELL! 60956ead()
APPSHELL! 60956926()
MOZILLA! 00402cad()
MOZILLA! 00401efc()
MOZILLA! 00401ac9()
MOZILLA! 004036a4()
KERNEL32! 77e87903()

this makes me think it's dying in nsLocalFileWin.cpp, in MoveTo(), which I 
recently tweaked.  that would explain why it's windows only.

investigating...
Status: REOPENED → ASSIGNED
on second thought, it if was moveTo(), profile would be on the stack.  so it
must be something else.

I'll continue to investigate...
I'm seeing *tons* of assertions in my windows debug build, in xpcom / string. 
ones that weren't there when I checked in.

I wonder if this is related the the failure?  Still investigating...
here's the first assertion:

NTDLL! 77f9f9df()
nsDebug::Assertion(const char * 0x1012a808 `string', const char * 0x1012a848 
`string', const char * 0x1012a6d0 `string', int 0x00000164) line 291 + 13 bytes
nsWritingIterator<char>::write(const char * 0x0012fbbc, unsigned int 
0x00000008) line 356 + 37 bytes
nsCharSinkTraits<nsWritingIterator<char> >::write(nsWritingIterator<char> & 
{...}, const char * 0x0012fbbc, unsigned int 0x00000008) line 559
copy_string(nsReadingIterator<char> & {...}, const nsReadingIterator<char> & 
{...}, nsWritingIterator<char> & {...}) line 90 + 39 bytes
nsACString::do_AppendFromReadable(const nsACString & {...}) line 881 + 55 bytes
nsACString::AppendFromPromise(const nsACString & {...}) line 857
nsACString::Append(const nsAPromiseCString & {...}) line 673
nsLocalFile::AppendRelativeNativePath(nsLocalFile * const 0x0179ddd8, const 
nsACString & {...}) line 869 + 44 bytes
nsLocalFile::AppendNative(nsLocalFile * const 0x0179ddd8, const nsACString & 
{...}) line 855
nsLocalFile::CopyMove(nsIFile * 0x00000000, const nsACString & {...}, int 
0x00000000, int 0x00000001) line 1184
nsLocalFile::MoveToNative(nsLocalFile * const 0x0179ddd8, nsIFile * 0x00000000, 
const nsACString & {...}) line 1205
nsProfile::RemigrateProfile(nsProfile * const 0x01787230, const unsigned short 
* 0x0178aa88) line 2310 + 74 bytes
nsAppShellService::CheckAndRemigrateDefunctProfile() line 389 + 42 bytes
nsAppShellService::DoProfileStartup(nsAppShellService * const 0x01787e08, 
nsICmdLineService * 0x01229e58, int 0x00000001) line 256 + 8 bytes
InitializeProfileService(nsICmdLineService * 0x01229e58) line 1086 + 31 bytes
main1(int 0x00000004, char * * 0x00304b80, nsISupports * 0x00000000) line 1384 
+ 14 bytes
main(int 0x00000004, char * * 0x00304b80) line 1779 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()
in my debug build, I loop forever with these assertions:

###!!! ASSERTION: |copy_string| will never terminate: 'count_copied > 0', file .
.\..\dist\include\string\nsAlgorithm.h, line 91
###!!! ASSERTION: You can't |write| into an |nsWritingIterator| with no space!:
'size_forward() > 0', file ..\..\dist\include\string\nsStringIterator.h, line 35
6
###!!! ASSERTION: |copy_string| will never terminate: 'count_copied > 0', file .
.\..\dist\include\string\nsAlgorithm.h, line 91
###!!! ASSERTION: You can't |write| into an |nsWritingIterator| with no space!:
'size_forward() > 0', file ..\..\dist\include\string\nsStringIterator.h, line 35
6
Created attachment 81635 [details] [diff] [review]
string issue workaround patch

Comment 87

16 years ago
Comment on attachment 81635 [details] [diff] [review]
string issue workaround patch

sr=alecf, or use NS_NAMED_LITERAL_CSTRING, sr= with that as well.
Attachment #81635 - Flags: superreview+

Comment 88

16 years ago
Comment on attachment 81635 [details] [diff] [review]
string issue workaround patch

see http://www.mozilla.org/projects/xpcom/string-guide.html#Concatenation for
more details
using NAMED_LITERAL_CSTRING had the same assertions as NS_LITERAL_STRING, 
unless I'm not doing what you suggested.

going with the original patch, I'll discuss with this alecf more tomorrow.

I'm able to re-migrate again, marking fixed so that gbush will try with the 
trunk builds again.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 90

16 years ago
update: single profile case is working on windows :)))))
moving on to other tests/platforms

Comment 91

16 years ago
Has a bug been filed on the underlying issue?
The underlying issue in the string code is bug 70083.  The hang case is exactly
the same as the one reported in bug 74709.  In particular (based on the stack in
comment 84), you're passing an nsPromiseConcatenation to
nsLocalFile::MoveToNative, and then nsLocalFile::AppendRelativeNativePath makes
another nsPromiseConcatenation with the first promise as the second part.  The
current fragment API is insufficient to handle that case.  (See why I think bug
70083 should be fixed before we freeze nsAString?)
er, nsPromiseConcatenation is called nsDependentConcatenation now.

Comment 94

16 years ago
I'm seeing a lot of crashes with the following stack in recent MozillaTrunk builds:
nsLocalFile::Append
[nsLocalFileCommon.cpp  line 271] 
	 CSpellChecker::CSpellChecker
[spellchk.cpp  line 369] 
	 nsSpellCheckGlue::ReplaceAllOccurrences
[nsSpellCheckGlue.cpp  line 1137] 
	 nsSpellCheckGlue::NextMisspelledWord
[nsSpellCheckGlue.cpp  line 272] 
	 nsQueryElementAt::nsQueryElementAt  
	 nsComponentManager::CreateInstance
[nsComponentManagerObsolete.cpp  line 115] 
	 nsEditorShell::InitSpellChecker
[nsEditorShell.cpp  line 3843] 
	 XPTC_InvokeByIndex
[xptcinvoke.cpp  line 106] 
	 XPCWrappedNative::CallMethod
[xpcwrappednative.cpp  line 1995] 
	 XPC_WN_CallMethod
[xpcwrappednativejsops.cpp  line 1267] 
	 js_Invoke
[jsinterp.c  line 790] 
	 js_Interpret
[jsinterp.c  line 2744] 
	 js_Invoke
[jsinterp.c  line 806] 
	 js_InternalInvoke
[jsinterp.c  line 881] 
	 JS_CallFunctionValue
[jsapi.c  line 3414] 
	 nsJSContext::CallEventHandler
[nsJSEnvironment.cpp  line 1019] 
	 nsJSEventListener::HandleEvent
[nsJSEventListener.cpp  line 184] 
	 nsEventListenerManager::HandleEventSubType
[nsEventListenerManager.cpp  line 1220] 
	 nsEventListenerManager::HandleEvent
[nsEventListenerManager.cpp  line 1895] 
	 GlobalWindowImpl::HandleDOMEvent
[nsGlobalWindow.cpp  line 755] 
	 DocumentViewerImpl::LoadComplete
[nsDocumentViewer.cpp  line 1538] 
	 nsDocShell::EndPageLoad
[nsDocShell.cpp  line 3958] 
	 nsWebShell::EndPageLoad
[nsWebShell.cpp  line 731] 
	 nsDocShell::OnStateChange
[nsDocShell.cpp  line 3875] 
	 nsDocLoaderImpl::FireOnStateChange
[nsDocLoader.cpp  line 1105] 
	 nsDocLoaderImpl::doStopDocumentLoad
[nsDocLoader.cpp  line 744] 
	 nsDocLoaderImpl::DocLoaderIsEmpty
[nsDocLoader.cpp  line 642] 
	 nsDocLoaderImpl::OnStopRequest
[nsDocLoader.cpp  line 573] 
	 nsLoadGroup::RemoveRequest
[nsLoadGroup.cpp  line 531] 
	 nsJARChannel::OnStopRequest
[nsJARChannel.cpp  line 612] 
	 nsOnStopRequestEvent::HandleEvent
[nsRequestObserverProxy.cpp  line 213] 
	 PL_HandleEvent
[plevent.c  line 597] 
	 PL_ProcessPendingEvents
[plevent.c  line 530] 
	 _md_EventReceiverProc
[plevent.c  line 1078]  

I'm guessing this might be related to the tweaking mentioned by Seth in comment
#81.  If that is not true, let me know so I can log a new bug for those crashes.
If it is true, I can add the topcrash info here for tracking so I can verify any
fix that goes in for this bug (or log a new bug if that would be the best thing
to do).

Comment 95

16 years ago
Mac and Linux still testing ok with single profile
Windows gave me a few succesful tests on NT with build 2002043003 on NT.  Tests 
with more than one migrated profile failed- no remigration, no old file

the later trunk build, 2002043010- on 2k and 98 are not showing a remigration 
with single profile.
:(

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 96

16 years ago
jpatel: what platform's are crashing with that stack trace?
>I'm guessing this might be related to the tweaking mentioned by Seth in comment
>#81.  If that is not true, let me know so I can log a new bug for those crashes.
>If it is true, I can add the topcrash info here for tracking so I can verify any
>fix that goes in for this bug (or log a new bug if that would be the best thing
>to do).

jpatel, that issue is seperate from this bug.  can you start a new bug (probably
on darin?) for that issue with that stack?
accepting.

to avoid getting hit by trunk regressions, we want to land this on the 1.0 and 
resume testing there.

I'm going to see drivers approval for that.
Status: REOPENED → ASSIGNED
Whiteboard: [adt1],[hitlist] [eta 5/1] [m5+] → [adt1],[hitlist] [eta 5/1] [m5+] [waiting for drivers@mozilla.org approval for the branch]

Updated

16 years ago
Keywords: approval
Whiteboard: [adt1],[hitlist] [eta 5/1] [m5+] [waiting for drivers@mozilla.org approval for the branch] → [adt1],[hitlist] [ETA 5/2] [m5+] [Needs a=]

Updated

16 years ago
Blocks: 121324

Comment 99

16 years ago
Comment on attachment 81635 [details] [diff] [review]
string issue workaround patch

a=chofmann off by default on the 1.0 branch
Attachment #81635 - Flags: approval+

Comment 100

16 years ago
I have logged bug 141560 for the crash mentioned in comment #94.  Sorry for any
confusion.
Created attachment 81930 [details] [diff] [review]
supplimental patch, for safer approach that avoids the XUL.mfl in use problem.

grace had found a problem where in certain scenarios, we'd fail to remigrate.
I debugged and found that the problem was we were failing to move the existing
xxxxxxxx.slt directory to the side, because one of the files (XUL.mfl) was in
use.

Here's a new patch, that similifies how remigration works on disk.

currently we do this:

move xxxxxxxx.slt to xxxxxxxx.slt-old 
remigrate the 4.x profile to xxxxxxxx.slt

with this patch, we do this:

leave xxxxxxxx.slt alone
remigrate the 4.x profile to xxxxxxxx.slt-new

This is faster (no extra moving of files), safer (the existing .slt directory
is untouched, so if we have to restore, we can), and less error prone (no issue
if XUL.mfl is in use, or if moving the existing .slt folder fails.)

I'll get reviews for this on the trunk, land it, let it bake, and then prepare
something for the branch.
with this patch, we do this:

>leave xxxxxxxx.slt alone
>remigrate the 4.x profile to xxxxxxxx.slt-new

With the new profile dir having a new name, all of the absolute paths in the
old, backed-up profile are now invalid, right? The reason I did it the way it's
being done was so that the absolute paths in old vs. new would be the same so
"*-old" was useful for back-up purposes.

Little did I realize that moving a profile dir was going to be so fraught with
peril :-/ Also, the profile is shut down before it is moved so nothing should
still have open files. What component uses XUL.mfl?

Comment 103

16 years ago
Comment on attachment 81635 [details] [diff] [review]
string issue workaround patch

withdraw a= until in-use issues are worked out.
Attachment #81635 - Flags: approval+

Comment 104

16 years ago
Comment on attachment 81930 [details] [diff] [review]
supplimental patch, for safer approach that avoids the XUL.mfl in use problem.

sr=bienvenu
Attachment #81930 - Flags: superreview+
>With the new profile dir having a new name, all of the absolute paths in the
>old, backed-up profile are now invalid, right? The reason I did it the way it's
>being done was so that the absolute paths in old vs. new would be the same so
>"*-old" was useful for back-up purposes.

you mean, a user could just rename .slt-old to .slt to get the profile we moved
aside.  But not, since the registry points to .slt-new, they can't easily
restore the old .slt directory.  (renaming .slt to .slt-new will not work.)

Doesn't our profile UI allow users to pick existing folders?  To restore an old
6.0 profile, can't they use the UI and pick the existing .slt directory?  I'll
investigate that.

I haven't determined what was using XUL.mfl, but I could determine that the file
was in use.  (the windows error code we got while doing the move was
ERROR_SHARING_VIOLATION)
taking to ccarlen, restore is harder in the "-new" world, but still doable.
the user has to rename a folder and then hand edit their prefs.js

in the "-old" world, they could just rename a folder.

but "-new" is less risky, restoring should be rare.  I'll prepare a relnote on
how to do it.
Comment on attachment 81930 [details] [diff] [review]
supplimental patch, for safer approach that avoids the XUL.mfl in use problem.

r=ccarlen
Attachment #81930 - Flags: review+
Missed something:

+    // XXX todo: what if <xxxxxxxx>.slt-new already exists?
+    nsCAutoString newDirLeafName(origDirLeafName + NS_LITERAL_CSTRING("-new"));
+    rv = newProfileDir->SetLeafName(newDirLeafName.get());
     NS_ENSURE_SUCCESS(rv,rv);
      
     // Create a new directory for the remigrated profile
     rv = newProfileDir->Create(nsIFile::DIRECTORY_TYPE, 0775);

Since now we're actually creating the dir, avoiding the dup (useful for QA) is
just a matter of calling CreateUnique() instead of Create()
Created attachment 81962 [details] [diff] [review]
same patch, with CreateUnique()
Attachment #81930 - Attachment is obsolete: true
Comment on attachment 81962 [details] [diff] [review]
same patch, with CreateUnique()

forwarding r=ccarlen, sr=bienvenu
Attachment #81962 - Flags: superreview+
Attachment #81962 - Flags: review+
fixed on trunk.  seeking driver a= now...
Whiteboard: [adt1],[hitlist] [ETA 5/2] [m5+] [Needs a=] → [adt1],[hitlist] [ETA 5/2] [m5+] [Needs a=][fixed on trunk]

Comment 112

16 years ago
Adding adt1.0.0+.  After getting drivers approval, please check this into the
branch and add the fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
(Reporter)

Comment 113

16 years ago
Are the comments about having to edit the prefs.js caused by not being able to
choose a salted profile in the UI?  I seem to recall that we forcibly add the
profile name to the end of the selected directory name.  At some point, we
should detect the presence of a salted subdirectory use it.  I think doing that
would side-step the issues mentioned above.
Created attachment 81999 [details] [diff] [review]
the patch for the branch.
windows, mozilla, branch bits:

http://warp.mcom.com/u/sspitzer/mozilla-opt-branch.zip

and soon:

ftp://ftp.mozilla.org/pub/mozilla/nightly/experimental/bug-137886/mozilla-win32-
opt-branch.zip

I'm working on linux, mozilla, branch bits.

Comment 117

16 years ago
preliminary testing on all platforms for  commercial trunk builds is successful

Also tested mozilla builds on win and linux branch- small subset of test and 
these were successful also

Looking at an appreg or two, I find some absolute directory names including the
salt directory, which would make it hard to rename a salted profile.
> Looking at an appreg or two, I find some absolute directory names including the
> salt directory, which would make it hard to rename a salted profile.

I explained this over the phone.

The appreg contains the path to <xxxxxxx>.slt-new, yes.

So, to restore you'd have to move .slt-new to the side, rename .slt to .slt-new,
and then edit prefs.js, globally replacign ".slt" with ".slt-new"

Comment 120

16 years ago
Does Grace's testing suffice?  If not what more can she do, so we can get an a= thx

Comment 121

16 years ago
Comment on attachment 81999 [details] [diff] [review]
the patch for the branch.

a=chofmann -pref turned off- for the 1.0 branch
Attachment #81999 - Flags: approval+
fixed on branch.  marking fixed.

thanks to grace for all her testing this week and thanks to ccarlen for his
initial patch, which was the bulk of the fix.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Seth says its fixed on the branch, so fixed1.0.0. :-)
Keywords: fixed1.0.0

Comment 124

16 years ago
tested all platforms on branch builds for 5/7 
marking verified1.0.0

finished testing on trunk builds all platforms 5/5
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.