1709165 - Need to add account info to IMAP error messages like "Unable to open the summary file for {Inbox}. Perhaps there was an error on disk, or the full path is too long."

Status: RESOLVED FIXED

(MailNews Core :: Database, task, P2)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1708842 +++

When we show serious errors, both users and developers need more information. Knowing for which account the error occured is crucial.

STR

For reasons unknown, get error message "Unable to open the summary file for [Inbox]. Perhaps there was an error on disk, or the full path is too long." (as reported on several bugs).

Actual

• No idea which account caused the error.
• Debugging impossible.

Expected

• The error message should include account name and account type (apparently we only throw this error for IMAP, see code analysis below).

Starting points in code:

https://searchfox.org/comm-central/rev/6553298570f51f6b707eb757d1fdb14bf9876a0d/mail/locales/en-US/chrome/messenger/messenger.properties#93
errorGettingDB=Unable to open the summary file for %S. Perhaps there was an error on disk, or the full path is too long.

%S represents the folder. We want another variable for the account name.

https://searchfox.org/comm-central/rev/0141a51739d87d81c88b7d744084d3844ea6ef83/mailnews/imap/src/nsImapMailFolder.cpp#749

  rv = GetDatabase();
  if (NS_FAILED(rv)) {
    ThrowAlertMsg("errorGettingDB", aMsgWindow);
    return rv;
  }

Comment 1

[Thomas D. (:thomas8)]

:BenC, how hard would it be to add the account name to this error message?

We have a number of nasty bugs around this and near zero insight. Knowing the account name might help to narrow down problems...

(In reply to Thomas D. (:thomas8) from comment #0)

https://searchfox.org/comm-central/rev/0141a51739d87d81c88b7d744084d3844ea6ef83/mailnews/imap/src/nsImapMailFolder.cpp#749

  rv = GetDatabase();
  if (NS_FAILED(rv)) {
    ThrowAlertMsg("errorGettingDB", aMsgWindow);
    return rv;
  }

Comment 2

[Ben Campbell]

Attachment: 1709165-add-account-name-to-errorGettingDB-1.patch

Yes, I think the account name is important info!

Here's a patch which does just that. You might have a better way to word the error message...

Alternatively, maybe it'd be better to add the account name to all the generic alerts thrown up by the IMAP folder code? (i.e. make ThrowAlertMsg() always show the account name before the error message)
I notice the title isn't being used for the alert dialog - maybe that'd be a good place to show the account name (and folder), e.g. "Alert - Inbox on bob@localhost". I think all platforms would show the title, although 100% sure about Mac.

What do you think?

Comment 3

[Axel Schmidt]

Hi Ben,

you are right, the account info is an important info, but I can't handle the attachment, because I don't know the sources. So if you have a (nightly) build with a correction, please send me the link or installation file I will check the correction, because this problem occurs sometimes on one of my PCs, so I can verify the correction.
Thanks in advance
Axel

Comment 4

[Wayne Mery (:wsmwk)]

I (and hopefully speaking for all triagers) would like the account name in all circumstances.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9221036 [details] [diff] [review]
1709165-add-account-name-to-errorGettingDB-1.patch

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +89,5 @@
>  filterFolderTruncateFailed=There was an error truncating the Inbox after filtering a message to folder '%1$S'. You may need to shutdown %2$S and delete INBOX.msf.
>  
>  mailboxTooLarge=The folder %S is full, and can't hold any more messages. To make room for more messages, delete any old or unwanted mail and compact the folder.
>  outOfDiskSpace=There is not enough disk space to download new messages. Try deleting old mail, emptying the Trash folder, and compacting your mail folders, and then try again.
> +errorGettingDB=Unable to open the summary file for %1$S (under %2$S). Perhaps there was an error on disk, or the full path is too long.

you'd have to update the l10n key if you change the content

::: mailnews/base/src/nsMsgDBFolder.cpp
@@ +4677,5 @@
> +  // Show it.
> +  nsCOMPtr<nsIPrompt> dialog;
> +  rv = window->GetPromptDialog(getter_AddRefs(dialog));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  dialog->Alert(nullptr, formattedMsg.get());

I guess we could also just add the account to the alert title.

Error - [Account Name]

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 9221036 [details] [diff] [review]
1709165-add-account-name-to-errorGettingDB-1.patch

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

(In reply to Ben Campbell from comment #2)
> Created attachment 9221036 [details] [diff] [review]
> Yes, I think the account name is important info!

Thanks Ben for looking into this, and sorry that this has slipped through the cracks!

> Alternatively, maybe it'd be better to add the account name to _all_ the generic alerts thrown up by the IMAP folder code?

Yes, as others have said, that would be awesome!!!

> I notice the title isn't being used for the alert dialog - maybe that'd be a good place to show the account name (and folder), e.g. "Alert - Inbox on bob@localhost". I _think_ all platforms would show the title, although 100% sure about Mac.

I kinda like Magnus' proposal:

Error - `account name`

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +89,5 @@
>  filterFolderTruncateFailed=There was an error truncating the Inbox after filtering a message to folder '%1$S'. You may need to shutdown %2$S and delete INBOX.msf.
>  
>  mailboxTooLarge=The folder %S is full, and can't hold any more messages. To make room for more messages, delete any old or unwanted mail and compact the folder.
>  outOfDiskSpace=There is not enough disk space to download new messages. Try deleting old mail, emptying the Trash folder, and compacting your mail folders, and then try again.
> +errorGettingDB=Unable to open the summary file for %1$S (under %2$S). Perhaps there was an error on disk, or the full path is too long.

I'd avoid the brackets, and 'under' should be 'on'. But probably we won't need the account here at all if we put it in the alert title. Then you can just leave existing error messages as they are, and do the account info magic in a new string!

Comment 7

[Ben Campbell]

Attachment: Bug 1709165 - Show folder and account name in IMAP folder error alerts. r=thomasd,mkmelin.

Comment 8

[Ben Campbell]

WIP patch, just shows the account name directly as the dialog title. I'll revise it to add a localised string to format it (ie "Error - bob@localhost").
There's an argument for including it in the dialog body itself, rather than the title. It'd make for easier cut&paste with the rest of the message. Anyone have any strong opinions on that?

Comment 9

[Thomas D. (:thomas8)]

(In reply to Ben Campbell from comment #8)

WIP patch, just shows the account name directly as the dialog title. I'll revise it to add a localised string to format it (ie "Error - bob@localhost").
There's an argument for including it in the dialog body itself, rather than the title. It'd make for easier cut&paste with the rest of the message.

Thanks Ben, that's a very good point. Having the account name available in the alert text for copy & paste will definitely be helpful for support scenarios. There's a bit of a privacy side to that like when you post your error message in a public support forum and it will have your email address. Maybe users can be expected to handle that in their own responsibility?

As for embedding the account name in the error message, it appears that current localization already had this plan:
https://searchfox.org/comm-central/rev/6553298570f51f6b707eb757d1fdb14bf9876a0d/mail/locales/en-US/chrome/messenger/messenger.properties#86-87

# LOCALIZATION NOTE(verboseFolderFormat): %1$S is folder name, %2$S is server name
verboseFolderFormat=%1$S on %2$S

So that would come out as Inbox on [e-mail] - which is quite OK imo.
Maybe we could be more explicit and change that to Inbox of the account [e-mail], so for our error message the net outcome would be (slightly modified by adding the folder):

Unable to open the summary file for the folder Inbox of the account [e-mail]. Perhaps there was an error on disk, or the full path is too long.

That sounds clear and informative to me, and the same strategy can work for all error messages which mention a folder.
Some of the error messages currently do not mention the folder, but should.
So if you have folder and account name in their own combined string, you can then replace the single current folder variable with that string.

Comment 10

[Thomas D. (:thomas8)]

I understand that we cannot at this point change the error messages to Fluent because we're calling them from C++ code, is that so?

Comment 11

[Ben Campbell]

Thanks Thomas, all good advice!
New revision - let me know what you think.
This one uses the "verboseFolderFormat" to construct a "Drafts on bob@localhost" string.
That string is used as the folder for messages that expect the folder name.

ThrowAlertMsg() is a slightly odd function - the error message ID is passed in without any hint as to what formatting params are expected. There are actually a couple of params passed in, but a lot of messages just ignore one or all of them, which seems a little icky... I can think of a couple of ways to improve this, but not sure it's worth the bother of rewriting all the messages.
With all that in mind, I also use the string in the title of the alert, so there'll always be a title like: "Alert - Drafts on bob@localhost", even for messages that don't mention the folder themselves.

(In reply to Thomas D. (:thomas8) from comment #10)

I understand that we cannot at this point change the error messages to Fluent because we're calling them from C++ code, is that so?

Afraid I'm completely ignorant about both the past and future plans regarding localisation stuff :-( For this patch I'm just fiddling with what's already there. Let me know if you think some followup would be beneficial and I'll try and educate myself!

Comment 12

[Thomas D. (:thomas8)]

(In reply to Ben Campbell from comment #11)

Thanks Thomas, all good advice!
New revision - let me know what you think.

Thanks, this looks good - see my phabricator review for some comment nits.

This one uses the "verboseFolderFormat" to construct a "Drafts on bob@localhost" string.
That string is used as the folder for messages that expect the folder name.

Yeah, I think that's better than what we have. Note that folder identifier will be quoted by some messages (I guess we can live with that):
...for folder 'Archive on [e-mail]'...

ThrowAlertMsg() is a slightly odd function - the error message ID is passed in without any hint as to what formatting params are expected.

That looks ok to me, because the parameters are always more or less the same.

There are actually a couple of params passed in, but a lot of messages just ignore one or all of them, which seems a little icky... I can think of a couple of ways to improve this, but not sure it's worth the bother of rewriting all the messages.
With all that in mind, I also use the string in the title of the alert, so there'll always be a title like: "Alert - Drafts on bob@localhost", even for messages that don't mention the folder themselves.

That will suffice here - we can always improve strings in a followup bug.

(In reply to Thomas D. (:thomas8) from comment #10)

I understand that we cannot at this point change the error messages to Fluent because we're calling them from C++ code, is that so?

Afraid I'm completely ignorant about both the past and future plans regarding localisation stuff :-( For this patch I'm just fiddling with what's already there. Let me know if you think some followup would be beneficial and I'll try and educate myself!

This will be good enough until C++ goes away. I wouldn't know how to put Fluent in the mix when we're in C++, as Fluent works in the Javascript/HTML layer.

Comment 13

[Magnus Melin [:mkmelin]]

Probably not worth moving to Fluent here, but you can use it in c++ as well. See e.g. https://searchfox.org/mozilla-central/rev/294f10eff7398d6b05beac6aa256d86ac3cb7113/widget/nsPrinterListBase.cpp#138,147

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bd979e263f60
Show folder and account name in folder alert dialogs. r=thomasd,mkmelin

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2118a24a4a43
Backed out changeset bd979e263f60 for test failures in comm/mailnews/local/test/unit/test_over4GBMailboxes.js. r=backout DONTBUILD

Comment 16

[Magnus Melin [:mkmelin]]

Somehow this caused problems for the test.

Comment 17

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #16)

Somehow this caused problems for the test.

Wow - that's uncanny! I was doing some work on test_over4GBMailboxes.js as I was finalising this patch, but it's totally unrelated to any of that. It's down to the test expecting specific text in an alert (In English!).
Yet another argument in favour of better separation of GUI code...

New revision passes locally, but had trouble running a try build.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c13e2a5ef6e
Show folder and account name in IMAP folder error alerts. r=thomasd,mkmelin.