sairuh (rarely reading bugmail) Reporter
	Description • 20 years ago

hm, I thought there might be a bug on this already, but I cannot seem to find
it. pls dup as needed. found using 2004119905-0.9 tbird bits on linux fc2.

this is a problem with import, not migration (I can migrate my seamonkey profile
fine).

0. I've got a mozilla (seamonkey) profile containing a POP and news account
(located under ~/.mozilla/default/).

1. make sure there's no existing ~/.thunderbird profile (for simplicity).

2. launch tbird, but do *not* migrate from mozilla/netscape. I then created an
IMAP account.

3. select Tools > Import.

4. select Settings, click Next.

results: nothing is listed in the next panel under "select the mail program."
shouldn't mozilla/netscape 6/7 be displayed there? get the following js error:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIListBoxObject.getItemAtIndex]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/listbox.xml :: getItemAtIndex :: line 456" 
data: no]

5. dismiss (cancel) the Import dialog, then bring it up again.

6. select Mail, click Next.

results: the only thing listed in the next panel under "select the mail program"
is Communicator 4.x. this is odd since I don't have Netscape 4.x installed on my
linux box --not even a ~/.netscape exists.

note: if I go ahead and click Next at this point, I encounter bug 256652.

	Tracy Walker [:tracy]
	Comment 1 • 20 years ago

I'm seeing this on Windows as well;  No listing for Mozilla/Netscape in the
Import dialog.

	OstGote!
	Comment 2 • 20 years ago

See bug 187564 and bug 22689. And bug 63389 for the mails part. Duplicate?

	sairuh (rarely reading bugmail) Reporter
	Comment 3 • 20 years ago

also occurs on Mac: tested with 2004113002-0.9 on os x 10.3.6.

	Doug Wright
	Comment 4 • 20 years ago

Not a TB auto-migration bug -> Core:MailNews:Import

	OstGote!
	Comment 5 • 20 years ago

*** Bug 279861 has been marked as a duplicate of this bug. ***

	Asa Dotzler [:asa]
	Comment 6 • 20 years ago

Is this a regression or have we always lacked this feature?

	sairuh (rarely reading bugmail) Reporter
	Comment 7 • 20 years ago

asa, this has always been a problem. see bugs referenced in comment 2 (this bug
tracks it for tbird).

	Tuukka Tolvanen (sp3000)
	Comment 8 • 19 years ago

*** Bug 250261 has been marked as a duplicate of this bug. ***

	Jo Hermans
	Comment 9 • 19 years ago

*** Bug 292932 has been marked as a duplicate of this bug. ***

	vicky.sun
	Comment 10 • 18 years ago

This bug also can be reproduced in Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.8) Gecko/20060410 Firefox/2.0a1

	Wayne Mery (:wsmwk)
	Comment 11 • 16 years ago

note impact to litmus test https://bugzilla.mozilla.org/show_bug.cgi?id=272292

	Mark Banner (:standard8)
	Comment 12 • 16 years ago

(In reply to comment #11)
> note impact to litmus test https://bugzilla.mozilla.org/show_bug.cgi?id=272292
> 
Wayne, can you provide the correct litmus link please?

	Wayne Mery (:wsmwk)
	Comment 13 • 16 years ago

I was problably looking at Al Billings' test results in TB2
  https://litmus.mozilla.org/single_result.cgi?id=122770
for testcase
  https://litmus.mozilla.org/show_test.cgi?id=1919

3.0 testcase - Import Address Book from Another E-Mail Client:
https://litmus.mozilla.org/show_test.cgi?id=5449
https://litmus.mozilla.org/show_test.cgi?id=5650
https://litmus.mozilla.org/show_test.cgi?id=5448

note: I did no testing or investigation

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 14 • 16 years ago

I ran into this as well for:
https://litmus.mozilla.org/show_test.cgi?id=1919

I had trouble importing address book and mail settings (some data made it) from Outlook Expresss 6 on Windows XP.

	Wayne Mery (:wsmwk)
	Comment 15 • 16 years ago

The whole import area/bugs needs a good assessment so for now I'm not sure where this bug should go or what it should do.  But we will want start moving on them in order to have an improved import story for Thunderbird 3.

cc: Nikolay for any thoughts.

	Wayne Mery (:wsmwk)
	Comment 16 • 15 years ago

(In reply to comment #14)
> I ran into this as well for:
> https://litmus.mozilla.org/show_test.cgi?id=1919
> 
> I had trouble importing address book and mail settings (some data made it) from
> Outlook Expresss 6 on Windows XP.

nikolay's comment to me from this time frame "This not works for me at all, when using TB3a1 - its just give me an error. Can import mail, but creates folder structure under local folder. Futher more this structure can't be deleted"

	Magnus Melin [:mkmelin]
	Comment 22 • 4 years ago

Comment on attachment 9156877 [details] [diff] [review]
272292-migration.patch

Review of attachment 9156877 [details] [diff] [review]:
-----------------------------------------------------------------

There are two modes migrate (=replace everything, runs from a command line switch), and import (=copy over x and y to my existing profile).
Import everything should not migrate, it should do the import and not destroy the current profile.

	Magnus Melin [:mkmelin]
	Comment 24 • 4 years ago

Comment on attachment 9158508 [details] [diff] [review]
272292-import-mail-accounts.patch

Review of attachment 9158508 [details] [diff] [review]:
-----------------------------------------------------------------

Please have Ben Campbell review this

::: mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp
@@ +638,5 @@
> +      // Keep the three below first, or change the indexes below
> +      "mail.identity.",   "mail.server.",     "ldap_2.",       "mail.account.",
> +      "mail.smtpserver.", "mailnews.labels.", "mailnews.tags."};
> +  PBStructArray sourceBranches[MOZ_ARRAY_LENGTH(branchNames)];
> +  uint32_t i;

please just defined i inside the for clause, for all cases where it's not used outside

@@ +751,5 @@
> +    } else if (StringEndsWith(prefName, nsDependentCString(".identities"))) {
> +      nsDependentCString identityKey(pref->stringValue);
> +      nsCString newIdentityKey;
> +      bool exist = identityKeyHashTable.Get(identityKey, &newIdentityKey);
> +      if (exist) {

I think that would be with an s, exists. But You don't need this tmp variable

@@ +783,5 @@
> +  nsCOMPtr<nsIPrefBranch> branch;
> +  nsCString newAccounts;
> +  count = newKeys.Length();
> +  if (count) {
> +    aPrefService->GetBranch("mail.accountmanager.", getter_AddRefs(branch));

there are some cases such as this where you want to do an rv check, or there will be cases where we crash.

@@ +806,5 @@
> +    PrefKeyHashTable& keyHashTable) {
> +  nsTArray<nsCString> newKeys;
> +
> +  uint32_t count = aMailServers.Length();
> +  for (uint32_t i = 0; i < count; ++i) {

let's always use the i++ version instead for consistency (won't make a difference either way)

@@ +820,5 @@
> +    nsCString newKey;
> +    bool exist = keyHashTable.Get(key, &newKey);
> +    if (!exist) {
> +      do {
> +        std::this_thread::sleep_for(std::chrono::milliseconds(500));

why this? would also add a comment on why if it's really needed

	Ben Campbell
	Comment 26 • 4 years ago

Comment on attachment 9159120 [details] [diff] [review]
272292-import-mail-accounts.patch

Review of attachment 9159120 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r+, conditional on:
1. the `PR_Sleep()` instead of `<thread>`/`<chrono>`
2. checking up on a few places where it looks like there might be unchecked errors (`psvc->SavePrefFile() etc`) which might cause the import to fail without the user seeing anything wrong.

The other stuff (range-based for and [] instead of ElementAt()) I'd recommend, but is really just personal preference.

::: mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <chrono>
> +#include <thread>

I don't know if there's an "official" stance on standard C++ library use, but the existing code seems to favour using Mozilla-specific equivalents, when there are any.
In this case, `PR_Sleep()` is probably the one to use.

@@ +610,5 @@
> +/**
> + * Use the current Thunderbird's prefs.js as base, transform branches of
> + * Seamonkey's prefs.js so that those branches can be imported without conflicts
> + * or overwriting.
> + */

Just wanted to say that I wish _every_ function in the codebase had a comment like this above it!
Lovely and concise and clearly stating intent.

@@ +624,5 @@
> +  // base later.
> +  nsCOMPtr<nsIFile> targetPrefsFile;
> +  mTargetProfile->Clone(getter_AddRefs(targetPrefsFile));
> +  targetPrefsFile->Append(aTargetPrefFileName + NS_LITERAL_STRING(".orig"));
> +  psvc->SavePrefFile(targetPrefsFile);

Should there be an error check here, in case SavePrefFile() fails?
(same with the other psvc calls too)

@@ +687,5 @@
> +  nsTArray<nsCString> newKeys;
> +
> +  uint32_t count = aIdentities.Length();
> +  for (uint32_t i = 0; i < count; i++) {
> +    PrefBranchStruct* pref = aIdentities.ElementAt(i);

Given that you don't need `count` or `i`  elsewhere, these three lines could be replaced with a range-based for loop, eg:
```
for (auto pref : aIdentities) {
```

@@ +691,5 @@
> +    PrefBranchStruct* pref = aIdentities.ElementAt(i);
> +    nsDependentCString prefName(pref->prefName);
> +    nsTArray<nsCString> keys;
> +    ParseString(prefName, '.', keys);
> +    auto key = keys.ElementAt(0);

It's fine to use `keys[0]` instead of `ElementAt(0)` if you prefer. They are equivalent, and both require the index to be within bounds.
(And if ParseString() can return an empty keys array you'll need to add a check anyway).

@@ +738,5 @@
> +  nsTArray<nsCString> newKeys;
> +
> +  uint32_t count = aAccounts.Length();
> +  for (uint32_t i = 0; i < count; i++) {
> +    PrefBranchStruct* pref = aAccounts.ElementAt(i);

Another place for `for (auto pref : aIdentities) {` maybe?

@@ +742,5 @@
> +    PrefBranchStruct* pref = aAccounts.ElementAt(i);
> +    nsDependentCString prefName(pref->prefName);
> +    nsTArray<nsCString> keys;
> +    ParseString(prefName, '.', keys);
> +    auto key = keys.ElementAt(0);

keys[0]?

@@ +809,5 @@
> +  nsTArray<nsCString> newKeys;
> +
> +  uint32_t count = aMailServers.Length();
> +  for (uint32_t i = 0; i < count; i++) {
> +    PrefBranchStruct* pref = aMailServers.ElementAt(i);

`for (auto pref : aIdentities) {`?

@@ +813,5 @@
> +    PrefBranchStruct* pref = aMailServers.ElementAt(i);
> +    nsDependentCString prefName(pref->prefName);
> +    nsTArray<nsCString> keys;
> +    ParseString(prefName, '.', keys);
> +    auto key = keys.ElementAt(0);

`keys[0]`?

@@ +824,5 @@
> +      do {
> +        // Since updating prefs.js is batched, GetUniqueServerKey may return the
> +        // previous key. Sleep 500ms and check if the returned key already
> +        // exists to workaround it.
> +        std::this_thread::sleep_for(std::chrono::milliseconds(500));

`PR_Sleep(PR_MillisecondsToInterval(500));`

@@ +869,5 @@
> +
> +  uint32_t count = aServers.Length();
> +  for (uint32_t i = 0; i < count; i++) {
> +    PrefBranchStruct* pref = aServers.ElementAt(i);
> +    nsDependentCString prefName(pref->prefName);

`for (auto pref : aIdentities) {`?

@@ +872,5 @@
> +    PrefBranchStruct* pref = aServers.ElementAt(i);
> +    nsDependentCString prefName(pref->prefName);
> +    nsTArray<nsCString> keys;
> +    ParseString(prefName, '.', keys);
> +    auto key = keys.ElementAt(0);

keys[0]?

@@ +960,1 @@
>      PrefBranchStruct* pref = aPrefs.ElementAt(i);

`for (auto pref : aIdentities) {`?

	Ben Campbell
	Comment 34 • 4 years ago

Comment on attachment 9160554 [details] [diff] [review]
272292-import-addrbook.patch

Review of attachment 9160554 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp
@@ +365,5 @@
> +/**
> + * Copy .mab (Mork files) from Seamonkey profile to Thunderbird profile. The
> + * restarting after migration will convert .mab to .sqlite, so we don't need to
> + * read and convert the .mab here.
> + *

nit: I'd trim the blank comment line.

@@ +750,5 @@
>      WriteBranch(branchNames[i], psvc, sourceBranches[i]);
>  
> +  smtpServerKeyHashTable.Clear();
> +  identityKeyHashTable.Clear();
> +  serverKeyHashTable.Clear();

Are these needed?
Genuine question. They're not doing any harm, but I'd have expected the destructor to clear them out auotmatically.
(Digging down through the `nsDataHashTable` implementation I _think_ it cleans up in the dtor, but it's tricky to be 100% sure without really picking through the details).

@@ +870,5 @@
>    if (count) {
>      (void)branch->SetCharPref("accounts", newAccounts);
>    }
>  
> +  keyHashTable.Clear();

same here

@@ +1031,5 @@
> +    }
> +    pref->prefName = moz_xstrdup(prefName.get());
> +  }
> +
> +  keyHashTable.Clear();

and here

	Ben Campbell
	Comment 39 • 4 years ago

Comment on attachment 9161527 [details] [diff] [review]
[landed] 272292-import-separately.patch

Review of attachment 9161527 [details] [diff] [review]:
-----------------------------------------------------------------

(sorry about the slow response - I've been mostly away the last week or so)

And the patch looks fine to me.

::: mailnews/import/seamonkey/src/SeamonkeyImport.jsm
@@ +9,5 @@
> +let seamonkeyImportMsgs = Services.strings.createBundle(
> +  "chrome://messenger/locale/seamonkeyImportMsgs.properties"
> +);
> +
> +var EXPORTED_SYMBOLS = ["SeamonkeyImport"];

Would this line be better as:
```
const EXPORTED_SYMBOLS = ["SeamonkeyImport"];
```
?
The code base seems all over the place on this. There are 220 uses of `var EXPORTED_SYMBOLS` and 202 uses of `const EXPORTED_SYMBOLS`. I'm assuming they are equivalent...

	Magnus Melin [:mkmelin]
	Comment 43 • 4 years ago

Comment on attachment 9162726 [details] [diff] [review]
272292-import-mail.patch

Review of attachment 9162726 [details] [diff] [review]:
-----------------------------------------------------------------

If you run it on the main thread the UI will be blocked during the operation. It's not the end of the day perhaps, but especially if it takes a long time someone will think the application froze and force shut it down.
So you could collect the information first in XPCOM, then do the actual copy operations in a worker. See mailstoreConverter.jsm for an example of how we do this when converting maildir<->mbox

::: mail/components/migration/public/nsIMailProfileMigrator.idl
@@ +61,5 @@
>     * not support profiles, this attribute is null.
>     */
>    readonly attribute nsIArray sourceProfiles;
> +
> +  readonly attribute nsIArray sourceProfileLocations;

please make this ```readonly attribute Array<nsIFile> sourceProfileLocations;```

(we're trying to get rid of nsIArray. Array lets you use it like normal js arrays, Ben is working on that)

	Ben Campbell
	Comment 48 • 4 years ago

Comment on attachment 9164662 [details] [diff] [review]
272292-import-mail.patch

Review of attachment 9164662 [details] [diff] [review]:
-----------------------------------------------------------------

It all looks good, although I'd recommend replacing the `nsISupportsString` use, and removing `SeamonkeyImportWorker.js` (unless you want to keep it in as a stub).
I guess they're big enough changes to justify another review, so I'll wait for an updated patch rather than adding r+ on this one...

::: mail/components/migration/public/nsIMailProfileMigrator.idl
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
>  interface nsIArray;

I think this can be deleted now.

@@ +61,5 @@
>    /**
>     * An enumeration of available profiles. If the import source does
>     * not support profiles, this attribute is null.
>     */
> +  readonly attribute Array<nsISupportsString> sourceProfiles;

I think `Array<AString>` would be the type to use here.
I _think_ the original need for `nsISupportsString` was just so it could be stuffed into an `nsIArray`.

The `Array<AString>` would appear as `const nsTArray<RefPtr<nsAString>>&` in C++ and as a plain javascript array of plain javascript strings on the JS side.

There are some docs at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPIDL although it's not that clear about arrays of strings.

@@ +63,5 @@
>     * not support profiles, this attribute is null.
>     */
> +  readonly attribute Array<nsISupportsString> sourceProfiles;
> +
> +  readonly attribute Array<nsIFile> sourceProfileLocations;

This is part of the public interface, so it'd be nice to have a short comment explaining it.

::: mail/components/migration/src/nsNetscapeProfileMigratorBase.cpp
@@ +40,5 @@
>  NS_IMPL_ISUPPORTS(nsNetscapeProfileMigratorBase, nsIMailProfileMigrator,
>                    nsITimerCallback)
>  
>  nsresult nsNetscapeProfileMigratorBase::GetProfileDataFromProfilesIni(
> +    nsIFile* aDataDir, nsTArray<nsCOMPtr<nsISupportsString>>& aProfileNames,

`nsTArray<nsCOMPtr<nsISupportsString>>&` => `nsTArray<RefPtr<nsAString>>&`

There's nothing wrong with `nsCOMPtr` over `RefPtr`, but using `RefPtr` should make it easier to implement your `GetSourceProfileLocations()` and `GetSourceProfiles()`. You'll be able to just Clone() the arrays rather than Clear()/AddElements().

::: mail/components/migration/src/nsNetscapeProfileMigratorBase.h
@@ +91,5 @@
>    void CopyNextFolder();
>    void EndCopyFolders();
>  
> +  nsresult GetProfileDataFromProfilesIni(
> +      nsIFile* aDataDir, nsTArray<nsCOMPtr<nsISupportsString>>& aProfileNames,

`nsCOMPtr<nsISupportsString>` => `RefPtr<nsAString>`

::: mail/components/migration/src/nsOutlookProfileMigrator.cpp
@@ +115,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsOutlookProfileMigrator::GetSourceProfiles(
> +    nsTArray<RefPtr<nsISupportsString>>& aResult) {

`nsISupportsString` => `nsAString`.
Also, is it worth adding `aResult.Clear()`?
I can't imagine there would ever be anything already in the array... but just to make it clear that the intended result is an empty set.

::: mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp
@@ +166,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsSeamonkeyProfileMigrator::GetSourceProfiles(
> +    nsTArray<RefPtr<nsISupportsString>>& aResult) {

`nsISupportsString` => `nsAString`

@@ +173,5 @@
>      FillProfileDataFromSeamonkeyRegistry();
>    }
>  
> +  aResult.Clear();
> +  aResult.AppendElements(mProfileNames);

Could be 
````
aResult = mProfileNames.Clone();
```
(if `mProfiles` uses `RefPtr` rather than `nsCOMPtr`)

@@ +186,5 @@
> +    FillProfileDataFromSeamonkeyRegistry();
> +  }
> +
> +  aResult.Clear();
> +  aResult.AppendElements(mProfileLocations);

Again, could be Clone()

::: mail/components/migration/src/nsSeamonkeyProfileMigrator.h
@@ +26,5 @@
>                       const char16_t* aProfile) override;
>    NS_IMETHOD GetMigrateData(const char16_t* aProfile, bool aReplace,
>                              uint16_t* aResult) override;
> +  NS_IMETHOD GetSourceProfiles(
> +      nsTArray<RefPtr<nsISupportsString>>& aResult) override;

`nsAString`

@@ +77,5 @@
>                     PBStructArray& aPrefs, bool deallocate = true);
>  
>   private:
> +  nsTArray<nsCOMPtr<nsISupportsString>> mProfileNames;
> +  nsTArray<nsCOMPtr<nsIFile>> mProfileLocations;

`nsAString` and `RefPtr`.

And just to be clear: nothing wrong with nsCOMPtr, just that RefPtr seems more consistent.

::: mailnews/import/seamonkey/src/SeamonkeyImportWorker.js
@@ +8,5 @@
> +
> +self.addEventListener("message", function(e) {
> +  OS.File.copy(e.data.srcPath, e.data.dstPath);
> +  self.postMessage({ msg: "success" });
> +});

I don't think this worker is actually used in this patch, is it?
If it isn't, I'd probably remove it.
(and if you want to keep it in as a stub, I'd add a comment to say it's not used now, but the intention is to use it in future).

	Ben Campbell
	Comment 51 • 4 years ago

Comment on attachment 9165299 [details] [diff] [review]
272292-import-mail.patch

Review of attachment 9165299 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me!

	Ping Chen [:rnons] (away) Assignee
	Comment 61 • 4 years ago

Comment on attachment 9159614 [details] [diff] [review]
[landed] 272292-import-mail-accounts.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Can't import mail accounts from Seamonkey in Import Dialog > Import Everything 
Testing completed (on c-c, etc.):  Manually tested
Risk to taking this patch (and alternatives if risky): Only affects importing Seamonkey

	Ping Chen [:rnons] (away) Assignee
	Comment 62 • 4 years ago

Comment on attachment 9160850 [details] [diff] [review]
[landed] 272292-import-addrbook.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Can't import address books from Seamonkey in Import Dialog > Import Everything 
Testing completed (on c-c, etc.):  Manually tested
Risk to taking this patch (and alternatives if risky): Only affects importing Seamonkey

	Ping Chen [:rnons] (away) Assignee
	Comment 63 • 4 years ago

Comment on attachment 9161527 [details] [diff] [review]
[landed] 272292-import-separately.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Can't import mail accounts and address books from Seamonkey in Import Dialog > Settings / Address books
Testing completed (on c-c, etc.):  Manually tested
Risk to taking this patch (and alternatives if risky): Only affects importing Seamonkey

	Ping Chen [:rnons] (away) Assignee
	Comment 64 • 4 years ago

Comment on attachment 9165799 [details] [diff] [review]
[landed] 272292-import-mail.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Can't import mails from Seamonkey in Import Dialog > Mail
Testing completed (on c-c, etc.):  Manually tested
Risk to taking this patch (and alternatives if risky): All mail importers run in the main thread now, but they didn't work anyway.

	Wayne Mery (:wsmwk)
	Comment 65 • 4 years ago

Comment on attachment 9165799 [details] [diff] [review]
[landed] 272292-import-mail.patch

[Triage Comment]
Approved for esr78

	Wayne Mery (:wsmwk)
	Comment 66 • 4 years ago

Comment on attachment 9159614 [details] [diff] [review]
[landed] 272292-import-mail-accounts.patch

[Triage Comment]
Approved for esr78

	Wayne Mery (:wsmwk)
	Comment 67 • 4 years ago

Comment on attachment 9160850 [details] [diff] [review]
[landed] 272292-import-addrbook.patch

[Triage Comment]
Approved for esr78

	Wayne Mery (:wsmwk)
	Comment 68 • 4 years ago

Comment on attachment 9161527 [details] [diff] [review]
[landed] 272292-import-separately.patch

[Triage Comment]
Approved for esr78