Closed Bug 272292 Opened 16 years ago Closed 2 months ago

unable to import Seamonkey data into Thunderbird

Categories

(MailNews Core :: Import, enhancement, P2)

enhancement

Tracking

(thunderbird_esr78 fixed, thunderbird78 affected)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird_esr78 --- fixed
thunderbird78 --- affected

People

(Reporter: bugzilla, Assigned: rnons)

References

(Depends on 1 open bug, Regressed 1 open bug, )

Details

Attachments

(4 files, 10 obsolete files)

28.81 KB, patch
rnons
: review+
Details | Diff | Splinter Review
10.85 KB, patch
rnons
: review+
Details | Diff | Splinter Review
20.97 KB, patch
benc
: review+
Details | Diff | Splinter Review
25.80 KB, patch
rnons
: review+
Details | Diff | Splinter Review
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.
I'm seeing this on Windows as well;  No listing for Mozilla/Netscape in the
Import dialog.
OS: Linux → All
See bug 187564 and bug 22689. And bug 63389 for the mails part. Duplicate?
also occurs on Mac: tested with 2004113002-0.9 on os x 10.3.6.
Hardware: PC → All
Not a TB auto-migration bug -> Core:MailNews:Import
Assignee: mscott → nobody
Component: Migration → MailNews: Import
Product: Thunderbird → Core
Version: unspecified → Trunk
Assignee: nobody → mscott
Flags: blocking1.8b?
*** Bug 279861 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b? → blocking1.8b-
Keywords: helpwanted
Is this a regression or have we always lacked this feature?
asa, this has always been a problem. see bugs referenced in comment 2 (this bug
tracks it for tbird).
*** Bug 250261 has been marked as a duplicate of this bug. ***
*** Bug 292932 has been marked as a duplicate of this bug. ***
This bug also can be reproduced in Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.8) Gecko/20060410 Firefox/2.0a1
note impact to litmus test https://bugzilla.mozilla.org/show_bug.cgi?id=272292
Assignee: mscott → nobody
Depends on: 63389
QA Contact: import
(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?
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
Product: Core → MailNews Core
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.
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.
Severity: normal → major
(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"
Duplicate of this bug: 565838
Keywords: helpwanted

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.

We should really fix this.

Priority: -- → P2
Summary: [import] unable to import Mozilla (Seamonkey) data into Thunderbird → unable to import Seamonkey data into Thunderbird
Duplicate of this bug: 1642124
Assignee: nobody → remotenonsense
Attached patch 272292-migration.patch (obsolete) — Splinter Review

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.

Attachment #9156877 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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.
Attachment #9156877 - Flags: review?(mkmelin+mozilla) → review-

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.

Attachment #9156877 - Attachment is obsolete: true
Attachment #9158508 - Flags: review?(mkmelin+mozilla)
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
Attachment #9158508 - Flags: review?(mkmelin+mozilla)

Fixed review comments.

Attachment #9158508 - Attachment is obsolete: true
Attachment #9159120 - Flags: review?(benc)
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) {`?
Attachment #9159120 - Flags: review?(benc) → review+

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.

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

Attachment #9159120 - Attachment is obsolete: true
Attachment #9159610 - Flags: review+

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.

Attachment #9159610 - Attachment is obsolete: true
Attachment #9159614 - Flags: review+
Target Milestone: --- → Thunderbird 79.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/4422b4525c2f
Support importing accounts and mails from SeaMonkey. r=benc

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

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.

Attached patch 272292-import-addrbook.patch (obsolete) — Splinter Review

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

Will work on importing things separately next.

Attachment #9160554 - Flags: review?(benc)
Attachment #9159614 - Attachment filename: 272292-import-mail-accounts.patch → [landed] 272292-import-mail-accounts.patch
Attachment #9159614 - Attachment description: 272292-import-mail-accounts.patch → [landed] 272292-import-mail-accounts.patch
Attachment #9159614 - Attachment filename: [landed] 272292-import-mail-accounts.patch → 272292-import-mail-accounts.patch
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
Attachment #9160554 - Flags: review?(benc) → review+

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.

Attachment #9160554 - Attachment is obsolete: true
Attachment #9160850 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a857186a1054
Support importing addressbooks from SeaMonkey. r=benc

Attachment #9160850 - Attachment description: 272292-import-addrbook.patch → [landed] 272292-import-addrbook.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.

Attachment #9161527 - Flags: review?(benc)
Attached patch 272292-import-mail.patch (obsolete) — Splinter Review

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.

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

  2. 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 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...
Attachment #9161527 - Flags: review?(benc) → review+

(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 :-)

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

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/53b9cbabf6cb
Support importing addressbooks and settings from SeaMonkey separately. r=benc

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)

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.

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

Attachment #9161527 - Attachment description: 272292-import-separately.patch → [landed] 272292-import-separately.patch
Attached patch 272292-import-mail.patch (obsolete) — Splinter Review

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.

Attachment #9162726 - Attachment is obsolete: true
Attachment #9164662 - Flags: review?(mkmelin+mozilla)
Attachment #9164662 - Flags: review?(benc)

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

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).
Attachment #9164662 - Flags: review?(mkmelin+mozilla)
Attachment #9164662 - Flags: review?(benc)
Attachment #9164662 - Flags: review?
Attached patch 272292-import-mail.patch (obsolete) — Splinter Review

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.

Attachment #9164662 - Attachment is obsolete: true
Attachment #9164662 - Flags: review?
Attachment #9165278 - Flags: review?(benc)
Attached patch 272292-import-mail.patch (obsolete) — Splinter Review

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

Attachment #9165278 - Attachment is obsolete: true
Attachment #9165278 - Flags: review?(benc)
Attachment #9165299 - Flags: review?(benc)
Comment on attachment 9165299 [details] [diff] [review]
272292-import-mail.patch

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

That looks good to me!
Attachment #9165299 - Flags: review?(benc) → review+

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ac6d55b907d3
Support importing mail from SeaMonkey. r=benc

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

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

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.

Thanks, yes please combine the two patches for landing.

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

Attachment #9165299 - Attachment is obsolete: true
Attachment #9165655 - Attachment is obsolete: true
Attachment #9165655 - Flags: review?(mkmelin+mozilla)
Attachment #9165799 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6487f5a13d11
Support importing mail from SeaMonkey. r=benc

Type: defect → enhancement
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
Attachment #9159614 - Flags: approval-comm-esr78?
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
Attachment #9160850 - Flags: approval-comm-esr78?
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
Attachment #9161527 - Flags: approval-comm-esr78?
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.
Attachment #9165799 - Attachment description: 272292-import-mail.patch → [landed] 272292-import-mail.patch
Attachment #9165799 - Flags: approval-comm-esr78?
Comment on attachment 9165799 [details] [diff] [review]
[landed] 272292-import-mail.patch

[Triage Comment]
Approved for esr78
Attachment #9165799 - Flags: approval-comm-esr78? → approval-comm-esr78+
Comment on attachment 9159614 [details] [diff] [review]
[landed] 272292-import-mail-accounts.patch

[Triage Comment]
Approved for esr78
Attachment #9159614 - Flags: approval-comm-esr78? → approval-comm-esr78+
Comment on attachment 9160850 [details] [diff] [review]
[landed] 272292-import-addrbook.patch

[Triage Comment]
Approved for esr78
Attachment #9160850 - Flags: approval-comm-esr78? → approval-comm-esr78+
Comment on attachment 9161527 [details] [diff] [review]
[landed] 272292-import-separately.patch

[Triage Comment]
Approved for esr78
Attachment #9161527 - Flags: approval-comm-esr78? → approval-comm-esr78+
Regressions: 1663363

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?

You need to log in before you can comment on or make changes to this bug.