709581 - invalid folder in spamActionTargetAccount pref causes problems when changing account settings

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[ts-d]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

change the settings in an account


Actual results:

clicking on OK - nothing happens


Expected results:

SOLUTION (not "what should..."):

One line in prefs.js which sets the account properties has to be (p.e.):

user_pref("mail.server.server4.spamActionTargetAccount", "mailbox://nobody@Local%20Folders");

In my prefs.js the line contained an old account-name instead of Local%Folders, such as [e-mail]

[also thunderbird is not able to manage it, if an user renames the 'local orders', p.e. in 'local' - I had to correct it in the prefs.js, because thunderbird got a problem by updating the prefs.js for a new thunderbird release]

That error I know since I work with Thunderbird. It seems to be a very old and ignored bug (Version3, Version 5, Version 8).

Perhaps the cause is an write-error. But then I think, it would be possible, to program this in another way so that this error doesn't occur again and again.

Thank you very much for your attention!

Bye, Thomas.

Comment 1

[:aceman]

I do not really understand what the problem is. Can you rephrase it somehow?

Comment 2

[ts-d]

o. k.

I choose menue extras - account settings

If I don't change a property, I can click on the ok button and the dialog-window will disappear.

But if I change a property, I can click on the ok button and nothing happens. I have to click on cancel, to get out of the dialog surface.

The reason for this bad behaviour I painted in my last message:

There's a line in the prefs.js, which thunderbird cannot understand, because there is an mistake in the line. The mistake didn't cause the user but thunderbird itself!

Comment 3

[:aceman]

How did you find out that line is the problem?

Also, after you click OK and nothing happens, then on Cancel, please go to Tools->Error console and see if there are any messages. Paste them here.

Comment 4

[ts-d]

no message in the error console

Comment 5

[:aceman]

OK, I manually edited prefs.js and made that preference point to nonexistent account. When I open Junk settings, the fields are empty. There are no addressbooks to choose, "junk folder on" and "other" are empty. But I get an error in Error console saying "Error: uncaught exception: unable to find folder to select!". I can't see the nonworking OK button. What is your Thunderbird version?
Do you know how Thunderbird got into this state? Have you deleted some account where the junk setting pointed to?

Comment 6

[ts-d]

Version 8
I don't know, but this problem I had got with different Thunderbird Versions (see obove).

Yes, I deleted this account - perhaps this is the cause of the error.

But if the setup of thunderbird would not react so very sensitive and silly only at a single line in the prefs.js, the programer could change the reaction in an other way, perhaps to check the line and to let change the value by the user and restart correctly, or not?

Comment 7

[:aceman]

I can confirm the behavior you describe on TB8.

The message I see only happens when I visit Junk settings pane.

If TB can detect the folder is nonexistent (as the error message tells), it should be able to just reset it to current account/junk folder. Better than nothing.

Comment 8

[WADA:World Anti-bad-Duping Agency]

Same problem as bug 536768? See also bug 472959.

Comment 9

[:aceman]

Well, the cause may be the same, but this report noticed, that also other panes can't be edited (stored) if the junk settings are broken. We'll see if fixes for those bugs will fix this one too.
The problem is, the fixes must work even without entering Junk settings. Something like we discuss in bug 472959.

Comment 10

[ts-d]

yes, thank you very much, bye.

Comment 11

[:aceman]

I have determined the problem happens in AccountManager.js. When clicking OK, the function saveAccount(accountValues, account) is called and the last lines are these:
if (server && server.spamSettings)
  server.spamSettings.initialize(server);

This code does not return properly (or throws, but nothing is in the Error console) when the spamactiontarget is invalid.

Comment 12

[:aceman]

Looks like the server.spamSettings.initialize C++ code works fine, detects the failure and returns error. But I can't see yet if it is propagated to the JS so that it can be catched.

WARNING: can't determine folder's server type: file mailnews/base/util/nsMsgDBFolder.cpp, line 3213
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file mailnews/base/src/nsSpamSettings.cpp, line 568
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file mailnews/base/src/nsSpamSettings.cpp, line 462

Comment 13

[:aceman]

If somebody helps me how I can get the proper error value from this function (out of the C++ implementation) then I can catch it and notify/move the user to fix his Junk settings. More description to this in Bug 472959 comment 47.

Comment 14

[Mark Banner (:standard8)]

The error should be propagated and you should just be able to wrap the initialize in a try/catch. I suspect the error console doesn't show it probably because there's some listener or something that is effectively hiding the error.

Comment 15

[:aceman]

Thanks, I can try it. I put an alert before server.spamSettings.initialize and after it, and the "After" one didn't happen. So I concluded the server.spamSettings.initialize either didn't return or somehow escaped from the saveaccount function prematurely (like exception). But there was nothing in the console. Yes, maybe the whole saveaccount function is in some try () block, but I can't see it. I will check it.

Comment 16

[:aceman]

You are right, the exception can be caught.

Bwinton, now I need UI advice. I can detect there is something wrong with the target folders in Junk settings (or something else). Should I now:
1. show some warning and throw the user into the Junk settings panel. This would allow sanitizing code in bug 472959 to run and fix the targets. CON: If the code or user can't fix the settings (i.e. the exception is caused by something else, outside of user reach) there would be no way to press OK and save settings. Only exit from the account manager would be via Cancel.
2. show some warning and let the user decide (Yes/No) whether to continue exiting the dialog (with broken target folders) or go review the Junk settings.
3. run sanitizing code from bug 472959 silently (I hope I can reach it as it is in another file). Whether it helps in any way or not, save the rest of the settings and close dialog. CON: the user would not see the target folders have changed and his Junk could go somewhere unexpected.

What do you think?

Comment 17

[Blake Winton (:bwinton) (:☕️)]

I don't like #3, because we shouldn't be changing the target folders without notifying the use.

#1 also doesn't seem right, because it's unlikely the user was intending to go to the junk settings page, and also because fixing the problem may not be the user's top priority.

That leaves us with #2, which seems reasonable, if more invasive than I'ld like.  But still, of those three options, #2 is the best.

Thanks,
Blake.

Comment 18

[:aceman]

Is there any other solution?

Comment 19

[Blake Winton (:bwinton) (:☕️)]

I can't think of one, off the top of my head.  If you think of any, please propose them.  :)

Comment 20

[:aceman]

Attachment: patch

So here it is. Ask the user and if he agrees throw him straight into the Junk settings pane. The sanitizing code there runs and fixes the targets silently. He sees the fixed values now. Even if he doesn't change any field and just clicks OK, it saves fine now (tested as bug 472959 has already landed).
Bug 536768 is not yet fixed, so some breakage will not be fixed in this way. If that is the case and he would be in a loop, he can click Cancel, or No to this new question. The Junk settings will stay broken but other changes should be saved.

The question string probably needs refinement.

Comment 21

[:aceman]

There is already one nit, when jumping to the Junk settings, it is not selected in the tree on the left. I'll try to fix that.

Comment 22

[:aceman]

Attachment: patch v2

So this one is better.

Comment 23

[Ian Neal]

Comment on attachment 600791 [details] [diff] [review]
patch v2

Even for accounts that don't have a "Junk Settings" (e.g. news accounts) you get presented with that accounts "Junk Settings" pane.
Also there is no indication to the user as to what the issue is (e.g. when the targetaccount doesn't exist), does the "e" variable give that in some way?
f- for the moment.

Comment 24

[:aceman]

(In reply to Ian Neal from comment #23)
> Comment on attachment 600791 [details] [diff] [review]
> patch v2
> 
> Even for accounts that don't have a "Junk Settings" (e.g. news accounts) you
> get presented with that accounts "Junk Settings" pane.

I don't understand this. This is dependent on "if (server && server.spamSettings)" so should not happen on accounts that don't have Junk Settings. What did you do exactly?

> Also there is no indication to the user as to what the issue is (e.g. when
> the targetaccount doesn't exist), does the "e" variable give that in some way?
I didn't want to analyze the error at this point. Also the exception seems not much helpful (0x80004005 from comment 12). So I just show a generic message. The code in am-junk.js then makes the proper analysis and fixes the values. It may also not be able to fix it.
But it could show a sliding alert (bug 725615) to say which field exactly it fixed. Would that be enough?

Comment 25

[Ian Neal]

(In reply to :aceman from comment #24)
> (In reply to Ian Neal from comment #23)
> > Comment on attachment 600791 [details] [diff] [review]
> > patch v2
> > 
> > Even for accounts that don't have a "Junk Settings" (e.g. news accounts) you
> > get presented with that accounts "Junk Settings" pane.
> 
> I don't understand this. This is dependent on "if (server &&
> server.spamSettings)" so should not happen on accounts that don't have Junk
> Settings. What did you do exactly?
Sorry, probably a bit of an edge case where about:config or prefs.js has been edited to change the setting for a mailnews account.
> 
> > Also there is no indication to the user as to what the issue is (e.g. when
> > the targetaccount doesn't exist), does the "e" variable give that in some way?
> I didn't want to analyze the error at this point. Also the exception seems
> not much helpful (0x80004005 from comment 12). So I just show a generic
> message. The code in am-junk.js then makes the proper analysis and fixes the
> values. It may also not be able to fix it.
> But it could show a sliding alert (bug 725615) to say which field exactly it
> fixed. Would that be enough?
Yes, outside the scope of this bug.

Comment 26

[:aceman]

So do I have to do anything?

Comment 27

[Ian Neal]

(In reply to :aceman from comment #26)
> So do I have to do anything?

No, that is fine.

Comment 28

[Ian Neal]

Comment on attachment 600791 [details] [diff] [review]
patch v2

>+++ b/mail/locales/en-US/chrome/messenger/prefs.properties
>+junkSettingsBroken=The Junk settings on account "%1$S" have a possible problem. Would you like to review them before saving Account settings?
Should this be "Account Settings?" rather than "Account settings?"

>+++ b/mailnews/base/prefs/content/AccountManager.js

>+  let changeView = (pageId && account);
Not sure you actually need the ().

>+    pageId = node.getAttribute('PageTag')
Is the style for ' or " in this file? Probably mixed, if so I prefer "

>+++ b/mailnews/base/prefs/content/AccountManager.xul

>+      <tree flex="1" onselect="onAccountTreeSelect(null, null)" id="accounttree"
Probably don't need to make this change as null is assumed when nothing is specified but if you do keep it put ; after )

>+++ b/suite/locales/en-US/chrome/mailnews/pref/prefs.properties
>+junkSettingsBroken=The Junk settings on account "%1$S" have a possible problem. Would you like to review them before saving Account settings?
As for TB version.

Comment 29

[:aceman]

Attachment: patch v3

Thanks, done. Let's get ui-review now and then I'll give it back to yoy for review.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 606865 [details] [diff] [review]
patch v3

I think I like this.  I would prefer it if the message was more explicit about what's broken, though.  ui-r=me with that change.

Also, tests would _really_ help me see how the UI works in practice (without me having to muck with my profile… :P )

Thanks,
Blake.

Comment 32

[:aceman]

Have you seen comment 24? We don't know what happens, the .cpp backend does not tell us much. How can we be more explicit? What do you suggest?
And I can't do tests yet :(

Comment 34

[Ian Neal]

Comment on attachment 606865 [details] [diff] [review]
patch v3

r=me

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 606865 [details] [diff] [review]
patch v3

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

Generally looks ok, just need that localisation note.

::: mail/locales/en-US/chrome/messenger/prefs.properties
@@ +4,5 @@
>  accountExists=A mail or newsgroup account with the same user name and server name already exists. Click Back and enter a different server name, or click Cancel.
>  modifiedAccountExists=An account with that user name and server name already exists. Please enter a different user name and/or server name.
>  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
>  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> +junkSettingsBroken=The Junk settings on account "%1$S" have a possible problem. Would you like to review them before saving Account Settings?

This needs a localisation note for the %1$S

Comment 36

[:aceman]

Attachment: patch v4

Good catch, fixed and some bitrot too.

Comment 37

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/comm-central/rev/e46f87c271f1