272292 - unable to import Seamonkey data into Thunderbird

Status: RESOLVED FIXED

(MailNews Core :: Import, enhancement, P2)

Description

[sairuh (rarely reading bugmail)]

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.

Comment 1

[Tracy Walker [:tracy]]

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

Comment 2

[OstGote!]

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

Comment 3

[sairuh (rarely reading bugmail)]

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

Comment 4

[Doug Wright]

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

Comment 5

[OstGote!]

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

Comment 6

[Asa Dotzler [:asa]]

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

Comment 7

[sairuh (rarely reading bugmail)]

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

Comment 8

[Tuukka Tolvanen (sp3000)]

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

Comment 9

[Jo Hermans]

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

Comment 10

[vicky.sun]

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

Comment 11

[Wayne Mery (:wsmwk)]

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

Comment 12

[Mark Banner (:standard8)]

(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?

Comment 13

[Wayne Mery (:wsmwk)]

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

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 15

[Wayne Mery (:wsmwk)]

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.

Comment 16

[Wayne Mery (:wsmwk)]

(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"

Comment 18

[Flex]

I can confirm that the import assistent still is broken in Thunderbird 68.4.1.

Use:

1. Select Tools - Import - Import Everything and click on "Next"
2. TB suggests "Seamonkey 2 or later"
3. Click on "next"

TB will immediately show the Import Complete message without importing anything.

I'm not sure how long I've been using Seamonkey, maybe I startet with 1.x, but the current installed version is 2.49.4.

Comment 19

[Magnus Melin [:mkmelin]]

We should really fix this.

Comment 21

[Ping Chen [:rnons] (away)]

Attachment: 272292-migration.patch

In the Import Dialog, there are options to import everything or import mail/addressbook separately. So there are two parts of this bug:

1. when "Import Everything" is selected, click the Next button, the Migration Dialog is shown listing "SeaMonkey 2 or later". However, by selecting SeaMonkey and clicking the Next button, migration finished immediately but nothing is imported.
2. when "Import Mail" is selected, click the Next button, "No application or file to import data from was found." is shown.

This patch fixed the first part, the migration works, but will overwrite all existing mail accounts/settings. Not sure why, it requires a restart of TB to show imported mail accounts. I was a bit worried about this overwriting at first, but maybe it's expected?

For the second part, I think we need to implement a SeaMonkey importer similar to the existing Outlook/AppleMail importer.

Comment 22

[Magnus Melin [:mkmelin]]

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.

Comment 23

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail-accounts.patch

This patch added support for importing mail and accounts from Seamonkey.
Steps: Import --> Import Everything --> Seamonkey --> Next --> Finish.

Will submit followup patches to import addressbook/filters.

Comment 24

[Magnus Melin [:mkmelin]]

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

Comment 25

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail-accounts.patch

Fixed review comments.

Comment 26

[Ben Campbell]

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) {`?

Comment 27

[Ben Campbell]

Doh! Sorry, forgot to mention clang-format. You'll want to run your patch through it before we land it:

to see the changes:

$ mach clang-format -s -p "comm/mail/components/migration/src/nsSeamonkeyProfileMigrator.h"
$ mach clang-format -s -p "comm/mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp"

if you're happy with the changes, run again without the -s to let clang-format alter the files.

The javascript changes all seem to pass linting just fine.

Comment 28

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail-accounts.patch

Thanks for your detailed review. Strangely, I didn't find anything by running ../mach clang-format, will wait for the Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=444b48d22db623b8daee3720ac1162756c9caa14

Comment 29

[Ping Chen [:rnons] (away)]

Attachment: [landed] 272292-import-mail-accounts.patch

Ran clang-format in m-c root. Thanks.

Found that ../mach clang-format inside comm folder doesn't work. Also found bug 1611580, that clang-format is not run on treeherder for c-c.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4422b4525c2f
Support importing accounts and mails from SeaMonkey. r=benc

Comment 31

[Ben Campbell]

(In reply to Ping Chen (:rnons) from comment #29)

Found that ../mach clang-format inside comm folder doesn't work.

Ahh yes - I remember this. It /does/ work inside comm, but you have to include comm/ in the path, eg

$ ../mach clang-format -s -p comm/mailnews/....

But if you forget the comm/ it doesn't complain about missing files. It just outputs nothing, which is very confusing.
(In the end, I just hacked up a shell script to run eslint and clang-format for me: https://github.com/bcampbell/tb-notes/blob/master/scripts/cclint)

Comment 32

[Ping Chen [:rnons] (away)]

Good to know, thanks for sharing. It's interesting I also have a tb-notes folder on my local, but mostly for bugs I'm working on.

Comment 33

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-addrbook.patch

Support importing addressbook from seamonkey. Steps: Import --> Import Everything --> Seamonkey --> Next --> Finish

Will work on importing things separately next.

Comment 34

[Ben Campbell]

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

Comment 35

[Ping Chen [:rnons] (away)]

Attachment: [landed] 272292-import-addrbook.patch

You're right, they are not needed. I haven't touched C++ for a few years and am picking it back. I saw a aPrefs.Clear() in the WriteBranch function and thought maybe I need to clear the key hash tables as well.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a857186a1054
Support importing addressbooks from SeaMonkey. r=benc

Comment 37

[Ping Chen [:rnons] (away)]

Attachment: [landed] 272292-import-separately.patch

This patch added a SeamonkeyImport.jsm module, so that when selecting address books or settings in the import dialog, Seamonkey is listed as an option. The actual importing is still done in nsSeamonkeyProfileMigrator.

Steps: Import --> Address Books (or Settings) --> SeaMonkey --> Next

Will work on Import --> Mail next, haven't figured out how to do it. It's a bit confusing that import mail only imports emails but not mail accounts. Have to use import settings to import mail accounts.

Comment 38

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail.patch

This patch implements importing mail from SeaMonkey, it depends on 272292-import-separately.patch. I'm asking for some feedback not review, the current situation:

1. When using PR_CreateThread, NS_DISPATCH_SYNC fails to block the thread. Take ProxyGetChildNamed as an example

  RefPtr<GetChildNamedRunnable> getChildNamed = new GetChildNamedRunnable(aFolder, aName, aChild);
  // log "before dispatch"
  nsresult rv = NS_DispatchToMainThread(getChildNamed, NS_DISPATCH_SYNC); // log "Run" inside `Run`
  // log "before dispatch"
  return getChildNamed->mResult;

Expected result is
before dispatch | Run | after dispatch

Actual result is

before dispatch | after dispatch | Run

This problem is not related to the current patch. Importing mail from outlook doesn't work on trunk.

2. Strangely, using nsIThreadManager->NewThread doesn't have problem 1. NS_DISPATCH_SYNC blocks the thread as expected and importing mail from outlook works (without error, but mails not really imported, separate issue).

3. However there is another problem, I wrote the SeamonkeyImport module in js, and js xpcom can only be run in the main thread!

Jorg, I saw your work on bug 1176748, does it make sense to run the whole import code in the main thread? Considering we used so many NS_DispatchToMainThread in not only nsImportMail.cpp, but also nsOutlookMail.cpp and nsBeckyMail.cpp. Also considering the import dialog is a modal dialog, user can't do anything like checking/reading mail when import dialog is shown. What is the benefit of running import in a separate thread?

Comment 39

[Ben Campbell]

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...

Comment 40

[Ben Campbell]

(In reply to Ping Chen (:rnons) from comment #38)

1. When using PR_CreateThread, NS_DISPATCH_SYNC fails to block the thread.

Raw PRThreads don't have an event queue, so you can't dispatch to them anyway. They are just a cross-platform low level OS thread.
But the fancy nsThread objects that nsIThreadManager creates do have an event queue. The underlying PRThread used by nsThread is stored in nsIEventTarget::mThread, which nsThread derives from (eventually).

From what I understand, NS_DISPATCH_SYNC works by entering a loop which pumps the event queue of the local thread repeatedly, checking each time to see if the dispatched event has finished executing on the remote thread.
Since PRThread don't have an event queue, I guess it just doesn't work... (a cursory reading of the code seems to indicate that the Dispatch() call will return NS_ERROR_NOT_AVAILABLE. See https://searchfox.org/mozilla-central/source/xpcom/threads/ThreadEventTarget.cpp#72 )

I didn't know any of this 30 minutes ago, but now I do :-)

Comment 41

[Ping Chen [:rnons] (away)]

Thanks, no worries, I can wait till you're back.

Raw PRThreads don't have an event queue, so you can't dispatch to them anyway.

The dispatch was sent from PRThread to main thread, because some xpcom can only be ran on the main thread. I guess it worked in bug 1176748, but some later changes in m-c broke it. Anyway, the problem for me right now is whether it's good to run the whole import module in the main thread. If not, I need to rewrite the code of importing mail from Seamonkey in c++. -_-

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/53b9cbabf6cb
Support importing addressbooks and settings from SeaMonkey separately. r=benc

Comment 43

[Magnus Melin [:mkmelin]]

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)

Comment 44

[Ping Chen [:rnons] (away)]

Thanks, I will take a look at mailstoreConverter.jsm, but this change doesn't only affect seamonkey importer, outlook/applemail importer will run in the main thread as well.

if it takes a long time someone will think the application froze and force shut it down

This seems the same to me by running it in a worker thread. I mean user cannot close the import dialog and can only wait. Instead, the progress indicator or status message update may be more helpful.

Comment 45

[Magnus Melin [:mkmelin]]

E.g. a progress indicator isn't possible when running in the main thread, because the UI is completely blocked until the function returns.

Comment 46

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail.patch

I tried to use worker in SeamonkeyImport.jsm, but the C++ code won't wait for the JS ImportMailbox function. As a result, the ImportDialog will advance to the next step with a blank screen, no success message.

With this patch, I tried to import 44 mailboxes, 12.4GB in total from Seamonkey, it took about 31 seconds to finish.

On the other hand, with the current trunk code, success message is accumulated in the sub-thread, only when all importing finished, it gets shown in the import dialog. https://searchfox.org/comm-central/source/mailnews/import/src/nsImportMail.cpp#655. So I think from the user's perspective, it's the same as running in the main thread.

From reading https://searchfox.org/comm-central/source/mailnews/import/public/nsIImportGeneric.idl#65-72, there is BeginImport and ContinueImport, I guess the original idea is to use BeginImport to kick start the importing, and use ContinueImport to get the current progress and do the next importing. However, our current code does everything inside BeginImport. So no matter using sub-thread or not, user only get the success/progress message after all import finished. I think if we want, we can improve this in another bug, since it will touch the code of other importers.

Comment 47

[Magnus Melin [:mkmelin]]

Looks fairly ok to me. I'll let Ben review

Comment 48

[Ben Campbell]

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).

Comment 49

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail.patch

Turns out readonly attribute Array<AString> sourceProfiles; is compiled to GetSourceProfiles(nsTArray<nsString >& aSourceProfiles) in C++. I have dropped nsISupportsString in both C++ and JS accordingly.

aResult.Clear();
aResult.AppendElements(mProfileNames);

Could be
aResult = mProfileNames.Clone();

I saw this Clear and AppendElements in one of your earlier patch (perhaps https://bugzilla.mozilla.org/attachment.cgi?id=9137950&action=diff), I thought it was the way to go. Glad to know Clone is enough, please take another look.

Comment 50

[Ping Chen [:rnons] (away)]

Attachment: 272292-import-mail.patch

Just noticed bug 1652371. So ChromeUtils.generateQI([Ci.nsIImportMail]) became ChromeUtils.generateQI(["nsIImportMail"]).

Comment 51

[Ben Campbell]

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

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

That looks good to me!

Comment 52

[Ben Campbell]

(In reply to Ping Chen (:rnons) from comment #49)

aResult.Clear();
aResult.AppendElements(mProfileNames);

Could be
aResult = mProfileNames.Clone();

I saw this Clear and AppendElements in one of your earlier patch (perhaps https://bugzilla.mozilla.org/attachment.cgi?id=9137950&action=diff), I thought it was the way to go. Glad to know Clone is enough, please take another look.

Yep, there's probably a few places where I could have used Clone() instead...
AppendElements() works also when copying from various other collection types I think... and you can pass in on-the-fly arrays, eg

array.AppendElements({thing1, thing2, thing3});

which is kind of cool.

For copying a whole nsTArray<> of the same type, there used to be an assignment operator, like so:

aResult = mProfileNames;

but that's been removed, so .Clone() is now the way to go.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ac6d55b907d3
Support importing mail from SeaMonkey. r=benc

Comment 54

[Magnus Melin [:mkmelin]]

Did this get a try run? Looks like it broken on windows: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310827968&repo=comm-central&lineNumber=56430
lld-link: error: undefined symbol: public: virtual enum nsresult __cdecl nsOutlookProfileMigrator::GetSourceProfileLocations(class nsTArray<class RefPtr<class nsIFile>> &)

Comment 55

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/2aa2b87fccd995228c457b7b37dd33df24738acb

Comment 56

[Ping Chen [:rnons] (away)]

Attachment: 272292-outlook-migrator.patch

Not tested, I pushed to Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1a2a766de9bcce7dd228ca324dc596fccf2082f8

Comment 57

[Ping Chen [:rnons] (away)]

This is the correct Try link: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1b7ba003c31d15974d30920f6333a048f1f07c09, not finished yet.
Let me know if you want me to merge the two patches.

Comment 58

[Magnus Melin [:mkmelin]]

Thanks, yes please combine the two patches for landing.

Comment 59

[Ping Chen [:rnons] (away)]

Attachment: [landed] 272292-import-mail.patch

Merge 272292-outlook-migrator.patch into 272292-import-mail.patch, to fix building on Windows.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6487f5a13d11
Support importing mail from SeaMonkey. r=benc

Comment 61

[Ping Chen [:rnons] (away)]

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

Comment 62

[Ping Chen [:rnons] (away)]

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

Comment 63

[Ping Chen [:rnons] (away)]

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

Comment 64

[Ping Chen [:rnons] (away)]

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.

Comment 65

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 66

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 67

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 68

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 69

[Geoff Lankow (:darktrojan)]

78.1.1:
https://hg.mozilla.org/releases/comm-esr78/rev/1c4404ab0eeb
https://hg.mozilla.org/releases/comm-esr78/rev/a937647a7088
https://hg.mozilla.org/releases/comm-esr78/rev/e415bc428af9
https://hg.mozilla.org/releases/comm-esr78/rev/1c14fb1fbd5f

Comment 70

[Flex]

I have a quite big Seamonkey database. Should I help testing if this works fine now? Is there any Windows build I could use for testing?

Comment 71

[Flex]

I did test this successfully with TB 78.3.1, however I had to close Seamonkey before, otherwise TB won't import the currently opened folder without leaving a failure message.

Comment 72

[alexa]

Import from SeaMonkey mixes up identities and accounts, see Bug 1696839