Closed Bug 577775 Opened 14 years ago Closed 12 years ago

Invalid account directory path to folders should raise an error dialog/verify setting, not just log in error console

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: cigydd, Assigned: aceman)

References

(Blocks 1 open bug, )

Details

(Keywords: ux-error-prevention, Whiteboard: [datalossy])

Attachments

(1 file, 5 obsolete files)

22.24 KB, patch
aceman
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.99 Safari/533.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.9.1.10) Gecko/20100512 Lightning/1.0b1 Thunderbird/3.0.5

If the path seen in Account Settings > [Account] > Server Settings > Local Folder is invalid it generates only a silent error message in the Error Console which normal user doesn't notice.
Having wrong or non-existent path set causes the Account Settings dialog window to behave strangely:
- Pressing OK doesn't close the dialog.
- When switching once to the Copies & Folders page you can't then switch to any other page more.
- As a logical consequence, you can't select the account's folders as a target location of junk mail or in Copies & Folders. Those fields appear blank.

It takes a huge investigation for the user where the error is (see my discussion at the forum: http://forums.mozillazine.org/viewtopic.php?f=31&t=1941299).
So it would be nice if Thunderbird said something as "Sorry man but your account isn't there where it claims to be placed on the disk."

Reproducible: Always

Steps to Reproduce:
Set a wrong or non-existent path in the Account Settings dialog under [Account] > Server Settings > Local Folder.
If it weren't possible you should know that I copied the whole Thunderbird directory from another computer where it was stored on another hard drive partition (D) than on the system one (C). On the target computer there is only one hard drive partition (C) and the D paths became wrong. So if you cannot change the path to a wrong one try to follow similar steps.
Actual Results:  
No user-level warning appears; only an error message in the Error Console.

Discovering folders for account failed with exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIMsgFolder.subFolders]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://messenger/content/folderPane.js :: ftv__mg_all :: line 1208" data: no]

And a second error message:

Chyba: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIMsgFolder.subFolders]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Thunderbird/modules/MailUtils.js :: MailUtils_discoverFolders :: line 74" data: no]

- "Chyba" means "error" in Czech.

And then the Account Settings dialog window behaves oddly, as described in Details.

Expected Results:  
There should be a clear warning (e. g. a message box) that would ask the user to correct the path to the account.

I repeat the link to the forum discussion where this bug has grown:
http://forums.mozillazine.org/viewtopic.php?f=31&t=1941299
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Keywords: dataloss
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dataloss
Summary: Invalid account path should raise an error message box → Invalid account directory path to folders should raise an error dialog/verify setting, not just log in error console
Whiteboard: [datalossy]
If the error is thrown while we are still in the account manager (when pressing OK), then I could look at this.

However, this assumes that when a user makes a migration as you have done, and starts up TB and his email/folders are not there he will naturally go to the account manager to check the settings. That is not 100% sure, but better than nothing.
Assignee: nobody → acelists
Thank you, aceman. It's always preferable the software to be idiot-proof (I'm speaking of myself) :-) So if you want to dive into it, you're welcome. I'm not a Thunderbird developer and I think it isn't going to change soon so it's up to you. Greetings to Slovakia from Czechia.
Thanks :)

I'll check it out after bug 224831 settles, so that the changes do not interfere.

There are similar bugs where clicking OK in account manager does nothing due to invalid folder targets: bug 709581, bug 536768, bug 557030, bug 472959. You can watch them if you are interested.
Also see bug 218439.
Depends on: 224831
Depends on: 218439
It looks like I could work on this soon, the blocking patches are landing.
Depends on: 709581
I can't seem to reproduce this with a recent Thundebird (15). 
I have edited the prefs.js file, where I had an account. I deleted the pref "mail.server.server1.directory-rel" and edited "mail.server.server1.directory" so that it points to a non-existent path. Starting TB, the folder pane on the left contained the account, but only the line with its name, not folders. But no errors in the Error console. Of course, some operations (like Read messages or Manage filters on the Account central) produced errors. But no new errors when entering account manager. I could also OK it just fine.

Pavel, can you reproduce it somehow in recent versions?

Even in case of no errors I will proceed according to comment 1. The user can see that the account is somehow broken (no folders and no expander grippy). So let's hope he goes into the Account settings. I'll try to check the Local direcotry path when the user exits the dialog and can warn the user the path is invalid.

But note that if the path is invalid in the sense that the folder does not exist, but is creatable, then TB will silently create it and create some initial folders. This case can't be catched (we can't know that previously there was other data (other mount point) in the path). The solution will probably only catch paths that are really invalid and can't be created (missing disk drive, read-only path, etc.)

Also of interest is bug 750781, where we could reject choosing some really bad folders as the Local Directory.
I'll try to build some common infrastructure to check various usability properties of the selected path.
Blocks: 750781
Version: unspecified → Trunk
I will look into this soon. Maybe in one or two weeks.
I have to simulate the situation with the wrong path somehow.
No longer blocks: 750781
No longer depends on: 218439, 224831, 709581
Bugzilla is broken today...
Blocks: 750781
Depends on: 218439, 224831, 709581
David Bienvenu, in what cases does TB create the Local Directory? Does it create whole path or only if the last component is missing?
In my test it created even two levels of directories (like profile/Mail/dir). Is there any limit on that? Maybe the first component (drive letter, or / in linux) must exist.

Maybe if TB is already up and user goes in the Account manager and the directory is still not existing then we have a problem and can report it.

I also tested having the Local directory be read only. That displayed the folder pane, but no folder could be shown (account central was always shown and not the list of message in the folder). Produced tons of errors in the Error console. So that is also a thing to reject.
OS: Windows XP → All
Hardware: x86 → All
The main rule I would have is that the local directory can not be a parent of the profile directory, or any other account's local directory.
could you not say, to be more bulletproof, that it if it is anywhere in the "thunderbird heirarchy", that it can't be anywhere other than immediately under "the profile" in question? 

That would exclude most any possible stupid idea, such numbskull things such as in the the directory of some other profile
(In reply to Wayne Mery (:wsmwk) from comment #10)
> could you not say, to be more bulletproof, that it if it is anywhere in the
> "thunderbird heirarchy", that it can't be anywhere other than immediately
> under "the profile" in question? 
yes, that makes sense. It's similar to saying that it can't be above any other local directory, which also implies that any other local directory can't be under it :-)
It can be elsewhere than immediately under the profile, e.g. profile/Mail/.../pop.server .
(In reply to :aceman from comment #12)
> It can be elsewhere than immediately under the profile, e.g.
> profile/Mail/.../pop.server .

I believe the suggestion was that if the directory is under the profile, then it can only be in the expected place - under Mail for pop3/local servers, and ImapMail for imap accounts, News for news accounts.
Your suggestion is more strict, it even says in which subdirectories of profile it must be. Theoretically any subdirectory is OK, but TB can start to use some new subdirectory any time for internal purposes and then there would be a collision. This sounds reasonable to try to prevent it. Can I get the predefined directory names in any way? Is there some property containing those "Mail", "ImapMail", "News" (RSS seems to be under Mail) ?
(In reply to :aceman from comment #14)
 Can I get
> the predefined directory names in any way? Is there some property containing
> those "Mail", "ImapMail", "News" (RSS seems to be under Mail) ?

Those are hard-coded, so you have them already.
Note to myself: the proposals since comment 8 are actually for bug 750781 (existing dirs deemed problematic).

David can you now answer comment 8, which is for this bug?
(In reply to :aceman from comment #8)
> David Bienvenu, in what cases does TB create the Local Directory?

It creates it lazily whenever the code tries to do something with the account.

> Does it
> create whole path or only if the last component is missing?
I think it creates the <server> part, and perhaps the Mail part.

But empirical tests are better.
I mean if mail.server.server1.directory = "C:/TB/profile/Mail/pop.server" but nothing except C: exists, will TB create the whole "TB/profile/Mail/pop.server" path?
(In reply to :aceman from comment #18)
> I mean if mail.server.server1.directory = "C:/TB/profile/Mail/pop.server"
> but nothing except C: exists, will TB create the whole
> "TB/profile/Mail/pop.server" path?

I understood what you meant - I think it creates the server part, and perhaps the Mail part, but if you try it, and it creates the whole path, then I'll defer to the empirical evidence.
With Thunderbird 12.0.1 (build 20120430050211) on Linux Mint 11 Katya, everything works like :aceman first wrote in comment 5:

– No messages in the JavaScript console.
– No error messages on the command line.
– I tried to delete the setting mail.server.server1.directory-rel in the prefs.js file and to change the setting mail.server.server1.directory to "/test" (non-existent and non-creatable directory) and still no error messages.
– Pressing OK in the account manager – having the wrong path set – doesn't raise any user-noticeable errors. It maybe should.
Could you see if TB still left the bad "/test" value in the Local Directory field?

In my tests on Linux giving unusable directory caused TB to just create a new empty one under the profile/Mail.
Yes, TB kept the bad "/test" value in the Local Directory field.

I didn't notice if TB created something in the [profile]/Mail folder :-(
Maybe I'll look into that but no promises.
Attached patch patch (obsolete) — Splinter Review
First stab at the checking infrastructure.
The reporters problem should be covered by the test for existence of the directory. If TB could not create it at start, entering account manager should detect the dir is not existent and show a warning. The AM will stay on the server pane until the problem is fixed (other directory selected). The user can still click Cancel to exit AM without any change.

Now when I look at it, not all server types have the Local Directory on the am-server.xul pane... I'll need to fix that.
Attachment #638218 - Flags: ui-review?(bwinton)
Attachment #638218 - Flags: feedback?(dbienvenu)
Attachment #638218 - Flags: feedback?(iann_bugzilla)
To see the full featured checking, apply also the patch from bug 750781.
Comment on attachment 638218 [details] [diff] [review]
patch

When I tried to choose my home directory the message that popped up was:
http://dl.dropbox.com/u/2301433/Screenshots/BlogsNews.png
which was slightly surprising, and doesn't seem quite right.  Could we fix the error message there to be more accurate?

Choosing a non-readable directory resulted in:
http://dl.dropbox.com/u/2301433/Screenshots/InvalidDir.png
which is better, but still doesn't let me know what the next step is.  I would prefer it if the message was:
"The directory specified in Local Directory setting is invalid.  Please choose another."

And ui-r=me with those changes.

Thanks,
Blake.
Comment on attachment 638218 [details] [diff] [review]
patch

(One of these days, I'll remember to set the flag to the value I said I would set it to…)
Attachment #638218 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 638218 [details] [diff] [review]
patch

looks reasonable - nit - don't want commented out code.
+    pageId = currentPageId;
+//    return true;
Attachment #638218 - Flags: feedback?(mozilla) → feedback+
Bwinton, the NewsBlog message is interesting. Can you see what the local directory there is? I assume you wanted to use the home directory on one of your mail accounts?

I will test other than mail accounts better there is already one known bug.
Comment on attachment 638218 [details] [diff] [review]
patch

>+function checkDirectoryIsUsable(aLocalPath) {
>+    if (currentServer.localPath.equals(aLocalPath) ||
>+        currentServer.localPath.contains(aLocalPath, true) ||
>+        aLocalPath.contains(currentServer.localPath, true)) {
You will need different alerts for each of these cases.
The first one is covered by the string "directoryUsedByOtherAccount" but the others need to be something like:
This directory is a subdirectory of the one used by the %S account. Please pick a different directory.
This directory is a parent directory to the one used by the %S account. Please pick a different directory.

f- as I would like to see the new patch.
Attachment #638218 - Flags: feedback?(iann_bugzilla) → feedback-
Ian, why would it be worth to have different alerts? Isn't the one common string enough?
(In reply to :aceman from comment #30)
> Ian, why would it be worth to have different alerts? Isn't the one common
> string enough?

To make it more clear what the issue is. Not sure if this is the same issue as Blake described in comment 25 or not.
Then we could change the existing string to something like:
"This or its containing/contained directory is already used by the %S account. Please pick a different directory."
(In reply to :aceman from comment #32)
> Then we could change the existing string to something like:
> "This or its containing/contained directory is already used by the %S
> account. Please pick a different directory."

My preference is to have 3 strings, but probably best to check with Blake as it needs to be the same for both TB and SM.
Comment on attachment 638218 [details] [diff] [review]
patch

Bwinton, can you comment on out last comments about the 3 separate error strings?
Attachment #638218 - Flags: feedback?(bwinton)
Comment on attachment 638218 [details] [diff] [review]
patch

I think the three different strings would be a good idea, since then we could make the error messages that much more explanatory.
Attachment #638218 - Flags: feedback?(bwinton)
Blocks: 575077
Attached patch patch v2 (obsolete) — Splinter Review
OK, this disambiguates the 3 strings and seems to also fix bwinton's issue. Selecting home should now say "a subdir of the selected dir is already used by other account". It seems to have a problem with symlinks (selecting HOME when .thunderbird is a symlink to somewhere else does not trigger the error). But better than nothing.
Attachment #638218 - Attachment is obsolete: true
Attachment #652221 - Flags: ui-review?(bwinton)
Attachment #652221 - Flags: review?(iann_bugzilla)
Status: NEW → ASSIGNED
Keywords: uiwanted
Comment on attachment 652221 [details] [diff] [review]
patch v2

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

>+  if (!checkDirectoryIsUsable(getAccountValue(currentAccount, accountValues, "server", "localPath", null, false)))
Nit: watch the line length, this can be wrapped to get it <80 characters.

>+  if (currentPageId == "am-server.xul") {
>   // check if user/host names have been changed
>+  if (!checkUserServerChanges(false)) {
>+    changeView = true;
>+    account = currentAccount;
>+    pageId = currentPageId;
>+  }
> 
>   if (gSmtpHostNameIsIllegal) {
>     gSmtpHostNameIsIllegal = false;
>     selectServer(currentAccount.incomingServer, currentPageId);
>     return true;
>   }
>+  }
Nit: you need to correct the indentation of the above sections with the new if condition.

r=me with those fixed.
Attachment #652221 - Flags: review?(iann_bugzilla) → review+
Still need to fix my comment 23...
Comment on attachment 652221 [details] [diff] [review]
patch v2

>+++ b/mail/locales/en-US/chrome/messenger/prefs.properties
>@@ -29,17 +30,19 @@ failedRemoveAccount=Failed to remove thi
>+directoryAlreadyUsedByOtherAccount=The directory specified in the Local Directory setting is already used by the "%S" account. Please pick a different directory.
>+directoryParentUsedByOtherAccount=A parent directory of the directory specified in the Local Directory setting is already used by the "%S" account. Please pick a different directory.
>+directoryChildUsedByOtherAccount=A subdirectory of the directory specified in the Local Directory setting is already used by the "%S" account. Please pick a different directory.

These seem kind of verbose, and a little confusing, but I think they're good enough for now.

ui-r=me!

Thanks,
Blake.
Attachment #652221 - Flags: ui-review?(bwinton) → ui-review+
Attached patch patch v3 (obsolete) — Splinter Review
OK, this fixes my comment 23, it does not use a fixed page name but checks for the existence of the "id=localPath" field on the page.
While testing that I found selectServer does not properly select the page it was passed (that functionality probably wasn't used much till now). So I make some changes there.

Ian, please try this in SM:
In prefs.js give some invalid path for an account (user_pref("mail.server.server1.directory","..."), something that can't be created when SM starts. Then in SM go to account manager, select that accounts Server settings. Then select another page (in the left tree). You should get the warning about invalid path and the selection should automatically go back to the Server settings line. So you should not be allowed to leave the Server settings page (or the main page in case of News/Local folders). It works in TB now, please check in SM too.

I also has to add a try {} catch around aLocalPath.normalize() in checkDirectoryIsUsable as in some cases that failed (when the path was really bad).
Attachment #652221 - Attachment is obsolete: true
Attachment #656026 - Flags: review?(iann_bugzilla)
If you change your local mail account directory to something that doesn't exist:
mail.server.server1.directory-rel to [ProfD]/Mail/Example
Then go to the Local Folders pane in the account manager, it won't let you move away from that even if you select a suitable folder.
Attachment #656026 - Flags: review?(iann_bugzilla) → review-
Attached patch patch v4 (obsolete) — Splinter Review
OK, let's try this. 
- properly check NEW path entered in the Local Directory
- another attempt to fix selectServer (should now work at both POP3 account and also Local Folders (the pane with Local Directory is at a different level in the tree))
- also fix properly restarting after Local Directory change.
Attachment #656026 - Attachment is obsolete: true
Attachment #658580 - Flags: review?(iann_bugzilla)
Comment on attachment 658580 [details] [diff] [review]
patch v4

r=me that's fixed it for me, though i've not tested every possible iteration of options.
Attachment #658580 - Flags: review?(iann_bugzilla) → review+
Attachment #658580 - Flags: review?(mconley)
Comment on attachment 658580 [details] [diff] [review]
patch v4

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

Just a few nits in here - nothing major. Overall, it's really nice code. Let me know if you have any questions.  Thanks aceman!

::: mailnews/base/prefs/content/AccountManager.js
@@ +195,5 @@
>  
> +/**
> + * Called when OK is clicked on the dialog.
> + *
> + * @param aNoChecks  If true, do not execute checks on data (hoping they

I find it a tiny bit confusing to have a boolean have the negation built in to the meaning.

Like, it's strange to say "Yes for noChecks" or "No for noChecks", as opposed to just saying "Yes checks" or "No checks".

Can we have this boolean called "doChecks" instead, and flip the meaning, boolean-wise?

@@ +232,5 @@
> + *
> + * aLocalPath  the nsIFile of a directory to check.
> + */
> +function checkDirectoryIsValid(aLocalPath) {
> +  // Any directory selected in the file picker does already exist.

Nit - "does already exist" -> "already exists"

@@ +235,5 @@
> +function checkDirectoryIsValid(aLocalPath) {
> +  // Any directory selected in the file picker does already exist.
> +  // Any directory specified in prefs.js will be created at start if it does
> +  // not exist yet.
> +  // If at the time of entering Account manager the directory does not exist,

Nit: capital M on Manager.

@@ +245,5 @@
> +  if (Services.appinfo.OS == "WINNT") {
> +    // Do not allow some special filenames on Windows.
> +    // Taken from mozilla/widget/windows/nsDataObj.cpp::MangleTextToValidFilename()
> +    let dirLeafName = aLocalPath.leafName;
> +    let forbiddenNames = [

For stuff like this, I like to set it to a const.

@@ +262,5 @@
> +}
> +
> +/**
> + * Check if the specified directory does meet all the requirements
> + * for safe mail storage.

I'm really liking the documentation you're putting in here. +1.

@@ +267,5 @@
> + *
> + * aLocalPath  the nsIFile of a directory to check.
> + */
> +function checkDirectoryIsUsable(aLocalPath) {
> +  const alertTitle = document.getElementById("bundle_prefs")

nit - we generally prefix consts with k, so kAlertTitle.

@@ +269,5 @@
> + */
> +function checkDirectoryIsUsable(aLocalPath) {
> +  const alertTitle = document.getElementById("bundle_prefs")
> +                             .getString("prefPanel-server");
> +  let alertString = null;

Why does alertString need to be declared up in this scope? Doesn't seem to be used outside of the blocks below...

@@ +284,5 @@
> +  }
> +
> +  // Check that no other account has this same or dependent local directory.
> +  let allServers = MailServices.accounts.allServers;
> +  for (let i = allServers.Count(); --i >= 0;) {

Why are we doing this backwards? If order does not matter, you can (I believe) use iteratorUtils, like this:

for each (let server in fixIterator(allServers, Ci.nsIMsgIncomingServer)) {
  // Do stuff with server
}

@@ +290,5 @@
> +      .QueryElementAt(i, Components.interfaces.nsIMsgIncomingServer);
> +    if (currentServer.key == currentAccount.incomingServer.key)
> +      continue;
> +
> +    let alertStringID = null;

We don't use alertStringID out in this scope - I think this can be brought into the try block.

@@ +309,5 @@
> +
> +        Services.prompt.alert(window, alertTitle, alertString);
> +        return false;
> +      }
> +    } catch (e) { /* the other account's path is seriously broken, so just ignore it. */ }

I think we should report this case in the Error Console at the very least, as opposed to failing silently.

@@ +428,5 @@
>      }
>    }
>  
> +  // Check the new value of the server.localPath field for validity.
> +  for (let i = 0; i < pageElements.length; i++) {

Instead of iterating through the pageElements, can't you use document.getElementById, or querySelector?
Attachment #658580 - Flags: review?(mconley) → review-
Attached patch patch v5 (obsolete) — Splinter Review
Done, except the last point for which I'll create a new bug as the same optimization could be used for other similar loops in the file.
Attachment #658580 - Attachment is obsolete: true
Attachment #660974 - Flags: review?(mconley)
Blocks: 791093
Comment on attachment 660974 [details] [diff] [review]
patch v5

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

Assuming the below is a problem and gets fixed, r=me. Thanks!

::: mailnews/base/prefs/content/AccountManager.js
@@ +200,5 @@
> + *                   were already done elsewhere and proceed directly to saving
> + *                   the data.
> + */
> +function onAccept(aDoChecks) {
> +  if (!aDoChecks) {

The logic needs to be flipped here, I think - we want to execute this block is aDoChecks is true, right?
Attachment #660974 - Flags: review?(mconley) → review+
Attached patch patch v6Splinter Review
Right, good catch!
Attachment #660974 - Attachment is obsolete: true
Attachment #660995 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ab785eab3a80
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
See Also: → 817681
You need to log in before you can comment on or make changes to this bug.