Closed Bug 472959 Opened 12 years ago Closed 9 years ago

GUI leaves invalid junk settings when "spamActionTargetFolder" is moved FROM a deleted account


(Thunderbird :: Account Manager, defect)

Not set


(Not tracked)

Thunderbird 13.0


(Reporter: rickstockton, Assigned: aceman)


(Blocks 3 open bugs)


(Keywords: dataloss, Whiteboard: [gs])


(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090109 Shiretoko/3.1b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090109 Shredder/3.0b2pre

Thunderbird (on a very old profile, migrated many times). There is no problem clicking buttons and pull-down menus within the "Account Settings" GUI, but many possible changes which I make to "Junk Settings" are not applied after clicking "OK".

Reproducible: Always

Steps to Reproduce:
1. Open "Account Settings" on a profile with many POP counts, but with all accounts using "Local Folders" for storage.

2. Choose "Local Folders"->"Junk Settings" in the Accounts Tree on the left.

3A. Toggle the checkbox for "enable adaptive junk mail controls for this account".

4. Press "OK".

5. Re-open the "Account Settings" panel, and again bring up "Junk Settings" for "Local Accounts".

6A. Note that your change was not made, the toggled value has gone back to what it was before step 3A, and the orignal setting is being used.

--These other settings also fail to be applied. Alternate steps:--

3B: Toggle selection of one or more "do not mark as junk if..." Address books. This leads to
6B: Old address book selections have been retained, and are still being used.

3C: Change "Move new junk messages to" Folder destination (different folder, and/or toggle use of default location). This leads to:

6C: old values are still present and used.
Actual Results:  
The GUI behaves _as if_ it is functioning correctly, but changes to these settings are not used, and they are not applied to the profile.

GUI fails to apply the same changes of "Junk Settings" within the "real" POP Account names, in exactly the same manner.

Old profile has been migrated from Windows to Linux, first created in 2003 (Mozilla 0.6) Many release-to-release migrations, as well as Mozilla-?Thunderbird. So, I think that it is very possible the Shredder 3.0 works OK with less "prehistoric" Profiles.

Didn't check-- even if this morphs into a "TB 3 doesn't handle data within old profiles right" bug, I think there MIGHT be huge exposure among current TB2 users. So I'm entering a Severity of "Major", at least for now. After you all isolate the problem, maybe it won't be.
let me restate, for clarity: This bug occurs with both "real" POP accounts AND with the special "Local Folders" account, in the exact same way.
I am not able to reproduce this on my current profile (Windows XP, custom trunk build about one week old.)
Kent, thanks for your verification regarding that guess (that the bug is only exposed when you're using a "prehistoric" profile). If this was affecting everybody, it would have been reported long ago.

BTW, my search for Dups in Bugzilla netted this other one, bug #457269. ("Junk Settings in account configuration Enable Checkbox has no effect", Unconfirmed.) I added a couple of questions within comment-5, which I hope will be helpful in isolating his bug. The relevance here is: Maybe his Mac-based profile contains the same sort of "cruft" which my own has in it, exposing a similar problem.

For myself, of course, it's easy to hack prefs.js within a text editor to make the changes I want. And so, although it will be "MAJOR" for less technical users who depend on the GUI, maybe the *smart choice* is to leave this alone, still "UNCONFIRMED", and spend all the precious Dev hours elsewhere:

*IF* it turns out to be widespread among 3.0 "Official" users, do the deep diving for 3.0.1. Please advise if you agree, I'll just save tarball snapshot of my current profile PERMANENTLY, for possible testing later.

BTW, I can't write code of any significant complexity, and have never done an actual patch... But if you can point me at the code which handles that 'OK' button, I'd be delighted to spend acouple of hours "rooting around" in there. (I'm incompetent, but tenacious).

Thanks for your ultra-fast response :)
The OK button is a return from a dialog, which is defined here:

If you would like to see a what a patch looks like on this section of code, you can see my patch at:

Any assistance you could give to track down the particular failure in your case would be appreciated.
Is there any inconsistency in Junk related settings in prefs.js?
>(Tools/Options/Advanced/General/Config Editor, "spam" in Filter: field)
>(Check inconsistency relates to next settings)
>  mail.server.serverX.moveTargetMode
>   (No moveTargetMode  : spamActionTargetAccount is used. Always Junk folder.)
>   (moveTargetMode = 1 : spamActionTargetFolder  is used. )
>  mail.server.serverX.spamActionTargetAccount = mailbox://xxx@x.x.x
>  mail.server.serverX.spamActionTargetFolder  = mailbox://nobody@Local%20Folders/Junk

> Meaning of mailbox://abc@x.y.z in above entry is;
>  Mail folder owned by account uses entries which has;
>       mail.server.serverY.hostname = x.y.z  
>   and mail.server.serverY.userName = abc
>   (If "Local Folders", abc is nobody, and x.y.z is Local%20Folders)    
> mail.account.accountP.server = serverZ points entries.
If inconsistency among above prefs.js entries exists, or if pointed serverY isn't defined, Tb possibly silently rejects to save Junk Settings, without warning.
See Bug 471344, for Copies&Folders case(Drafts,Templates,Sent).
WADA, I've not yet dived into the code which is called by from am-junk.xul, but I thank you VERY much showing me the relevant entries within prefs.js (as I thanked Kent previously, for showing me where the calling code resides.)

over time, I have have built my prefs.js "mail.identity.idY" collection all the way up to "id10" (with of course, corresponding "serverY" entries).  

"mail.accountmanager.accounts" == account2,account9,account10,account1,account3

With your assistance, I have found the exact "breakage". All of my accounts USED TO point to the Junk folder in "account1", now deleted (an old POP Account), and I removed it's folders after deleting it's settings.) But all the valid accounts say:

mail.server.server10.spamActionTargetAccount == mailbox://

mail.server.server10.spamActionTargetFolder ==

ISOLATED BUG: The GUI and subsequent pref-rewriting code did make the change to spamActionTargetFolder, but failed to match it up with any kind of valid "TargetAccount".
This leads to obvious follow-up questions:

(1) is this "TargetAccount" *NEEDED* to hit some listener on the head, forcing updated of index/message counts/unread and the folder tree?

(2) What purpose is ever served by setting a "spamActionTargetAccount" to something other than the "Local Folders" account when a "Local Folders" folder has been chosen?

(3) If a spamActionTargetAccount *is* needed, for some mysterious purpose, why do use a mailbox reference instead of an ACCOUNT reference? 
- - - - -

It's probably about a 10-liner to just "special case" overwrite of the "TargetAccount" value when a "Local Folders" destination folder has been chosen. But I think that a quality bugfix plan needs to know how this parameter is used. Kent, Wada I have no idea how to go about doing a network based "grep" of this parameter to see where it is used. Do either of you want to run with the ball from here, or can you advise me of the how to perform that search?

Meanwhile, I'll start digging around for some "newby patch submitter instructions" web pages.

BTW, is a public webserver, and I've got no problem with putting the genuine server name in here. I didn't catch on, unfortunately, that this particular account name reference is formatted exactly as a genuine email address, and would be formatted by bugzilla as a mailto: link. (No "renocomp" account exists there.)
Summary: changes to junk settings are not saved, or applied, after pressing "OK" from GUI. → GUI leaves invalid junk settings when "spamActionTargetFolder" is moved FROM a deleted account
(In reply to comment #7)
> But all the valid accounts say:
> mail.server.server10.spamActionTargetAccount == mailbox://
> mail.server.server10.spamActionTargetFolder == mailbox://nobody@Local%20Folders/Junk

As I wrote in my Comment #5, "which entry is used" depends on moveTargetMode. 
>  mail.server.server10.moveTargetMode
>   (No moveTargetMode  : spamActionTargetAccount is used. Always Junk folder.)
>   (moveTargetMode = 1 : spamActionTargetFolder  is used. )

If "No moveTargetMode", spamActionTargetAccount == mailbox:// is used.
It means that folder pointed by mailbox:// is used.
And, if account which uses entries is hidden account(use Global Inbox), folder pointed by mailbox:// can not be seen at folder pane.
This situation can occur, if user changed account's "Server Settings/Advanced" from "not use Global Inbox" to "use Global Inbox(Global Inbox owner=Local Folders) in the past.  

Tb 2 probably doesn't warn about above situation. 
Tb trunk possibly doesn't close each account's Junk setting panel when OK is clicked, in order to warn "pointed folder for Junk move can not be viewed", with no warning message. It's similar situation to Bug 471344. See Bug 471344 Comment #11.

Does mail.server.serverX.moveTargetMode exist? Is it set to 1?
WADA, thanks for further analysis and emphasis. 
The only "moveTargetMode" pref which exists within my prefs.js is this one:
<span class="quote">mail.server.default.moveTargetMode ; default; 0</span >

I do suffer the exact problem you showed in <a href="show_bug.cgi?id=471344#c11" title="NEW - Account Manager stops working after selecting one specific Copies &amp; Folders pane">Bug 471344
Comment #11</a>. These appear to be duplicate bugs-- and I did see that Ricardo also uses a "migrated-from-Seamonkey/Mozilla" profile.

But your summary above (when talking 471344) seems wrong in an important respect: The folder which I've selected in the GUI *is* a valid folder, which is viewable in the selection pull-down and SHOULD be selected when I say "OK".

The "cannot be viewed" problem is with the *old* one. It doesn't appear when I pop-up the "junk settings" panels and that's expected. But the invalidity of the old setting should not prevent the new choice from being set-- the warning should no longer be present.

The now-out-of-date test result, which led to that non-shown warning and refusal to apply the folder setting, SHOULD BE re-executed to test the newly-selected Folder when I press "OK"-- and the test should pass.

I'll SWAG 99% chance that these are duplicate bugs, and that bug should be marked as a dup of this one (because the more complete analysis has been done in comments here.)
Wada, since you already know where this test happens, and I'd be doing a lot of fishing to find it-- can you take it from here?
You wrote following in bug summary.
> when "spamActionTargetFolder" is moved FROM a deleted account

What do you mean by it? What opeartion? "Manual editing of prefs.js" before or after account deletion via UI?
What is the "deleted account"? Account which was changed from "account who never use Global Inbox" to "account who uses Global Inbox" via UI?
Hi Wada, I was pleading for you to do the code to "rewrite prefs with new, good values, even when OLD "mail.server.serverx.spamActionTargetAccount" refers to an account which was previously DELETED.

step 1 was: create a couple of new folders, within "Local Folders", to contain mail from "about to be deleted Account".

step 2 was: move all email into those new folders, within GUI, before deleting the account (Folders were emptied by via TB GUI).

step 3 was: Account deletion was via TB's UI. It's Folders no longer appear in the folder tree of the 3-panel display, or anywhere else, but some "bad" references were left behind-- specifically, "spamActionTargetAccount" values in other ACTIVE, NOT DELETED mail accounts.

step 4: probably irrelevant was: removed now-empty, now-invisible from TB GUI panels "dead folders" via OS-supplied tools (command line, or a GNOME or KDE "file manage0" app, I don't remember which).

step 5: Note that I have not done any editing of the "bad" directory references. Hand editing was to only to change hard-coded Windows Paths "C:\Documents and Settings\xxxxxxxxxx" into similarly hard-coded Linux paths "/home"/myhome/.thunderbird/xxxxxxxxxxxx".

I am leaving these particular intact so that I may continue to be a test of this bug. I can work around this problem by hand-editing prefs.js (easily), but the problem has and probably will be found by others too.

The "take it" is the actual fix coding-- right now, the GUI does allow me to select VALID folder and junk choices in the "specialFolderPickerGrid" of the GUI vbox. But after pressing "OK", the original bad values (which were shown as fairly thin line, maybe 20px tall, with nothing in it) somehow prevent prefs.js to be written correctly.
2nd from last paragraph above "I am leaving these particular intact" should be:

I am leaving these bad values (spamActionTargetAccount == DELETED Account, and spamActionTargetFolder == 'Trash' folder of DELETED Account) intact, and ...

With setting "mail.server.default.moveTargetMode ; default; 0", only spamActionTargetAccount is used (junk mail tries to be moved into "junk" folder of deleted account, its' not there). But we should write new values when good, new values are chosen in the GUI. that's the bug, and it is in code.
(In reply to comment #13)
> The "take it" is the actual fix coding

I absolutely agree with you on "some improvements are required".
But I'm not a developer. Sorry but I can't take it.
I can help external problem analysis only(my position is "Tester" like in software development). I can read Mozilla's code written in JavaScript, and I can distinguish JavaScript code and C++ code and C code used by Mozilla. However, I cant't write code for Mozilla, because I know nothing about Mozilla's internal design/spec/rule nor very important technology such as XPCOM, XUL, XBL, IDL, ...
To invite appropriate developers to this bug, we have to discover appropriate Product/Component and appropriate "Severity" for this bug.
I'm going to reverse myself on comment 2. I updated my trunk build, and I am now seeing the errors in the original description on my test profiles. That is, "Enable adaptive ... " does not get changed when set in the Junk Settings (nor other settings suchas the junk folder). I tested this in beta1, and it does work, so this is a regression.

Let me confirm it. Where are you at on this, Rick? Did you want to give a try at fixing it? I've done a couple of patches in this section of code in the last few months, so I could probably fix it again it necessary. If this is a hard error, I'll want it fixed before beta2. I'm still a little nervous about declaring it a hard error, as I did not see it earlier, but what I am seeing now is serious and makes the junk control essentially unusable.
Ever confirmed: true
Keywords: regression
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Bug 473781 reported phenomenon with Tb trunk that "Do not mark mail as junk if the sender is in:" setting was not retained upon re-open of the panel, even though setting panel was closed when "OK" is replied.
OK now I'm going to rereverse myself on comment 2 and comment 16. What I am seeing is much more severe than the reporter sees, and I have traced it to a regression from bug 435775 - which landed January 10 after the initial report in this bug. I am seeing bug 473781, and I will report any further work there.
Keywords: regression
Kent, I've got a handle on it, will code up a new am-junk.js in moments.
It already contains a function "onInit(aPageId, aServerId)"
spamActionTargetAccount which tries to accomplish some "clean up" of funky and
empty values in "spamActionTargetAccount" and "spamActionTargetFolder".

here's how it failed to be effective with my profile: The cleanup of
"spamActionTargetAccount" was successful, but at line 28 the
"spamActionTargetFolder" is taken from the current value of the "Local Filders"
Account-- and it's a bad one. Unfortunately, though, it is *not* null, and
passes doesn't fall into the set as /Junk within Local Folders" code block
(lines 30-33).

I see at least two ways to fix it in here: I could either check out whether the
"Local Folders" value is bad before using it; or, I can add fix-up code into
the "catch" block where the folder didn't exist.

I'm gonna try the 2nd way: put the try-catch block within a "while" based on a
counter preset as 2 times; modify the "catch" to include another copy of the
"reset as /junk" code (lines 31-32) and decrement the "while" counter to ASSURE
that we don't get stuck in there forever.

Sounds OK? If anyone's listening, please advise-- I'll assume that this is an
acceptable design, and start hacking it up. I'm clueless about this, but I have
expanded messenger.jar locally for hacking, will recreate the jar and use the
existing manifest when I'm ready to test-- there's no changes to the manifest,
I'm merely hacking a file which is already there. My entire profile tree is
already tar'ed up for multiple test tries. "Diff" the two versions of
am-junk.js afterwards. Is this more or less the right way?
(In reply to comment #18)
> .....regression from bug 435775.....

I don't see any getAttribute() calls in this routine, but it's stuffed to the gills with setAttribute() and removeAttribute() code.

Should I try updating those lines as a part of MY draft patch, or just leave it for <a href=">bug 473781</a> to handle?
(In reply to comment #20)
> Should I try updating those lines as a part of MY draft patch, or just leave it
> for <a href=">bug 473781</a>
> to handle?

If you can solve the issues in bug 473781, then I think you should go ahead and do so. BTW if you just put "bug 473781" (or for that matter "comment 20") in your comments, then bugzilla will magically add the links.

> I have expanded messenger.jar locally for hacking

I always compile with "ac_add_options --enable-chrome-format=flat" on debug builds so that I do not have to exapand the jars.
Thanks for hints, Kent. My comment 19 won't support switching between the "use junk on account X" and use other folder bullets. What I really need to do for "use junk on account X" is detect that it's referring to an invalid account here, inside onInit(). But because this mailbox ref more of a "folder" reference, rather than an "account" reference, Ive got a bad feeling that I must iterate through all Accounts, check that they're "valid", and only reset spamActionTargetAccount after finding that it DOESN'T match any valid account.

lots more lines of code, and quite a few statements to execute, but that seems to be what I must do. Is there an easier way?
I decided to stay with my way for now, because it's a one-liner "jar" command in this instance finished in about 1/4 second. My draft "fix" for am-junk.js definitely solves MY problem, I'll just do a little testing for unwanted side effects before I post the diff here. But it'll be a few hours, DW has ordered me to step away from the computer RIGHT NOW.

Plea for help: I'd like to push a javacript "info" message into the error console, whenever we've reset a server reference which failed the "match-to-valid-servers" test at the heart of the fix. (Reset as "null", it falls into the "if (!spamActionTargetAccount)" code block before being shown-- with "Local Folders" pre-selected. I don't know what this sort of thing looks like, and  whether I'd need to pile on a not-yet-watching listener for the event. Comment in the middle is where I'd like to have it done.

BTW, it looks like most of these "diff" patches are being posted via git's tool, rather than just plain old "diff". Is the regular GNU "diff" acceptable too? (Patch only touches the one file, am-junk.js).
My patches are generated from hg directly, using hg diff. If you are using message queues, there are also patches kept in the src/.hg/patches directory. Sometimes I use those as well. I think you will be better off to use the hg tools to generate your diffs.

I used to try to put messages into the error console, you do that with nsIConsoleService and related components. It's 2-3 lines in js.

But I don't bother anymore. dump() is available in js, and printf in c++, to send output to the system console. That's all that I do now for debug info.
Attachment #357927 - Attachment is obsolete: true
Attachment #357926 - Attachment is obsolete: true
Attachment #357926 - Attachment is patch: false
grrr. upon further testing, I've found that rev-A only works in the special case of a deleted account AND a deleted folder target. I'm working a a refined version which will handle the "deleted-account-only, bad folder still present" case too.
Blocks: junktracker
Attached file new am-junk.js (full file) (obsolete) —
I can confirm this bug in Thunderbird 3.0.14.

My Thunderbird has to manage several accounts. Some of them are using folders of another account.
e.g. account1 = and account2 =

For account2 I had configured the following (in Tb2):
Tools -> Account Settings -> Server Settings -> click Advanced -> check "Inbox for different account" and select account1

After a long time I wanted to delete account1 - now in Tb3. First I changed the above setting for account2 to "Inbox for this server's account". After moving away of existing mails from folders of account1 I deleted the account1.

Later I noticed that the junk settings for account2 are invalid. I reconfigured it but after opening the account settings my changes went away. It doesn't care which junk-setting I change of account2. They aren't persistent.

Searching in Bugzilla brought me to this bug report. The information that the spamActionTargetAccount is referencing to an invalid account was the clue. My spamActionTargetAccount pointed to the deleted account1. Manually changing this setting in the prefs.js to a valid value solved the problem with the user interface. 

I have no experience of bug fixing but was very interested in the am-junk.js from Rick. The testing of matching server entries is okay and identified my spamActionTargetAccount as not-matching. But then no new value was set. I made some changes to the code and now the GUI sets a new value if spamActionTargetAccount is referencing to a deleted account. I have attached the modified am-junk.js which solved MY problem.

As I said before I have no experience to fix bugs in Thunderbird. And I don’t know if the fix is universally valid. Generally I think that it's just a kind of workaround. In my opinion the bug is to be fixed earlier. If an account is being deleted, Thunderbird has to check if there is any junk-folder referencing to this deleted account. On the other hand invalid entries are already in the wild. So fixing the problem at this place is not irrelevant.
Bug 536768 reported phenomenon of "empty selected folder" and some others with hiden account/with official Tb 3.0 in addition to problems reported to this bug.
That bug is dup of this bug but keep open to hold phenomenon of "empty selected folder". Setting dependency of Bug 536768 to this bug.
Blocks: 536768
Jends, thanks for the patch. Are you willing to keep moving this forward?

If so, I have two comments that you could incorporate:

1) Please submit this as a diff-style patch.

2) I'm not sure I like the behavior that results from this fix. That is, somebody had a valid reason in the first place for sending junk messages to a different account. Now that account is invalid. When someone opens the junk dialog for an account with your fix, the junk folder is silently reset to point to the junk folder in that account - but the move still happens. Default behavior is to NOT move junk messages. It seems to me that if the old account is invalid, then this dialog should revert to default behavior. In other words, you should disable the move.
Jens, Kent, and WADA-- thank you all for "picking up the ball" and running with it. :))

First and foremost: thank you, Jens, for correcting the "re-set the bad value" code! per my attempt, I feel that we should always reset to the "Local Folders" account -- because TB requires this account to be present.

The cases which we need to test for are:

(1) Account Deleted via GUI, but folder still exists. (This is the resulting situation from the the TB GUI, and the situation which my code failed to catch. I think Jens' code gets this one.) 

(2) Account Deleted via GUI, and folder subsequently deleted by user. (External from TB; using Windows Explorer, "rm" command, some other file manager, whatever.) My original code and Jens' code version both get this one, I think -- although I haven't tried to actually do it. 

(3) Account still valid, but folder deleted by user (external from TB). The folder should probably be re-created new and empty, rather than resetting spamActionTargetAccount to point at "Local Folders".

Kent, I chose to "fix it" in the easiest possible way for my case, and Jens simply fixed up my approach. (As it happens, I personally want junk filtering on all accounts.) But if the correct design to is to turn junk filtering completely off and LEAVE IT OFF for that account, until the User turns it back on, that should be an easy mod. So there's two design choices to be made:

#1: reset to "Local Folders", or turn off filtering completely?
#2: Jens' idea to __also__ try and fix it "earlier", elsehwere, too.

I recommend against a redesign to address #2, and here's why: in many cases, people like me will have poked around in prefs.js using an external editor, or deleted the account's folders using external file managers/external tools. "Fixing" TB's account deletion code, hunting for other accounts which point at the deleted account folders, will not solve inconsistencies created in these ways.

But more important, I think it's not necessary. When junk filtering mis-behaves, the user *WILL* go the junk settings GUI-- just as Jens and I did here, and as RuCla did in 472959, and various other people have done without enough bugzilla expertise to find this bug.  I feel that the simplest fix is the best, we've pretty much finished it here, and users WILL end up on this panel and execute the fix code when they've got a problem.

But anyway-- if we do go after the account deletion code, that's completely separate place in TB. My suggestion would be to make a separate bug for it, and implement a completely separate fix for that bug. Jens, what do you think?

Kent, please make a call on those two design questions. If Jens' code is ready to test as "the" solution for this bug, then I'll do the 3 scenarios listed above. Can you think of any other test conditions I need to cover? If it passes the tests, then I'll do the diff-style patch for Jens and attach it here.
I'm busy at the moment, so only briefly:

I agree that the option to move junk mails should be disabled if an invalid entry for spamActionTargetAccount exists. I have improved the patch in this way. It should fix case (1) and (2) Rick listed above.

This patch fixes the bug in the GUI also in cases that someone have worked with an editor in prefs.js. I think, it is a good suggestion to fix this GUI-problem AND to file a separate bug which catches the invalid spam-entries after/while deleting an account.

The improved patch is attached. I hope I’ve created the diff-file correctly. 

(The component entry of this bug should be "account manager" and not "preferences". Can anyone change this?)
Attachment #436911 - Attachment is obsolete: true
Attached patch patch for am-junk.js (obsolete) — Splinter Review
Comment on attachment 439967 [details] [diff] [review]
patch for am-junk.js

(In reply to comment #35)
> Created an attachment (id=439967) [details]
> patch for am-junk.js

You need to request reviews if you want to get the patch in :-)
Attachment #439967 - Flags: superreview?(dmose)
Attachment #439967 - Flags: review?(bienvenu)
Comment on attachment 439967 [details] [diff] [review]
patch for am-junk.js

Thx for looking at this.

Can you use let instead of var, and use a consistent braces style? Also, need a space between if and ( for this line: if(!spamActionTargetAccountIsValid...)

Should we be clearing the action target pref completely in this case (or does updateMoveTargetMode do that)?

I have to disagree a bit with the premise that the user is going to naturally go to the junks settings UI, so I really think we need to fix this behind the scenes when we notice the issue, before the user goes to the UI, and such a fix would probably make this patch moot.

such a fix would probably be in nsSpamSettings.cpp, in the getter methods, which could do some verification of the server.

An alternate way of verifying the server might be to get the folder object from the target account uri using MailUtils.getFolderFromURI, and then asking the folder for its server. If that throws an exception, then the server is invalid.
Comment on attachment 439967 [details] [diff] [review]
patch for am-junk.js

minusing based on review commens and questions...
Attachment #439967 - Flags: review?(bienvenu) → review-
Comment on attachment 439967 [details] [diff] [review]
patch for am-junk.js

Cancelling sr request based on the already given r- which means this patch needs an update before it is ready. Please see the previous comments.
Attachment #439967 - Flags: superreview?(dmose)
Duplicate of this bug: 580648
someone up for doing a new patch based on comment 37?

raising severity because this and variations are datalossy. and adding [gs] because dependents have [gs]. and there are also many reports in mozillazine, recent ones being

rick, et al, can you clarify whether this is strictly a regression from v2? Or, did bad orofile/account data exist in v2, but v2 was able to save new settings and v3 can't?
Severity: major → critical
Keywords: dataloss
Whiteboard: [patchlove][has draft patch][needs new assignee][gs]
Comment on attachment 439967 [details] [diff] [review]
patch for am-junk.js

Obsoleted based on r-.
Attachment #439967 - Attachment is obsolete: true
In reply to Wayne Mery (

IIRC, I have created and destroyed account folders in the past- and previous to V3, I was able to fix-up my Folder Targets by using the GUI. I am unsure of my memory about this, and so I did not make a more "voluntary" answer for the comment at that time. But this is my impression:

"Junk" would be marked, but left in it's original Inbox when the "move to Trash on...." Folder was deleted. But the GUI could be used to choose a new Folder, and the setting "stuck" correctly.
I agree with David, the underlying issue causes problems already before entering Junk settings. Other settings can't be changed in other panes, when spamActionTargetFolder is bogus.
My suggestion: somewhere deep in the base, when junk classification is to be run, check if the folders are valid. If not, just disable junk classification on the account.
I think if junk classification stops working, the user will go to Junk settings.
Then, in am-junk.js, if folders are found invalid, automatically reset them to default folders and fix other prefs.
David, can you help me where to look? Or does comment 37 cover this?
Component: Preferences → Account Manager
QA Contact: preferences → account-manager
Blocks: 709581
Actually, has anybody noticed any problems arising from this bug, outside of non-working account manager ?

How does Junk mail classification behave when the target folder is invalid? Does it mark messages as junk but leaves them in the original folder? I couldn't find talk about this in the comments.
Assignee: nobody → acelists
Attached patch cleaned up patchSplinter Review
Patch reduced per comment 37, also added fix for the case when spamActionTargetFOLDER is broken. Also refined: only disable moveOnSpam when the broken account OR folder was actually used (selected as target).

I have not encountered any problem when the target is bad and marking messages manually as junk in which case they should have moved to the defined folder automatically. They just stayed in the current folder without any notice. I think in that case people could figure out they should check the Junk settings. Also I noticed the text in preferences "when I mark messages as Junk, move them to account's 'junk' folder" is misleading. It actually moves them to the folder defined in Junk settings (the spamActionTargetAccount/Folder).
That label should be fixed somehow (I can do that if there is any good proposal).

So I suggest fixing this the problem on a deeper level in bug 709581. Like when the spamSettings.initialize() fails a warning can throw the user back to the Junk settings pane and then this code here kicks in.
Attachment #590515 - Flags: ui-review?(bwinton)
Attachment #590515 - Flags: review?(dbienvenu)
Comment on attachment 590515 [details] [diff] [review]
cleaned up patch

Yeah, I think this sounds like reasonable behaviour.  ui-r=me.

Attachment #590515 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 590515 [details] [diff] [review]
cleaned up patch

thx for the patch - I also think it wouldn't hurt to put up a sliding alert or something when this happens, just to cut down on the surprise.
Attachment #590515 - Flags: review?(dbienvenu) → review+
What is a sliding alert?
(In reply to :aceman from comment #50)
> What is a sliding alert?

it's how we tell the user about errors on some platforms w/o blocking the ui or user (e.g., windows). It's an alert that opens in the bottom of the window, slides up, displays a message, and then slides back down a short time later.
Thanks. I can't currently do such a sliding alert.
But it is not used anywhere in account manager (as far as I can see). So it would probably need a separate bug and UI considerations for overhauling the account manager on which messages should only use this sliding type and which should show proper modal alert. (But I would be willing to work on that bug if anybody files it and points me to any usage of such sliding alert).
Keywords: checkin-needed
Whiteboard: [patchlove][has draft patch][needs new assignee][gs] → [gs]
Attachment #357923 - Attachment is obsolete: true
Blocks: 725615
Checked in:
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Blocks: 729147
You need to log in before you can comment on or make changes to this bug.