1690093 - Dragging a folder from an authenticated server to a not yet authenticated server fails

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

This was originally described in the top half of Bug 1688782 Comment 0. I decided to make it a separate bug because it is not UTF8=ACCEPT related and my proposed fix needs additional discussion independent of the bottom half proposed fix of Bug 1688782 Comment 0.

This issue was pointed out in bug 1687727 comment 51 and bug 1687727 comment 64 in relation to follow-on fixes to the new UTF8=ACCEPT feature.

If you drag a folder from account A to an existing folder or the root of account B and account B uses plaintext (normal) password and has not yet connected with the IMAP server during the current tb session, the copy/move operation never occurs and no error is indicated. But looking at the IMAP log during this activity, the indication of a problem is clear:

D/IMAP Try to log in
D/IMAP IMAP auth: server caps 0x20044e3325, pref 0x1006, failed 0x0, avail caps 0x1004
D/IMAP (GSSAPI = 0x1000000, CRAM = 0x20000, NTLM = 0x100000, MSN = 0x200000, PLAIN = 0x1000, LOGIN = 0x2, old-style IMAP login = 0x4, auth external IMAP login = 0x20000000, OAUTH2 = 0x800000000)
D/IMAP Trying auth method 0x1000
E/IMAP IMAP: password prompt failed or user canceled it
E/IMAP login failed entirely
D/IMAP SetConnectionStatus(0x80004005)
D/IMAP URL failed with code 0x80004005 (imap://[e-mail]:143/ensureExists%3E.INBOX.unique-%26AOT%2Cww-)
I/IMAP 0x7fddd30b1000:mail.tana.it:NA:ProcessCurrentURL: aborting queued urls
I/IMAP 0x7fddd30b1000:mail.tana.it:NA:TellThreadToDie: close socket connection
D/IMAP ImapThreadMainLoop leaving [this=0x7fddd30b1000]
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:SendData: 16 close
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:SendData: 17 logout
I/IMAP 0x7fddd7211800:mobile.charter.net:S-unique-&AOT,ww-:TellThreadToDie: close socket connection

A possible stored password is not obtained or prompted for so it is never sent to the server so login fails and the connection is dropped. The folder copy never occurs so the folder never appears at the destination account and there is no error indication or hint as to what the problem might be.

Comment 1

[gene smith]

This also fails, for the same reason, when the subscribe dialog is requested (right click on account name, select "subscribe") and the server is not yet authenticated. The subscription screen comes up empty (no folders are shown) and no error is indicated because the connection is dropped in the same way as the log shows in comment 0.

The problem originates here:

https://searchfox.org/comm-central/rev/4eb14a03690b8d6bbe53853a33b6e09d330a05ea/mailnews/imap/src/nsImapProtocol.cpp#8195

because msgWindow is null. In the case where the very first URL requested to run for an account is "EnsureExists" or "subscribe", there is no "window" associated with the URL. However, for the "select" URL, which is typically the first URL to run when the user clicks a folder, there is a window associated with the URL. Without this window pointer, the call to GetPassword() currently returns an error and a prompt for a password never occurs. Also, even if the correct password was stored for the account, it is never retrieved due to the null msgWindow pointer.

If the NS_ENSURE_TRUE(msgWindow, NS_ERROR_NOT_AVAILABLE); // biff case is removed and there is a previously stored password for the account, the problem is partially solved. In this case, the correct password is used during the authentication and the "login failed entirely", shown in comment 0, does not occur.

However, if the user opts to not store the password but enters it manually, this doesn't help. Without a "msgWindow" with the URL, as will occur with "EnsureExists" or "subscribe" no prompt for password ever occurs and the "login failed entirely" still occurs in the log and the user is not notified.

Without changes at the UI level to somehow provide a "window" with window-less URLs like "EnsureExists", it is possible to provide an alert notification of the problem from within the GetPassword() function as shown in the attached diff. It would only occur if the stored password is empty and "msgWindow" is null so a prompt for a password can't occur. Also all it does is inform the user that they must first open a folder on the destination account for the operation just attempted to work.

Regarding biff, if the ENSURE_TRUE for msgWindow is removed it does not seems to cause any problems with biff. If there is no password stored for the account, biff never seems to occur and no prompt for a password occurs. I never see GetPassword() called during biff either. The line NS_ENSURE_TRUE(msgWindow, NS_ERROR_NOT_AVAILABLE); // biff case was added in Bug 525238 with no explanation as to what "biff case" this addresses.

Comment 2

[gene smith]

Attachment: Possible fix for GetPassword() due to null msgWindow (t.diff)

Including Ben C. for feedback on this since I know he doesn't like the "alert" calls directly from nsImapProtocol.cpp.

Also, don't know why, but I only see the alert in "Activity Manager" and not as pop-ups from tray like I expected. Maybe a linux problem or maybe due to running home-built daily?

Comment 3

[Klaus B.]

Uff, a patch with three lines context, that's impossible to look at. Please set

[diff]
git = 1
showfunc = 1
unified = 8

in your mercurial.ini. There's really not enough context, I'd have to pull out the file.

Comment 4

[Klaus B.]

... was added in Bug 525238 with no explanation as to what "biff case" this addresses.

The original author and reviewer from this bug are still around, you can NI them ;-) - See whether they remember. "Biff" means displaying a notification for a new message, no?

Comment 5

[gene smith]

Attachment: AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

(In reply to Klaus B. from comment #3)

Uff, a patch with three lines context, that's impossible to look at. Please set

[diff]
git = 1
showfunc = 1
unified = 8

in your mercurial.ini. There's really not enough context, I'd have to pull out the file.

Don't know what "Uff" means but it sounds unpleasant :).
Yeah, sorry about that. I forgot to update default hgrc with that on this new computer. (I guess it wasn't a problem with the other diffs you've looked at with the same hgrc.)

I've requested info on this from Ben Bucksch regarding this area of the code from 11 years ago.

Edit: Yes, I know "biff" just means checking for new mail every X minutes and producing a notification. Just not sure why the NS_ENSURE_TRUE is present and marked with comment "biff case", and when msgWindow is false/null, GetPassword() returns an error and causes a complete server disconnect. And not sure what a msgWindow even is and why some URLs have one ("select") while others don't (ensureExists).

Comment 6

[Klaus B.]

Comment on attachment 9200591 [details] [diff] [review]
AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

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

Looks OK, much more readable with more context, thanks. The "gds" logging will need to go, if course.
For your other question: https://7esl.com/uff/ ;-)
Also, Neil is your better bet here.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8475,5 @@
>        newPasswordRequested = false;
>        if (rv == NS_MSG_PASSWORD_PROMPT_CANCELLED || NS_FAILED(rv)) {
>          MOZ_LOG(IMAP, LogLevel::Error,
>                  ("IMAP: password prompt failed or user canceled it"));
> +          AlertUserEventUsingName("imapAuthChangeEncryptToPlainSSL");

Where did this come from? Is that to match the alert above?

Comment 7

[Ben Bucksch (:BenB)]

Comment on attachment 9200591 [details] [diff] [review]
AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8254,5 @@
> +  if (!password.IsEmpty()) {
> +    MOZ_LOG(IMAP, LogLevel::Debug, ("gds: pwd not empty"));
> +    m_lastPasswordSent = password;
> +  }
> +  else  {

Same as below, you're fixing your case, but you're breaking other cases. You are presuming that this is your case, while this would trigger in many more cases as well, where the message is not warranted.

@@ +8475,5 @@
>        newPasswordRequested = false;
>        if (rv == NS_MSG_PASSWORD_PROMPT_CANCELLED || NS_FAILED(rv)) {
>          MOZ_LOG(IMAP, LogLevel::Error,
>                  ("IMAP: password prompt failed or user canceled it"));
> +          AlertUserEventUsingName("imapAuthChangeEncryptToPlainSSL");

This is wrong. This will prompt an error to the user, after the user has explicitly canceled the prompt.

Comment 8

[gene smith]

Klaus wrote:

•      AlertUserEventUsingName("imapAuthChangeEncryptToPlainSSL");
  

Where did this come from? Is that to match the alert above?

and
Ben Bucksch wrote:

•      AlertUserEventUsingName("imapAuthChangeEncryptToPlainSSL");
  

This is wrong. This will prompt an error to the user, after the user has explicitly canceled the prompt.

Oops, sorry, forgot to remove this line. The alert I added only showed up in Activity Mgr and never in a pop-up. Just dropped that one in to see if an alert line that was used before in nsImapProtocol.cpp would cause a pop-up. It didn't either. Don't know why, but that's a secondary issue.

Ben B. wrote:

Same as below, you're fixing your case, but you're breaking other cases. You are presuming that this is your case, while this would trigger in many more cases as well, where the message is not warranted.

By "cases" I assume you mean other authentication methods than just normal/plain password? Or maybe "biff case"?

Also, I was wondering if you could shed light on the significance of msgWindow and the line

NS_ENSURE_TRUE(msgWindow, NS_ERROR_NOT_AVAILABLE); // biff case

which seems to prevents using the stored password or a prompting for a new password when the first URL has no "window" associated with it like "EnsureExists". Typically, the first URL that occurs is "select" which does have a window:
Select:
https://searchfox.org/comm-central/rev/f6018345454b019bd41c50da80c2bb34f042db86/mailnews/imap/src/nsImapService.cpp#160
EnsureExists, no window:
https://searchfox.org/comm-central/rev/f6018345454b019bd41c50da80c2bb34f042db86/mailnews/imap/src/nsImapService.cpp#2244

Thanks!

Comment 9

[Ben Bucksch (:BenB)]

By "cases" I assume you mean other authentication methods than just normal/plain password?

I mean a large number of cases. Your change affects all cases where no password could be obtained. It could be: other authentication methods, it could be biff, or other non-interactive connections, it could be the user pressing Cancel on the login prompt, it could be various error situations, and many others. You cannot just presume that this is just your case here.

Comment 10

[Ben Campbell]

Comment on attachment 9200591 [details] [diff] [review]
AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -8242,1 @@
>      // Get the password from pw manager (harddisk) or user (dialog)

My guess is that the msgWindow check is designed to test if it's an unattended operation (the "biff" case) or if it's OK to prompt the user for input.
So the proposed changes bypass this test, which feels a bit dangerous here.
(Presumably the AsyncGetPassword() pops up GUI prompts, which is what the check seems to be trying to avoid).

Comment 11

[gene smith]

My guess is that the msgWindow check is designed to test if it's an unattended operation (the "biff" case) or if it's OK to prompt the user for input.
So the proposed changes bypass this test, which feels a bit dangerous here.
(Presumably the AsyncGetPassword() pops up GUI prompts, which is what the check seems to be trying to avoid).

When I took out the NS_ENSURE_TRUE and let it call AsyncGetPassword() with msgWindow null (as is the case for non-select URLs) nothing popped up to enter a password even though I had deleted the stored password.

I guess what I don't understand (along with a lot of stuff) is what this msgWindow really means. It's either set or not set in the two calls I link to in comment 8 (set when select, not set when ensureExists it seems). If it was set for ensureExists too there would be no problem with the code as-is, I think.

Comment 12

[Ben Bucksch (:BenB)]

My guess is that the msgWindow check is designed to test if it's an unattended operation

Agreed. Which suggests the real fix here is to make sure that there's a msgWindow populated for these operations where you do want to have a prompt.

Make sure these operations were directly initiated by the user, i.e. the user clicked into that exact msgWindow a second earlier and contiously initiated the action that is triggering the prompt.

I guess what I don't understand (along with a lot of stuff) is what this msgWindow really means

It's just the TB window (3 pane window, or maybe standalone message viewer) that the user was interacting with, when he started the operation. Any UI should show in that window. If you didn't get a window (i.e. no msgWindow reference), you shouldn't show UI.

Comment 13

[alta88]

Maybe the fix in Bug 1071754 might offer some insight into msgWindow and "attended" operations.

Comment 14

[gene smith]

Attachment: t3.diff

Here's another possibility that doesn't use the "alert" at all. I did this before seeing the previous two comments from Ben B. and alta88.

This skips the prompt for password if the msgWindow is null AND if biff is in progress (if that's what GetPerformingBiff() actually tells you). If just msgWindow is null, it now obtains the "topmost" window to produce the prompt. This fixes the problem as described in comment 0, but I definitely haven't tested all the different cases.

I noticed that the patch that alta88 did and points out also uses "topmost" window when the msgWindow is null. So maybe I'm on the right track.
Edit: I left a couple of commented out code lines in the diff so just ignore them.

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 9200817 [details] [diff] [review]
t3.diff

As I said: You need to pass in the actual msgWindow where the end user directly initiated the operation, i.e. the user clicked into that exact window a second earlier. He must have contiously initiated the action that is triggering the prompt, in that exact window.

If you cannot get that window, you shouldn't show any UI at all.

uses "topmost" window when the msgWindow is null

Yes, that's wrong. The popup might be in the wrong window, depending on situation, timing etc., or might show in cases where it shouldn't show up at all.

Comment 16

[alta88]

(In reply to Ben Bucksch (:BenB) from comment #15)

Comment on attachment 9200817 [details] [diff] [review]
t3.diff

As I said: You need to pass in the actual msgWindow where the end user directly initiated the operation, i.e. the user clicked into that exact window a second earlier. He must have contiously initiated the action that is triggering the prompt, in that exact window.

This is obvious.

If you cannot get that window, you shouldn't show any UI at all.

This is false. For proper "attended" requests, async calls with callbacks will include the contextual msgWindow argument, and the mandatory prompts will happen correctly. But not all user initiated ultimately async resolutions have msgWindow in callbacks. So for these cases (which should be audited, especially for imap operations, and are likely numerous) it is mandatory to figure out, by hook or crook, an msgWindow in order to show a prompt if it is determined the request was user initiated. And there is even a method to do that.

uses "topmost" window when the msgWindow is null

Yes, that's wrong. The popup might be in the wrong window, depending on situation, timing etc., or might show in cases where it shouldn't show up at all.

If it is determined to be user initiated, it is not wrong. This statement contains a lot of guessy hypotheticals that are not useful.

And after all these years you still fail at basic bugzilla etiquette. Seriously, WTF. You are not to remove r/f/ni flags for anyone unless set by yourself or to yourself. You are not a peer of this or any module. You do not have r- authority for any r? unless it is to you. Is this clear?

Comment 17

[Ben Bucksch (:BenB)]

You need to pass in the actual msgWindow where the end user directly initiated the operation, i.e. the user clicked into that exact window a second earlier. He must have contiously initiated the action that is triggering the prompt, in that exact window.

This is obvious.

Glad you agree. That means you cannot use getTopMostWindow(), because - by very nature of the call - you cannot be sure that it's the same window that the user clicked in.

if it is determined the request was user initiated

So, how do you determine this? By whether the original caller - where the click happened - passed a msgWindow in. That's how this code works. If some callers do not currently pass in a msgWindow, they should be fixed. If there are many, then well, there are many places to fix. All the better to straighten this out.

getTopMostWindow() will go wrong in a number of cases. And, most importantly, violates the assumption in this code that msgWindow == null means non-interactive. Just grabbing any window when we need a password, from the backend, irregardless of where the call comes from, is violating that contract. Instead, the window reference needs to be grabbed in the UI where the click happens, and then passed on properly.

This is a classical problem. I've seen this bug a thousand times, in several projects.

FWIW, why I did the review here: This is code that I wrote. I wrote the very line that's being modified here. Custom is that the author of code also has review rights. After all, I am a legal owner of this code. I'd rather you say thank-you than attack me for doing review duty.

My interest is simply to ensure that the code change doesn't break anything. No need to be getting personal here, nor vulgar.

Comment 18

[gene smith]

Maybe I should state again what I'm trying to fix. If a folder is dragged from account A to account B and account B is has not connected to and authenticated with the IMAP server of account B, nothing happens at all. This happens if account B is set to not check mail at startup and not check mail periodically and no folder in account B has been opened by the user.

My experimental changes in t3.diff "fix" that problem and now when the folder is "dropped" a prompt for account B's password occurs.

Next I made a filter to copy messages with a specific word in the subject from account A to account B and attempted to "Run Now" the filter. When this occurs, an error pops up on top of the "Message Filters" dialog saying "Filter .... failed. Would you like to continue applying filters?". If I dismiss the message with OK or Cancel, doesn't matter, it goes away and the "Message Filters" dialog remains. At the bottom it says "Copy message 1 or 14 to <folder on account B>" but no copy actually occurred.

Looking at the MOZ_LOG items I added (now in t4.diff) I see that a non-null msgWindow was provided OK but in the loop waiting for password to be entered, m_passwordStatus is set to 0x80004002 and no password is ever prompted for. The error means "An attempt was made to call QueryInterface to retrieve an interface which an object does not support." Looking closer, I see that the 0x80004002 error is set here: https://searchfox.org/comm-central/rev/f5f081ba007d398dace13ad30cd571f5132ad4a1/mailnews/imap/src/nsImapProtocol.cpp#8285.

I haven't looked inside PromptPassword() but it appear that the "msgWindow" being passed in is the window for "Message Filters" dialog used to create and test the filter with "Run Now" and that window doesn't support the password prompt (doesn't have the interface).

If I let the filter I just created run naturally (Display Filter window closed) when a new message arrives with a certain word in the subject, the password prompt appears, as I would expect now, for account B. In this case, the msgWindow OK as-is and allows the prompt to occur so the filter does the copy.

So it appears that if msgWindow is null inside GetPassword(), using "top most" window works OK. And if "top most" window should also be null, then no prompt for password occurs (not sure if "top most" can be null). Its also possible for msgWindow inside GetPassword() to be incompatible with the password prompt and refuse to display it, which seems to be the case with "Display Filter" window when I do "Run Now".

In attachment t4.diff, I've found another "experimental fix" for the missing password prompt when doing "Run Now". It requires retrying the call to PromptPassword() if it returns NS_ERROR_NO_INTERFACE and passing in "top most" instead. Now when "Run Now" is clicked, the password prompt for account B pops up and the filter runs as expected and no error dialog occurs.

So, how do you determine this? By whether the original caller - where the click happened - passed a msgWindow in. That's how this code works. If some callers do not currently pass in a msgWindow, they should be fixed. If there are many, then well, there are many places to fix. All the better to straighten this out.

The URL ensureExists doesn't have a window associated with it. How it gets the window (probably at the UI/Js level) I have no idea. This is the problem when dragging/dropping to a not yet connected account.
The URL appendMsgFromFile does have a window, but when doing "Run Now" apparently it is an incompatible window. The window is fine when the filter runs normally un-attended (a password prompt occurs).

getTopMostWindow() will go wrong in a number of cases. And, most importantly, violates the assumption in this code that msgWindow == null means non-interactive. Just grabbing any window when we need a password, from the backend, irregardless of where the call comes from, is violating that contract. Instead, the window reference needs to be grabbed in the UI where the click happens, and then passed on properly.

For drag/drop and normal filter transfers, there is no click happening. I guess what I don't understand is why would you not want to prompt for a password when doing the imap operation successfully requires it? Even in the case of biff, if the password is needed to check for new mail, why not prompt the user rather than just skipping getting new mail? Is it because it would hold off other operations maybe? I guess you could say the same thing for filtering if a password prompt pops up and possibly blocking other activities, assuming it does. (FWIW, my experimental code attempts to prevent biff from ever prompting for a password.)

Note also, with the current tb code, a password prompt will occur if needed to complete a non-interactive filter initiated copy/move operation (and possibly block other activities?).

Comment 19

[gene smith]

Attachment: t4.diff

This is purely experimental but has, so far, solved the issues I see, realizing that I am not looking at the complete picture. (Also, please forgive my "goto" sin.)
Feedback/reviews are completely optional.
Thanks!

Comment 20

[Alfred Peters [:infofrommozilla]]

It is not 100% guaranteed, but these are pretty sure dupes of this bug:

• Bug 1673178 - "error copying the message to the sent folder - retry?" sometimes when sending email
• Bug 1606047 - Your draft message was not copied to your drafts folder (Draft) due to network or file access errors.
• Bug 1583346 - Unable to save message to Imap Sent Folder - FIlter error
• Bug 1534077 - "Your message was sent but a copy was not placed in your sent folder (Sent) due to network or file access errors" despite local server on LAN connection
• Bug 1370758 - Can't create sub-folder in IMAP account until after checking for messages.
• Bug 1227467 - In version 38.3 when sending emails the program can not save copy in the Sent folder. The same applies to the Drafts folder and other.

and probably many more.

I can confirm that:
a) saving in Draft, Send etc. folder fails when there is no open IMAP connection to that IMAP account.
b) attachment 9201061 [details] [diff] [review] fixes a)

Comment 21

[Klaus B.]

Wow, Alfred, what a long list of issues that should be fixed one day.

Feedback/reviews are completely optional.

Sadly I'm no expert here, but hopefully the people who are (alta88, BenB, etc.) can move this along. Gene, if you don't set any flags, f?, r? NI? nothing will happen :-(

Comment 23

[Ben Bucksch (:BenB)]

Actually, I had this on my list to answer. Please set flags only when really necessary, and generally only 1-2 persons. For discussions like this, just comments are fine.

For drag/drop and normal filter transfers, there is no click happening.

Thank you for giving specific use cases and situations where this code is exercized.

When I wrote "click", I included drag&drop and keyboard and touch events. Any direct user action. The click was just an example. So, yes, drag&drop counts as interactive event, and should be triggering a dialog where necessary.

When it comes to filters, they are not a direct response to user actions, but can happen on a biff, without any relation to any user action.

I guess what I don't understand is why would you not want to prompt for a password when doing the imap operation successfully requires it?

Thanks for asking that question explicitly. I assumed it was known to everybody, because it's a long-standing convention, but it's good to make the reasons explicit. I'll try to explain the reasons for the current decisions:

1. Thunderbird might be in the background. If you pop up a modal dialog while checking mail, Windows might make the application blink and drag the user's attention. This interrupts the user's work flow (the user might be in a video call, or watch a movie, or doing work that requires concentration), and that is detrimental to usability. Of course it's always easy to demand attention, but like parents teach their children, the timing must be right. You cannot interrupt more important things. That's why we don't want to pop up dialogs out of nowhere, without user interaction.

2. Another case is that the user is using Thunderbird, but doing something else, while the background action like biff happens. For example, the user might be typing an email, we pop up the password dialog, and the user types his email text into the password field. Ooops! Even if that doesn't happen, a dialog out of nowhere is confusing. The user wouldn't know why the dialog popped up.

3. And that can even be dangerous security wise. If we pop up legitimate password dialogs out of nowhere, and the user is supposed to enter is real password, what do you think will happen when somebody tries to phish their password with a mock password dialog? We shouldn't train our users to answer uninitated password prompts. That's why the password prompt should always be for the immediate action that the user initiated himself.

4. The same is true while the user is creating a calendar event, or reading email from a different mail account. If a password prompt pops up, the user is in a different frame of mind and thinks of the account that he's reading right now, not the account that the biff is happening for and the password prompt is for. Sure, the password prompt does mention the account, but we all know that users don't read. The user thinks of the account he's reading, so he enters that password, so he ends up sending his Gmail password to Yahoo. Ooops. That is our fault, because the prompt came out of context.

I hope that shows the rationale why we explicitly do not want password prompts or any prompts whatsoever pop up, unless they are about the immediate action that the user himself initiated right there.

Often, the very same action can be triggered either by the user or by a background task - e.g. a message move can be drag&drop (interactive) or a incoming mail filter (background). So, the way to reflect that - in the TB architecture - is to pass in the window reference to the function (or no window for background tasks, respectively), and then the IMAP code can know whether it should pop up a dialog or not.

Also, by passing the window reference, the code then knows the right window to show the prompt in. The top most window is often right, but not always, and when it's not, it's a bug.

This is how this code is designed: No window reference means background task, and don't throw up dialogs. Having a msgWindow means that the user initiated the action, and you can show prompts where absolutely necessary, and you have a window to root the dialogs on. (If there is code that does something else, that is a bug in that code.)

Comment 24

[gene smith]

(In reply to Alfred Peters from comment #20)

It is not 100% guaranteed, but these are pretty sure dupes of this bug:

• Bug 1673178 - "error copying the message to the sent folder - retry?" sometimes when sending email
• Bug 1606047 - Your draft message was not copied to your drafts folder (Draft) due to network or file access errors.
• Bug 1583346 - Unable to save message to Imap Sent Folder - FIlter error
• Bug 1534077 - "Your message was sent but a copy was not placed in your sent folder (Sent) due to network or file access errors" despite local server on LAN connection
• Bug 1370758 - Can't create sub-folder in IMAP account until after checking for messages.

This one is definitely the same problem as this and I marked it as a dup. However, I can't tell for sure about the others on this list.

• Bug 1227467 - In version 38.3 when sending emails the program can not save copy in the Sent folder. The same applies to the Drafts folder and other.

and probably many more.

I can confirm that:
a) saving in Draft, Send etc. folder fails when there is no open IMAP connection to that IMAP account.
b) attachment 9201061 [details] [diff] [review] fixes a)

Yes, attachment t4.diff will allow the password to be obtained and sent so the connection doesn't drop if there has been no connection during the current tb session with the account you are saving the message to. (It also allows a prompt for the password if it's not stored, even with "windowless URLs". Maybe this shouldn't be allowed?)

Comment 25

[Ben Bucksch (:BenB)]

Comment on attachment 9201061 [details] [diff] [review]
t4.diff

Comment 15 and comment 23 apply also to this patch.

Comment 26

[gene smith]

(In reply to Ben Bucksch (:BenB) from comment #15)

Comment on attachment 9200817 [details] [diff] [review]
t3.diff

As I said: You need to pass in the actual msgWindow where the end user directly initiated the operation, i.e. the user clicked into that exact window a second earlier. He must have contiously initiated the action that is triggering the prompt, in that exact window.

Right now the msgWindow doesn't have a direct relation to where the user clicked. It is embedded in the URL and is somehow set in nsImapService.cpp where the URL (and URI) are created. And another problem is that some URLs like EnsureExists and Subscribe don't have windows passed to them at all in nsImapService.cpp.

If you cannot get that window, you shouldn't show any UI at all.

uses "topmost" window when the msgWindow is null

Yes, that's wrong. The popup might be in the wrong window, depending on situation, timing etc., or might show in cases where it shouldn't show up at all.

I haven't seen this but I haven't looked everywhere. Some windows refuse to show the password prompt, like the filter window with the "Run Now" button. But if you tell it to use top-most, it shows the password when the user clicks "Run Now".

But I see your point in comment 23 about not wanting unsolicited prompts for passwords coming up that could be confusing and/or dangerous.

So I think the easiest solution to this was what I suggested here: Attachment 9200591 [details] [diff]. This still allows a windowless URL (like with biff or EnsureExists) to get the stored password and login to the server. Only if there is also no stored password would it fail and since most users probably just let tb store the password, this reduces the problem. Then to help the more paranoid users who don't store the password, I propose producing an alert describing the problem and a possible solution (i.e., "you need to login to server, and try that again") instead of in-your-face prompt to enter a password now.

Also, I don't think this will affect biff since I've notice that for accounts without a stored password, biff never occurs and nsImapProtocol::GetPassword() is never called. Also, if the msgWindow is null, the loop in GetPassword() that asynchronously tries to prompt for a password just silently fails and falls through (as you would expect since there is no window to display the prompt).

Note also that nsImapProtocol::GetPassword() is only used for authentication where the user types in a text password the old fashion way. It's not used for the more modern auth methods like oauth2 or kerberos. It's called here: https://searchfox.org/comm-central/rev/19d26c50df73a50d1d26fc568f75f614dec9ddf5/mailnews/imap/src/nsImapProtocol.cpp#8467. The changes proposed in Attachment 9200591 [details] [diff] are confined to this function, except for the new alert string.

Comment 27

[Alfred Peters [:infofrommozilla]]

(In reply to gene smith from comment #26)

(In reply to Ben Bucksch (:BenB) from comment #15)

Yes, that's wrong. The popup might be in the wrong window, depending on situation, timing etc., or might show in cases where it shouldn't show up at all.

So I think the easiest solution to this was what I suggested here: Attachment 9200591 [details] [diff]. This still allows a windowless URL (like with biff or EnsureExists) to get the stored password and login to the server. Only if there is also no stored password would it fail and since most users probably just let tb store the password, this reduces the problem.

[Vote +1]

Then to help the more paranoid users who don't store the password,

I understand BenB's concerns and share them in large part. However, these users have willingly chosen to have to enter the password over and over again. Shouldn't we respect that? For those who don't want to be disturbed, a [ ] don't ask me again checkbox could help.

I propose producing an alert describing the problem and a possible solution (i.e., "you need to login to server, and try that again") instead of in-your-face prompt to enter a password now.

That would definitely be better than simply doing nothing. Again with a [ ] don't ask me again checkbox!?

Just my (1(1¢)

Comment 28

[gene smith]

(In reply to Alfred Peters from comment #27)

(In reply to gene smith from comment #26)

(In reply to Ben Bucksch (:BenB) from comment #15)

[Vote +1]

Then to help the more paranoid users who don't store the password,

I understand BenB's concerns and share them in large part. However, these users have willingly chosen to have to enter the password over and over again. Shouldn't we respect that? For those who don't want to be disturbed, a [ ] don't ask me again checkbox could help.

I propose producing an alert describing the problem and a possible solution (i.e., "you need to login to server, and try that again") instead of in-your-face prompt to enter a password now.

That would definitely be better than simply doing nothing. Again with a [ ] don't ask me again checkbox!?

The "alert" message I'm proposing here Attachment 9200591 [details] [diff] is just a box that pops up out of the system tray area and it has no user interaction. It's like what you see when you have new mail. It's completely read-only with no capability for user interaction so don't ask me again is not an option for these type of messages. (You maybe be able to disable alerts from selected apps via O/S controls, I think. But that would stop all tb alerts.)

Comment 29

[gene smith]

(In reply to Ben Bucksch (:BenB) from comment #23)

Ben, I've now gone through your points in detail and address why I think maybe my patch t4.diff above is OK or at least not totally wrong.

For drag/drop and normal filter transfers, there is no click happening.

Thank you for giving specific use cases and situations where this code is exercized.

When I wrote "click", I included drag&drop and keyboard and touch events. Any direct user action. The click was just an example. So, yes, drag&drop counts as interactive event, and should be triggering a dialog where necessary.

That's true and the activities for folder drag/drop or doing copy/move right click dialogs for messages all occur on the top level window it seems. So if there is no msgWindow associated with the driving URL, it seem appropriate to just use the "top most" window. At least it seems to work.

When it comes to filters, they are not a direct response to user actions, but can happen on a biff, without any relation to any user action.

With the current tb code, biff itself doesn't trigger a password prompt at all. If the account is not authenticated, no biff actions occur at all and nsImapProtocol::GetPassword() is never even called during biff.

But if a filter action that, say, moves a message from an authenticated to an unauthenticated account occurs after biff brings in new messages on the authenticated account, a password dialog pops up for the currently unauthenticated account. This is not in response to any user action.

I guess what I don't understand is why would you not want to prompt for a password when doing the imap operation successfully requires it?

Thanks for asking that question explicitly. I assumed it was known to everybody, because it's a long-standing convention, but it's good to make the reasons explicit. I'll try to explain the reasons for the current decisions:

1. Thunderbird might be in the background. If you pop up a modal dialog while checking mail, Windows might make the application blink and drag the user's attention. This interrupts the user's work flow (the user might be in a video call, or watch a movie, or doing work that requires concentration), and that is detrimental to usability. Of course it's always easy to demand attention, but like parents teach their children, the timing must be right. You cannot interrupt more important things. That's why we don't want to pop up dialogs out of nowhere, without user interaction.

What I've observed is that any pop ups that tb produces don't go on top of or block out other window. But I haven't tested this extensively. Also, alert messages like "you've got new mail" or "can't connect to mail server" pop up at any time but go away on their own and generally require no user interaction.

2. Another case is that the user is using Thunderbird, but doing something else, while the background action like biff happens. For example, the user might be typing an email, we pop up the password dialog, and the user types his email text into the password field. Ooops! Even if that doesn't happen, a dialog out of nowhere is confusing. The user wouldn't know why the dialog popped up.

At least for biff, it never pops up a dialog to enter a password because biff (scan all account for new mail based on a timer) only occurs for already authenticated accounts. That seems to be a designed in feature independent of the biff-blocking code in nsImapProtocol::GetPassword().
If you click the "get mail" button, you do get prompts for passwords on unauthenticated accounts. But that's in response to a user activity which is OK.
But you will get a "dialog out of nowhere" if the filter actions requires a copy to an unauthenticated account. This is with the current tb code.

3. And that can even be dangerous security wise. If we pop up legitimate password dialogs out of nowhere, and the user is supposed to enter is real password, what do you think will happen when somebody tries to phish their password with a mock password dialog? We shouldn't train our users to answer uninitated password prompts. That's why the password prompt should always be for the immediate action that the user initiated himself.

That seems reasonable. However, the filter that copies to an unauthenticated account example violates this since it puts up a password dialog out of the blue. But I don't think this is a huge problem because the tb password dialog tends to not be aggressive and stays below any non-tb window that is in focus, so it's unlikely you will start typing accidentally into the password dialog.

4. The same is true while the user is creating a calendar event, or reading email from a different mail account. If a password prompt pops up, the user is in a different frame of mind and thinks of the account that he's reading right now, not the account that the biff is happening for and the password prompt is for. Sure, the password prompt does mention the account, but we all know that users don't read. The user thinks of the account he's reading, so he enters that password, so he ends up sending his Gmail password to Yahoo. Ooops. That is our fault, because the prompt came out of context.

This sounds like good reasons not to put up spontaneous password prompts. However as I mentioned:

• Biff never password prompts (even with my proposed changes in t4.diff)
• Filter actions potentially prompt for password

I hope that shows the rationale why we explicitly do not want password prompts or any prompts whatsoever pop up, unless they are about the immediate action that the user himself initiated right there.

Often, the very same action can be triggered either by the user or by a background task - e.g. a message move can be drag&drop (interactive) or a incoming mail filter (background). So, the way to reflect that - in the TB architecture - is to pass in the window reference to the function (or no window for background tasks, respectively), and then the IMAP code can know whether it should pop up a dialog or not.

Also, by passing the window reference, the code then knows the right window to show the prompt in. The top most window is often right, but not always, and when it's not, it's a bug.

The only example I've seen is when the filter "Run Now" dialog tries to produce a password dialog and it rejects the filter window since there is no interface. I then tell it to use "top most" window and it works fine. So there's a bug in the current code that my t4.diff actually sort-of fixes.
Do you know any examples where the "top most" won't work? (Maybe some of the unit tests will fail due to this, I haven't tried running them with my t4.diff patch.)

This is how this code is designed: No window reference means background task, and don't throw up dialogs. Having a msgWindow means that the user initiated the action, and you can show prompts where absolutely necessary, and you have a window to root the dialogs on. (If there is code that does something else, that is a bug in that code.)

I know two URLs that need a window but don't have one and causes the problem. They are:

• ensureexists
• discoverallandsubscribedboxes

I've mention the 1st one several times. The 2nd one is the case where you try to subscribe to folders on an not yet authenticated account. The subscribe dialog comes up empty with no indication of an error. t4.diff fixes this too by forcing the use of "top most" window when there is no msgWindow parameter.
I haven't been able to figure out how to provide these with a msgWindow. This get up into "javascript land" where I'm not very comfortable.

Comment 30

[Ben Bucksch (:BenB)]

if a filter action that, say, moves a message from an authenticated to an unauthenticated account occurs after biff brings in new messages on the authenticated account, a password dialog pops up for the currently unauthenticated account

As I've mentioned, it's very possible that there are bugs in the existing code that violate the assumptions and expected behavior. These are bugs and should not be extended (by adding more instances of that), but fixed.

Comment 23 explains in detail how and why that matters to end users.

alert messages like "you've got new mail" or "can't connect to mail server" pop up at any time but go away on their own and generally require no user interaction.

You mean the Windows/Linux/Mac notifications, not popup windows within Thunderbird, right? That's fine, because they are intended for background notifications, i.e. actions that the application initiated and not the end user. I agree, that's an appropriate place to show such errors.

As far as I remember, AlertUserEventUsingName() is not the same as these OS notifications, but IIRC can get converted back into an error for the caller. That may or may not do what we want here. You'd need to go back the call stack, for all possible callers - there are several scenarios. If you can show what this does in all the different cases, and that is the right thing in every case, I'd be happy to revert my r- into a r+ for that patch.

Comment 31

[Klaus B.]

So we're moving forward here? Is comment #12 clear enough to continue? We could fix a handful of bugs (see comment #20) if we can fix this.

Comment 32

[gene smith]

(In reply to Klaus B. from comment #31)

So we're moving forward here? Is comment #12 clear enough to continue? We could fix a handful of bugs (see comment #20) if we can fix this.

No we're not. I've kind of given up on this one because everything I suggest and that fixes the problem seems to violate some de-facto or explicit design rule. And I have no idea how to cause the URLs that don't have a window that need one to have one. So I'll leave this to the powers that be to decide what to do rather than arguing my points any further. Also, removing my name, for now, as assignee.

Comment 33

[gene smith]

You mean the Windows/Linux/Mac notifications, not popup windows within Thunderbird, right? That's fine, because they are intended for background notifications, i.e. actions that the application initiated and not the end user. I agree, that's an appropriate place to show such errors.

As far as I remember, AlertUserEventUsingName() is not the same as these OS notifications, but IIRC can get converted back into an error for the caller. That may or may not do what we want here. You'd need to go back the call stack, for all possible callers - there are several scenarios. If you can show what this does in all the different cases, and that is the right thing in every case, I'd be happy to revert my r- into a r+ for that patch.

Ok, sorry I misunderstood this comment when I first read it about 2 weeks ago. I think you are saying you "almost approve" of my 2nd diff patch above except you are not sure about the call to AlertUserEventUsingName().
I was not able to get the notification to appear as an OS pop-up. It only appeared in the Activity Manager. I did some deeper debugging. The message string only goes to activity manager because the associated URL has no msgWindow.

What happens is here
https://searchfox.org/comm-central/rev/e85e0354ecc1a5343a501e8c7a61eb935c1b8e81/mailnews/base/src/nsMsgMailSession.cpp#183
OnAlert() is called as defined here:
https://searchfox.org/comm-central/rev/8b2121df11b0c25fcd4d594247c411e324e81072/mail/components/activity/modules/alertHook.jsm#45
It looks like OnAlert() sends the warning message to the activity manager and, since the msgWindow is null for the URL, OnAlert() is exited without actually triggering the alert.
Where OnAlert() is called from the .cpp code in AlertUser() at the first searchfox link, bool listenerNotified is set true so AlertUser() exits resulting in just the message appearing in activity mgr. But even if listenerNotified wasn't true, again nothing more would happen in AlertUser() because msgWindow is null. (FWIW, I temporarily changed to code in AlertUser() to ignore msgWindow and allowed to it use the "default" dialog and a modal window appeared with my message in it with an OK button.)

This allows tb to trigger the OS notification by ignoring the null msgWindow in OnAlert():

diff --git a/mail/components/activity/modules/alertHook.jsm b/mail/components/activity/modules/alertHook.jsm
--- a/mail/components/activity/modules/alertHook.jsm
+++ b/mail/components/activity/modules/alertHook.jsm
@@ -55,17 +55,18 @@ var alertHook = {
       warning.groupingStyle = Ci.nsIActivity.GROUPING_STYLE_STANDALONE;
     }
 
     this.activityMgr.addActivity(warning);
 
     // If we have a message window in the url, then show a warning prompt,
     // just like the modal code used to. Otherwise, don't.
     try {
-      if (!aUrl || !aUrl.msgWindow) {
+      if (!aUrl) {
         return true;
       }
     } catch (ex) {
       // nsIMsgMailNewsUrl.msgWindow will throw on a null pointer, so that's
       // what we're handling here.
       if (
         ex instanceof Ci.nsIException &&
         ex.result == Cr.NS_ERROR_INVALID_POINTER

I don't know if this one line change causes other problems but with this change my new message now appears as an OS notification instead of only in Activity Mgr.

Comment 34

[Ben Bucksch (:BenB)]

Comment on attachment 9200591 [details] [diff] [review]
AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

This is the patch I mentioned at the end of in comment 30.

Comment 35

[gene smith]

Attachment: v2--AlertUserEventUsingName - Possible fix for GetPassword() due to null msgWindow

Here's another attempt at a patch.
This ignores the msgWindow being null thus allowing the OS Alert to occur instead of the message only appearing in the Activity Manager. I haven't seen any adverse effects from this.
Also, the alert message now tells the user to restart TB since, without a restart the failed URL locks out the next copy attempt to the destination folder, even after successfully logging into the server. I've seen this problem before when copying folders and I don't see a way to fix it without deep understanding of very obscure code.

Comment 36

[Klaus B.]

Gene, you're going to ask someone for feedback or review here? Without it, the patch will just rot away :-(

Comment 37

[Ben Bucksch (:BenB)]

Regarding the last sentence from comment 30: Comment 33 is a partial answer, but we need to cover all possible code paths.

Comment 38

[gene smith]

Attachment: t6.diff

My proposed change in the previous diff caused a regression of bug 577343 which added the restriction that a null msgWindow in the URL meant don't show the alert pop-up. What that fixed was the case where TB "biffs" for new mail every X minutes and the server fails to respond. Previous to the fix in Bug 577343, an alert would pop-up every X minutes annoying the user. With the fix the alert is only recorded in Activity Manager. So my previous diff allowed the message to "can't connect to server" to pop-up every X minutes again.

(Seems like the user might want to know this, especially if it's continuous so they can report the server problem or turn off biff for the account. However, there were a lot of complaints, so I guess the fix is OK. Also, back in time, I guess before the pop-up alert was invented, the message was always a "modal" message that required user intervention to dismiss which was even more annoying per ancient bug reports.)

So this proposed fix now takes a somewhat different approach. Now when I trigger the alert due to the account not being authenticated, if there is null msgWindow with the URL (as is the case for this bug) I now set a new mailnews URL attribute that I defined call "allowAlertWithoutMsgWindow" true, but it keeps the default of false for all other URLs that arrive at onAlert() in alertHook.jsm.

Since there are 100s of code paths leading to onAlert(), this eliminates the need to check them all since the functionality of onAlert() is only changed for the very special case when the URL attribute allowAlertWithMsgWindow is true and I think it can be mostly verified by code inspection.

I need someone who knows JS try/catch/throw stuff who can look at the changes I made and see if it looks right. I left in some dump()s and it appears to work OK, but there may be a better way to arrange the code.

The idea is if parameter aUrl is null, then just get out with no alert. If aUrl.msgWindow is null AND aUrl.allowAlertWithMsgWindow is false, also get out with no alert. But if aUrl.msgWindow is not null OR aUrl.allowAlertWithMsgWindow is true then show the alert.

Comment 39

[Klaus B.]

Comment on attachment 9208534 [details] [diff] [review]
t6.diff

Hmm, I'm a few days late, and I'm still the first person to comment :-(

Can you explain the reason for the missing authentication: The password is not stored, and he user hasn't entered it yet, yes? Having to restart TB isn't a good thing, is it? Why can't they select the target folder now and then retry the copy without restarting.

There are a few issues with the patch. Please don't add stuff to nsIMsgMailNewsUrl. That's the "base" class. If you need this, add it to nsImapUrl:
https://searchfox.org/comm-central/rev/e7c25ffdd125799ce4a6c4be4405b6c1d9e8739a/mailnews/imap/src/nsImapUrl.h#17

I'm happy to point out more nits later, like that comments should be full English sentences and start with a capital letter and end with a full stop. TB code is also not the place to try out "experimental" spelling, like "try-ing" ;-)

I'll let the others speak to the general approach.

Comment 40

[alta88]

Comment on attachment 9208534 [details] [diff] [review]
t6.diff

There should not be any restarting, that's a rather poor application.

Upon drop, can't you just check if the target folder's server is logged in, and if not then simulate a login, which will put up a password dialog (if there isn't a password saved), and then continue with the copy? For this specific use case.

Big picture: it is far better to err on the side of asking for a password (getting the topmost window if msgWindow is null) than getting stuck on low probability or questionable and hypothetical unattended concerns if that is - as it seems - very hard to figure out for the many callbacks here. For every user who is annoyed at being interrupted by an unattended error in the middle of composing a message, there will be a user who wants to know their filter failed on a biff, at all times.

Comment 41

[Ben Bucksch (:BenB)]

Upon drop, can't you just check if the target folder's server is logged in, and if not then simulate a login, which will put up a password dialog
(if there isn't a password saved), and then continue with the copy? For this specific use case.

That's a nice idea.

Comment 42

[gene smith]

Upon drop, can't you just check if the target folder's server is logged in, and if not then simulate a login, which will put up a password dialog (if there isn't a password saved), and then continue with the copy? For this specific use case.

I haven't looked at the code in a while but I doubt it will be obvious (to me) how to "check if ... logged in" or "simulate a login". Any specific advice on how to to this would be appreciated.

Comment 43

[alta88]

In Bug 1695703c#4 I mentioned how to check for imap userAuthenticated state. At that point, you could show a dialog "Select the target folder to log in", which would be less than spiffy but better than silent failure. I don't know offhand the exact call to make that initiates this login for imap.

Comment 44

[gene smith]

Attachment: Bug1690093-Proposed-fix.patch

After a taking a break from the bug and looking at it again fresh I think I have found a simple and acceptable way to fix the problem. As I mentioned several times above, the root problem is that when the "ensureExists" URL occurs, it has no "window" associated with it unlike, say, the "select" URL. So when a password prompt is needed, nothing occurs since a URL without a window is assumed to be an unattended operation (such as biff and filter action) where a window popping up would be unexpected and possibly confusing or annoying.

So the proposed solution is to just associate the "top most" window to the ensureExists URL before it is passed on to IMAP protocol. Since the ensureExist URL never occurs unattended but always in response to direct user activity, it avoids the potential problems with my previous and similar proposed solution (t4.diff).

So with this patch, if the not yet connected destination account has a stored password, it is used and the copy occurs as expected. If it has no stored password, the prompt for password occurs first. So the problem is solved without requiring a special user alert and action other than entering a password.

For good measure, I have also include in the patch the same fix for the URL that causes the subscription dialog to occur. It had the same problem as folder drag/drop in that an attempt to show the subscription dialog on an account that is not yet connected comes up blank. With this patch, it comes up blank at first if there is no stored password but then a password prompt occurs and then the folders are listed in the dialog as expected.

Comment 45

[Ben Campbell]

Comment on attachment 9211681 [details] [diff] [review]
Bug1690093-Proposed-fix.patch

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

r+, conditional on `mailSession->GetTopmostMsgWindow()` being considered acceptable. So I think a second opinion is in order, from someone more familiar with this code... BenB, perhaps?

Comment 46

[Klaus B.]

Comment on attachment 9211681 [details] [diff] [review]
Bug1690093-Proposed-fix.patch

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

A few pedantic nits ;-)

::: mailnews/imap/src/nsImapService.cpp
@@ +1765,5 @@
> +          mailnewsurl->SetMsgWindow(msgWindow);
> +      }
> +      NS_ASSERTION(
> +          NS_SUCCEEDED(rvWindow) && msgWindow,
> +          "No top most window for password: URL discoverallandsubscribedboxes");

What is this string? discoverallandsubscribedboxes

@@ +2318,5 @@
> +        if (NS_SUCCEEDED(rvWindow) && msgWindow)
> +          mailnewsurl->SetMsgWindow(msgWindow);
> +      }
> +      NS_ASSERTION(NS_SUCCEEDED(rvWindow) && msgWindow,
> +                   "No top most window for password. URL ensureExists");

And ensureExists?

Comment 47

[Ben Campbell]

Just to expand on comment 45 after a little more thinking and digging, and in case using GetTopMostWindow() isn't considered appropriate:

Notionally, it seems that an interactive operation should always (by definition) have the window in question available at the time it is performed, and the issue here is that that window pointer isn't being passed down far enough to the code that needs it.

The nsImapService::EnsureFolderExists() case looks fixable, by adding the window as an extra parameter of EnsureFolderExists().
It's only called from two places:
https://searchfox.org/comm-central/search?q=EnsureFolderExists&path=

The first place it's called is from nsImapFolderCopyState::StartNextCopy(), which looks like it'd be the path involved here, where the user has dragged a folder between servers. In this case, the nsImapFolderCopyState object already has a pointer to the window, so it could easily pass it in to EnsureFolderExists(), avoiding the need to call GetTopmostMsgWindow().

The second place is in nsImapMailFolder::CreateStorageIfMissing().
This one is a little more tricky, as CreateStorageIfMissing() is called from all over the place, so it'd be way harder to propagate a window pointer... but if it's not part of the critical path on this bug, maybe it's OK just passing a null window down into EnsureFolderExists()?

nsImapService::DiscoverChildren() looks simpler - it's only ever called from one place:
https://searchfox.org/comm-central/search?q=nsImapMailFolder%3A%3APerformExpand

And PerformExpand explicitly has a window pointer available, so DiscoverChildren could just take it as a param.

Comment 48

[gene smith]

The first place it's called is from nsImapFolderCopyState::StartNextCopy(), which looks like it'd be the path involved here, where the user has dragged a folder between servers. In this case, the nsImapFolderCopyState object already has a pointer to the window, so it could easily pass it in to EnsureFolderExists(), avoiding the need to call GetTopmostMsgWindow().

Yes, the window is right there and I passed it to EnsureFolderExists() and it works fine when dragging/dropping a folder to another server. So don't have to use "top most" in this case.

The second place is in nsImapMailFolder::CreateStorageIfMissing().
This one is a little more tricky, as CreateStorageIfMissing() is called from all over the place, so it'd be way harder to propagate a window pointer... but if it's not part of the critical path on this bug, maybe it's OK just passing a null window down into EnsureFolderExists()?

This seems to be used when you save a message as an archive or if you save a message as a template to a not yet logged-in server. I too don't see a way to get a window here other that defaulting to the "top most" window. Since there is no explicit window here and if I pass "nullptr" as the window to EnsureFolderExists() you have the same problem when you save an archive or template: nothing happens silently if the folder you are copying to doesn't exist. Maybe in this case, the "top most" window should still be used?

nsImapService::DiscoverChildren() looks simpler - it's only ever called from one place:
https://searchfox.org/comm-central/search?q=nsImapMailFolder%3A%3APerformExpand

And PerformExpand explicitly has a window pointer available, so DiscoverChildren could just take it as a param.

Not sure what this is referring to. I think this is to fix the subscribe problem?
To fix empty subscribe dialog, I passed the aMsgWindow parameter (previously unused) here:
https://searchfox.org/comm-central/rev/c96bef7839f29e11313cad9d7a3cb15f83034f90/mailnews/imap/src/nsImapService.cpp#2863
to DiscoverAllAndSubscribedFolders() a few lines down and set aMsgWindow instead of "top most" window in the URL. This fixes the empty subscribe dialog without using "top most".

So yes, this does fix most of the problems. However, the archive and template save remains unfixed unless "top most" still used in that case.

Comment 49

[gene smith]

(In reply to Klaus B. from comment #46)

What is this string? discoverallandsubscribedboxes
And ensureExists?

These are URL literal strings I'm referring to, e.g.,
https://searchfox.org/comm-central/rev/c96bef7839f29e11313cad9d7a3cb15f83034f90/mailnews/imap/src/nsImapService.cpp#1749

Comment 50

[Ben Bucksch (:BenB)]

@benc in comment 47:
Thank you! Exactly that's the kind of information I needed.
Very much appreciated.

@gene in comment 48:

Yes, the window is right there and I passed it to EnsureFolderExists()

In your patch attached here, you don't, and you still use GetTopmostWindow().
Or are you talking about a patch work-in-progess not yet attached?

@Gene, can you please fix it exactly in the way BenC was saying in comment 47? This is what I was asking you for.

To summarize comment 47:

1. EnsureFolderExists(): Add parameter, and use existing (!) window reference. Do not use GetTopmostWindow().
2. Pass window reference from PerformExpand() to DiscoverChildren(). Do not use GetTopmostWindow().
3. Ignore the other cases not relevant to this bug.

Comment 51

[gene smith]

Attachment: t8.diff

Ben B wrote:

In your patch attached here, you don't, and you still use GetTopmostWindow().
Or are you talking about a patch work-in-progess not yet attached?

Yes, I just made the changes and tested them and hadn't attached the diff yet.

@Gene, can you please fix it exactly in the way BenC was saying in comment 47? This is what I was asking you for.

To summarize comment 47:

1. EnsureFolderExists(): Add parameter, and use existing (!) window reference. Do not use GetTopmostWindow().

Ok, at the place where the copy occurs I passed in m_msgWindow exiting variable. At the place where called from CreateStorageIfMissing() I pass in a nullptr since no window is available.

2. Pass window reference from PerformExpand() to DiscoverChildren(). Do not use GetTopmostWindow().

Ok, this is what I don't understand. DiscoverChildren() is not called for this bug. I put a printf in it and don't see it called at all when doing various related things.
To fix this bug I had to pass the msgWindow to DiscoverAllAndSubscribedFolders() from where it's called in GetListOfFoldersOnServer().

3. Ignore the other cases not relevant to this bug.

Right. This doesn't fix saving archive or templates to another not yet connected server. Probably a pretty unusual thing to do but if tried it will still be a no-op. (This is due to the nullptr in item 1 above.)

Comment 52

[Ben Bucksch (:BenB)]

Comment on attachment 9212935 [details] [diff] [review]
t8.diff

Much better!

Comment 53

[gene smith]

Attachment: Bug1690093-Proposed-fix-v4.patch

I'll ask Ben B to approve this since I assume the usual reviewer, Ben C, is busy since no response to my feedback request after a couple weeks.
This is the same as t8.diff but with clang formatting applied except for the .idl file.

Comment 54

[Ben Bucksch (:BenB)]

Comment on attachment 9216252 [details] [diff] [review]
Bug1690093-Proposed-fix-v4.patch

I can only review from the code perspective: Yes, this makes sense and is good design.
This is the right kind of changes. It makes sense to me. The patch looks good.

r=BenB

I have not tested the patch and consequently I don't know whether it fixes the bug, or breaks something. It is the burden of the contributor (gene) to test it carefully.

I would nonethless prefer, if BenC could give it a third pair of eyes.

Comment 55

[gene smith]

Comment on attachment 9216252 [details] [diff] [review]
Bug1690093-Proposed-fix-v4.patch

Thanks Ben B.
Added back in Ben C. for review.

Comment 56

[Ben Bucksch (:BenB)]

Comment on attachment 9216252 [details] [diff] [review]
Bug1690093-Proposed-fix-v4.patch

(You've accidentally removed my review. Restoring. Tip for next time: You want to use the "addl. review" field at the bottom.)

Comment 57

[Ben Campbell]

Comment on attachment 9216252 [details] [diff] [review]
Bug1690093-Proposed-fix-v4.patch

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

Sorry about the delay.
Looks good to me. xpcshell-tests pass for me locally and I kicked off a try run, which doesn't seem to have broken anything: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=115997f6300785bb4fbbfd39908985081fb1a9d1

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f6b7e4b43ce
Allow imap APPEND of folder to not yet authenticated account. r=BenB

Comment 59

[Ben Bucksch (:BenB)]

Thanks everybody!
I would like to particularly thank BenC for comment 47, which helped the proper resolution of this bug.