218439 - Account manager should disallow setting local directory of multiple accounts to the same physical directory (two accounts with same local directory causes mozilla to crash when checking IMAP mail or regenerate msf files)

Status: VERIFIED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Mirek Hankus]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030904
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030904

When checking mail list of all mail headers disappears and after about 1 second 
mozilla crashes with folowing backtrace

#0  0x417abadc in NSGetModule () from /usr/local/mozilla/components/libmsgdb.so
#1  0x417acc81 in NSGetModule () from /usr/local/mozilla/components/libmsgdb.so
#2  0x416e7a64 in NSGetModule () from /usr/local/mozilla/components/libmsgimap.so
#3  0x405d2a1f in XPTC_InvokeByIndex () from /usr/local/mozilla/libxpcom.so
#4  0x405bda8f in nsProxyObject::Post(unsigned, nsXPTMethodInfo*,
nsXPTCMiniVariant*, nsIInterfaceInfo*) () from /usr/local/mozilla/libxpcom.so
#5  0x405b5647 in PL_HandleEvent () from /usr/local/mozilla/libxpcom.so
#6  0x405b5574 in PL_ProcessPendingEvents () from /usr/local/mozilla/libxpcom.so
#7  0x405b690b in nsEventQueueImpl::ProcessPendingEvents() () from
/usr/local/mozilla/libxpcom.so
#8  0x40da8135 in parent_class_set_focus () from
/usr/local/mozilla/components/libwidget_gtk.so
#9  0x40da7ced in parent_class_set_focus () from
/usr/local/mozilla/components/libwidget_gtk.so
#10 0x4026a049 in g_io_unix_dispatch () from /usr/lib/libglib-1.2.so.0
#11 0x4026b806 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0
#12 0x4026be33 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
#13 0x4026bfec in g_main_run () from /usr/lib/libglib-1.2.so.0
#14 0x4018c6ab in gtk_main () from /usr/X11R6/lib/libgtk-1.2.so.0
#15 0x40da8536 in parent_class_set_focus () from
/usr/local/mozilla/components/libwidget_gtk.so
#16 0x40d853c4 in NSGetModule () from /usr/local/mozilla/components/libnsappshell.so
#17 0x0805b044 in getCountry(nsAString const&, nsAString&) ()
#18 0x0805b886 in main ()
#19 0x4038b3dd in __libc_start_main () from /lib/libc.so.6


I've got two different IMAP accounts on the same server (UW IMAP), so problem
may be related to having two accounts on one server

Reproducible: Always

Steps to Reproduce:
1.check mail 
2.
3.

Actual Results:  
mozilla crashed

Expected Results:  
mozilla should not crash

Comment 1

[David :Bienvenu]

can you attach or e-mail me your prefs.js so I can see your account setup? thx.

Comment 2

[David :Bienvenu]

I suspect this is because Mirek has two imap server accounts with the same local
mail directory. That doesn't and can't work well, and should be prevented in the
UI and/or backend.

Comment 3

[Mirek Hankus]

Changing local directory for one of the accounts fixed the problem, 

Now checking mail does not crash mozilla. 
Anyway mozilla should detect situation when two accounts are located 
on the same server and store info abount them in different directories

Comment 4

[David :Bienvenu]

yes, it should - and it should prevent the user from specifying the same local
directory for two different accounts, and create two different local directories
if there are two accounts with the same server name.

Comment 5

[David :Bienvenu]

changing summary

Comment 6

[WADA:World Anti-bad-Duping Agency]

When I try to choose directory of "Local Folders" for a POP3 account,
and when I try to choose directory of a POP3 account for "Local Folders" account,
Thunderbird version 1.6a1 (20051206) kindly issued following message.
> This directory is already used by another server.
> Please pick a different directry.
Then choosing already used directry was impossible.
I think this warning is issued by UI logic, and I guess this bug(crash) will still remain if user set duplicate directory in prefs.js by text editor or config-editor, but I think this kind of change(=manual corruption of prefs.js) is "User Error", then I belive this bug can be closed as WORKSFORME(or FIXED).

Comment 7

[Wayne Mery (:wsmwk)]

bienvenu, ludo, standard8, thoughts on comment 6?
note bug 379663.

Comment 8

[Wayne Mery (:wsmwk)]

would be nice to have a breakpad stack sig to add to the summary

Comment 9

[:aceman]

I can try to warn about this in the account manager. That would be a fix according to comment 6. I think if would also detect user editing of prefs.js but only when the user goes into the account manager for the first time after that change. That could be good enough.

WADA could you please help me where to find code of Thunderbird version 1.6a1 (20051206) so I can look how it detected this state?

David Bienvenu, are you working on this, or have you been just assigned automatically?

Comment 10

[:aceman]

For the record, this seems to be the recommended solution for somebody wanting messages from several accounts to land in one Inbox, without reusing the same Local Directory:

You can do this simply by deferring the storage for the second account to the 1st account (or defer all of them to the global inbox). See Account settings, Server settings, Advanced button for how to defer the storage of an account to an other account's inbox.

Comment 11

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #9)
> where to find code of Thunderbird version 1.6a1 (20051206) so I can look how it detected this state?

Binary? Source? If binary for testing, see next directory.
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2005/12/

Comment 12

[:aceman]

Source please so I can maybe reuse it.

Comment 14

[Magnus Melin [:mkmelin]]

http://mxr.mozilla.org/aviarybranch/source/mailnews/base/prefs/resources/content/amUtils.js#70

Comment 15

[:aceman]

Thanks Magnus. It seems the code is still there today, but does not alert the user, just silently does not change the directory. I'll look at it.

Comment 16

[:aceman]

Well, no wonder :)

Timestamp: 26.1.2012 12:21:44
Error: top.gPrefsBundle is undefined
Source File: chrome://messenger/content/amUtils.js
Line: 75

Comment 18

[:aceman]

Any idea why the title string for the filepicker in amUtils.js is passed in this way (in filepickertitle attribute)?
<button id="browseForLocalFolder" label="&browseFolder.label;" filepickertitle="&localFolderPicker.label;" ... >

Can I move them to normal prefs.properties?

Comment 19

[:aceman]

Attachment: fix stringbundle

This is a quick fix.

However I think there could be some reorganization of the strings done. Some strings could be moved from messenger.properties to prefs.properties and then maybe messenger.properties could be dropped from some of the files. Would that be useful as another bug? Could it save some memory?

Comment 20

[Ian Neal]

Comment on attachment 591885 [details] [diff] [review]
fix stringbundle

>+++ b/mailnews/base/prefs/content/am-server.xul
>+    <stringbundle id="bundle_prefs" src="chrome://messenger/locale/prefs.properties"/>

>+++ b/mailnews/base/prefs/content/am-serverwithnoidentities.xul
>+  <stringbundle id="bundle_prefs" src="chrome://messenger/locale/prefs.properties"/>

>+++ b/mailnews/base/prefs/content/amUtils.js
>-        var directoryAlreadyUsed =
>-          top.gPrefsBundle.getFormattedString(
>-            "directoryUsedByOtherAccount", [ currentServer.prettyName ]);
>+        var directoryAlreadyUsed = document.getElementById("bundle_prefs")
>+                                   .getFormattedString("directoryUsedByOtherAccount",
>+                                                       [currentServer.prettyName]);
Instead of adding the <stringbundle>s to the two xul files you could just use top.document.getElementById("bundle_prefs")

In fact, you could probably do that across all the am-*.xul and aw-*.xul files as all the top level xul files (AccoutManager.xul, AccountWizard.xul and msgAccountCentral.xul) already have the relevant <stringbundle> though it is worth testing first before creating a spin-off bug.

Comment 21

[:aceman]

Thanks I can try that. Can bundle_messenger be used in the same way?

Comment 22

[:aceman]

Attachment: use bundles from top documents

It seems you are right. I tried to change also smtpserveredit and smtpeditoverlay. It seems the bundles must be included in each <!DOCTYPE dialog>
but not in <!DOCTYPE page>. The page can reference the bundle via top.document.getElementId("bundle_name").

Would there be any gain when the bundles were removed from the <!DOCTYPE page> files?

Comment 23

[Ian Neal]

(In reply to :aceman from comment #21)
> Thanks I can try that. Can bundle_messenger be used in the same way?
Perhaps, in places, but it was probably less straight forward.

(In reply to :aceman from comment #22)
> Created attachment 592209 [details] [diff] [review]
> use bundles from top documents
> 
> It seems you are right. I tried to change also smtpserveredit and
> smtpeditoverlay. It seems the bundles must be included in each <!DOCTYPE
> dialog>
> but not in <!DOCTYPE page>. The page can reference the bundle via
> top.document.getElementId("bundle_name").
Pages are within a wizard, and the top is the wizard which has the bundle in.

> Would there be any gain when the bundles were removed from the <!DOCTYPE
> page> files?
Good question, I don't know the answer, perhaps Mark, Neil or someone else on the cc list does?

Comment 24

[Mark Banner (:standard8)]

(In reply to Ian Neal from comment #23)
> (In reply to :aceman from comment #21)
> > Would there be any gain when the bundles were removed from the <!DOCTYPE
> > page> files?
> Good question, I don't know the answer, perhaps Mark, Neil or someone else
> on the cc list does?

I'm not sure myself. I suspect there would be some minimal improvement at least, even if its just in code readability.

Comment 25

[:aceman]

I have made some tests (like putting all the 170KB of .properties files into prefs.js) but couldn't see any memory increase. So either it is insignificant or the strings are not loaded at once (but only as needed) or I couldn't read output of top properly.

I had in mind something like putting brand, messenger, prefs bundles only in the top accountManager.xul and let all the other subpanes access them via top.document. I don't know if it would be slower (top.document.getelement vs. document.getelement) but could save some memory.

I do not know now if it is worth pursuing.

Comment 26

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/61c6cbd61566

Comment 27

[Ian Neal]

(In reply to :aceman from comment #25)
> I had in mind something like putting brand, messenger, prefs bundles only in
> the top accountManager.xul and let all the other subpanes access them via
> top.document. I don't know if it would be slower (top.document.getelement
> vs. document.getelement) but could save some memory.
> 
> I do not know now if it is worth pursuing.

AccountManager.xul already has stringbundles for brand and prefs, so just a matter of adding messenger. Might be worth putting them all into a stringbundleset to make the life of add-on authors easier when it comes to overlaying. This of course is all food for a spin-off bug.

Comment 28

[:aceman]

Of course a new bug. I just wonder if it is worth to file it and do something like the proposed.
I do not know what stringbudleset is good for. So if any of you want to file the bug and propose some cleanup there that would surely bring some positive gain then I could play with it.