1360117 - Improve detection of servers which require additional IMAP select

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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

In bug 1231592 we implemented a preference to send an additional IMAP select to the server to support somewhat misbehaving servers like those used by Charter/Spectrum running InterMail or Openwave software.

There were complaints that this preference was too obscure and that we should detect the condition.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of the new configuration option at work

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: WIP: adaptive-select.patch

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8862293 [details]
Screenshot of the new configuration option at work

Thanks again, Gene, for providing this. This should fulfil Ben's requirement.

Personally, I think we used too much space on the panel for this feature. Could that be made more compact, something like:

Send IMAP select to server when checking for new mail:
[x] automatic (recommended) [ ] always [ ] never.

Or some sort of pulldown.

Richard, can you please make a suggestion here.

Comment 4

[Jorg K (CEST = GMT+2)]

What about that InterMail server software, you're not testing that.

Comment 5

[Richard Marti (:Paenglab)]

When we default to automatic, do we need a UI? In the very seldom case the user has to change it, he can do this through about:config. Like this we need no text which isn't clear for everyone.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8862295 [details] [diff] [review]
WIP: adaptive-select.patch

I think Magnus was not 100% in favour of server specific tweaks, so let's get his feedback here.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #5)
> When we default to automatic, do we need a UI? In the very seldom case the
> user has to change it, he can do this through about:config. Like this we
> need no text which isn't clear for everyone.
Well, the about:config setting is difficult since you need to set it for a specific server:
Check: https://www-dev.allizom.org/en-US/thunderbird/54.0beta/releasenotes/
  Better support for Charter/Spectrum IMAP: Set preference
  mail.server.default.forceSelect and mail.server.server[X].forceSelect
  to true to avoid not reliably receiving mail

The question is: How good is the detection and what happens if the server software ever changes and we don't need the feature any more.

Comment 8

[Richard Marti (:Paenglab)]

Then it would still work but with more traffic than needed, or not?

Comment 9

[Jorg K (CEST = GMT+2)]

Yes. I still think some UI for this would be good and I'd like to reduce it to two lines as I said in comment #3.

Comment 10

[Richard Marti (:Paenglab)]

Yeah, two lines is better. When more text would be needed for every entry then a menulist would be good.

Comment 11

[Ben Bucksch (:BenB)]

> When we default to automatic, do we need a UI? In the very seldom case the user has to change it, he can do this through about:config.

+1

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8862295 [details] [diff] [review]
WIP: adaptive-select.patch

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

I agree we can remove the UI and just auto-detect instead.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +7772,5 @@
>  }
>  
> +bool nsImapProtocol::IsExtraSelectNeeded()
> +{
> +  // This is a workaround for openwave servers that require an extra imap SELECT

probably a good idea to mention the bug number here, for future reference

Comment 13

[gene smith]

(In reply to Jorg K (GMT+2) from comment #4)
> What about that InterMail server software, you're not testing that.

Charter returns this unsolicited after TLS login. It is never requested by tb:
* OK IMAP4 server (InterMail vM.9.00.023.01 201-2473-194) ready Tue, 25 Apr 2017 18:46:35 -0500 (CDT)

I am not using this response and this is the only response from charter containing the string "InterMail". The response to tb ID command contains the "Email Mx" and "Openwave Messaging" string that I check for. I ignore the version and support strings in charter's ID response.

According to this, Email Mx is the "new" name for Intermail:
https://smxemail.com/how-to-build-a-mail-hosting-platform.html

Comment 14

[gene smith]

(In reply to Magnus Melin from comment #12)
> probably a good idea to mention the bug number here, for future reference

I was told at some point a while back *not* to put bug numbers into the code.

Comment 15

[Jorg K (CEST = GMT+2)]

I'm sorry about the confusion. In general we don't put bug numbers into the code since the code would be full of them.

However, in particular cases we do put them, especially if something is not so obvious and needs a reference. Also, different individuals make different judgements. I'm not sure I need a bug number here since DXR's blame facility will clearly show in which bug that code was added, but Magnus seems to prefer a quick reference, which is OK as well.

On some bugs, you get two reviewers involved, and then you're in trouble if they have different taste.

For example here: I'm in favour of some option, in the GUI or not, that will allow the user to correct/disable the auto-detect since it's easier to tell someone to set a preference than to ship a new version of the software with the auto-detect corrected. So I guess it's OK to remove the somewhat confusion UI again, but I'd really like to maintain some preferences that can be inspected or set, so people can work out what's going on without having to use a debugger.

Comment 16

[gene smith]

Based on the above comments I have modified my prototype.

I have tried keeping the configuration for the added imap advanced gui items hidden". This allows mail.server.serverXX.forceSelect to appear for each configured account in the config editor with a default value of 0 that can be set to 0, 1 or 2 as previously described. If values other than these are set, the default value 0 is now quietly set.

Unfortunately this has a problem if the mail.server.serverXX.forceSelect value is "reset" in the config editor. This causes the it to become a "string" type and it is not possible to set it back to an integer without un-hiding the UI and setting it again there. (This can also be seen, for example, by "resetting" the mail.server.serverXX.max_cached_connection in config editor.)

Also, when a new account is added, the mail.server.serverXX.forceSelect value, which is actually default 0, does not appear in the config editor at all. It only appears after it is set to something else in by the un-hidden UI.

I have changed to the suggested compact and horizontally placed radio button selectors as shown in the following attachment adaptive-select-compact.png. (Hiding/unhiding just requires setting attribute hidden="true"/"false" at two places in am-server-advanced.xul.)

adaptive-select-compact.patch show the changes to implement the changes. It also ensures that a value for m_forceSelectValue other than 0..2 defaults to 0 when setting bool m_forceSelect.

Hopefully, something like this is OK. But maybe you are wanting to just make forceSelect global and common for all accounts? If that is the case, there is no problem setting forceSelect only in config editor and removing entirely the UI for setting it. But if you want it specific to each account, it appears that the UI method is required (unless I am missing something about how all this works).

Comment 17

[gene smith]

Attachment: adaptive-select-compact.png

Comment 18

[gene smith]

Attachment: adaptive-select-compact.patch

Comment 19

[Jorg K (CEST = GMT+2)]

Well, as stated previously, I'm in favour of a solution *with* UI. As I said various times, the panel already has options that the normal user doesn't understand (directory/namespace) and which can do more damage than this new option.

Of course there's always the possibility to hide this option with yet another preference, like we do for mailnews.display.show_all_body_parts_menu, so mailnews.imap.show_imap_select ;-)

Or could mail.server.serverXX.forceSelect be a string ("auto", "yes", "no") to avoid the problem discussed in comment #16? I could imagine the detection also setting the value to "auto-yes" and "auto-no" do reflect the detection result. Then if any complaints arise, the user could be told to search for "forceSelect" and report/correct the value. With no strings, we can also ship this in TB 52 ESR.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to gene smith from comment #14)
> (In reply to Magnus Melin from comment #12)
> > probably a good idea to mention the bug number here, for future reference
> 
> I was told at some point a while back *not* to put bug numbers into the code.

It's true we don't generally do that, but this is a very specific bug fix that hopefully sometime in the future could be removed. Linking such behaviors to a bug number is easier for whoever is going to look at that code and figure out if it's still needed. 

The newer UI looks good in it self, it's just that it's still useless as someone not reading this bug can't really figure out what to choose. 

Yes, it's hard to set per server prefs without UI, but if you're affected, chances are you only have one account, so you could set the mail.server.default.forceSelect pref and will set it globally for you without downsides. And even if you have more than one account, it's just a slight performance issue, so I would not worry about it.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #20)
> Yes, it's hard to set per server prefs without UI, but if you're affected,
> chances are you only have one account, ...
Let's not make any assumptions here that we can't prove. Chances are that the person has an account from his local ISP, Charter, *and* a Gmail account. So what's wrong with my suggestion in the last paragraph of comment #19?

Comment 22

[Magnus Melin [:mkmelin]]

The other options there are also difficult, but they are standard IMAP concepts after all.

What problem would you solve by having UI, when auto-detect would be the default?

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #22)
> What problem would you solve by having UI, when auto-detect would be the
> default?
I can correct the auto-detect or see it at work. If we invest 15 lines of code now, I can solve foreseeable support hassle later. In case there's anything wrong, I'll tell the user to enter forceSelect into about:config and check/change the values that come up. There's no harm done having this.

Comment 24

[Magnus Melin [:mkmelin]]

Sorry, I still don't see what problem it would solve - why would the auto detection be wrong? (If so, that's a bug and not likely solvable by UI.) It's also almost as easy to enter the default pref in about:config for testing out the two scenarios.

Comment 25

[Jorg K (CEST = GMT+2)]

Which "default pref"? We have a per-server pref here.

Surely the auto-detection wouldn't be wrong, but other servers might require the same treatment. So instead of shipping a new binary we tell the user to set a pref. Or is the server changes its response, we can still switch the thing on.

That said, I'm not really a friend of this type of auto-detection since this doomed to fail when that server behaviour changes. 

With "auto", "auto-yes", "auto-no", "yes", "no" the pref is never at its default "auto", so a search will always find it.

IMAP has a huge lot of preferences you can tweak:

pref("mail.imap.chunk_size",                65536);
pref("mail.imap.min_chunk_size_threshold",  98304);
pref("mail.imap.chunk_fast",                2);
pref("mail.imap.chunk_ideal",               4);
pref("mail.imap.chunk_add",                 8192);
pref("mail.imap.hide_other_users",          false);
pref("mail.imap.hide_unused_namespaces",    true);
pref("mail.imap.auto_unsubscribe_from_noselect_folders",    true);
pref("mail.imap.mime_parts_on_demand",      true);
pref("mail.imap.mime_parts_on_demand_threshold", 30000);
pref("mail.imap.use_literal_plus",          true);
pref("mail.imap.expunge_after_delete",      false);
pref("mail.imap.check_deleted_before_expunge", false);
pref("mail.imap.expunge_option",            0);
pref("mail.imap.expunge_threshold_number",  20);
pref("mail.imap.hdr_chunk_size", 200);

and there are even ones not listed here, like mail.imap.noop_check_count:
https://dxr.mozilla.org/comm-central/search?q=mail.imap.noop_check_count&redirect=false

So what's wrong with having a per-server "force select" with five possible values?

Besides, do I have to think of a concrete application for having an extra tool in my toolbox?

Comment 26

[Andrei Hajdukewycz [:sancus]]

Controversial opinion: For prefs that you 'set and forget' one time, if ever, the Config Editor is actually more user friendly than the regular UI, because it has search and Thunderbird already has legitimately insane pref UI clutter. :)

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks for the opinion. I think we all agree that we're going to drop the UI but we're still discussing what the pref(s) should be.

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #25)
> Which "default pref"? We have a per-server pref here.

The mail.server.default.forceSelect pref. If nobody specifically sets a given pref for a given server, that would apply to all servers you have.

> Surely the auto-detection wouldn't be wrong, but other servers might require
> the same treatment. So instead of shipping a new binary we tell the user to
> set a pref.

Well the can just set the pref above.
 
> So what's wrong with having a per-server "force select" with five possible
> values?

Then you can't use the approach I wrote above.

> Besides, do I have to think of a concrete application for having an extra
> tool in my toolbox?

If it's a tool only a tiny percentage will/should use, that's the first thing you should think of.

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #28)
> The mail.server.default.forceSelect pref. If nobody specifically sets a
> given pref for a given server, that would apply to all servers you have.
Sure. Integer or String? Gene mentioned some problem with Integer, comment #16.
That's why I suggested String in the first place: "auto", "yes", "no".
And while we're at it, let the program change "auto" to "auto-yes/no" to show the detection result. That's all.

Comment 30

[gene smith]

Jorg, just been listening and never answered your question. I don't think making the server specific pref a string will help with the problem which is that if the item is "reset" back to default, it essentially disappears from the list. Also, when a new account is setup, the server specific items don't appear at all. They only appear after they are set to non-default in a UI (or maybe there is another way?).
I tried to make some server specific string variables but never could get it to work, so not 100% sure. Not sure what kind of strings these are since I don't know much about the strings: ascii, cstring nsCString, ACString etc. If this is what is wanted I may need some guidance.

Also, I don't really understand what you are proposing with "auto-yes", "auto-no" etc. 

With Magnus's proposal, I dont' think we need a server specific default like mail.server.default.forceSelect=0. Can't it just be a global pref like mail.server.forceSelect? All accounts would just check this common variable before checking and acting on the ID response.

Regarding the "tiny percent" that need this. Probably no market research but maybe it is because charter users (any maybe others) tried tb and it didn't work quite right and then tried client B and it did so just moved on.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: adaptive-select-compact.patch pref as string

I hope this patch answers the questions.

When the preference was "auto", set it to "auto-yes/no" after detection. Missing from the code which only compiles, not tested, 12:10 AM here now.

Also, of course, you want to lose the UI and the strings in the .xul and .dtd files.

String processing in Mozilla can be a pain, I know.

As for resetting the pref: Well, I hope it stays a string. And if the user resets it to "auto", the next run will set it to "auto-yes/no", so it reappears.

Comment 32

[gene smith]

OK, I tried to reset a server specific string that defaults to "SpamAssasin" to default and I can set it back to a non-default via config editor. However, server specific string don't appear in config editor until that are set to a non-default in UI. In this case I first had to change "SpamAssasin" to "DSPAM". Then in the config editor I could "reset" it and then set it back to "SpamAssasin".

Comment 33

[gene smith]

Thank! I tried something like that yesterday but it wouldn't even compile. I will see if I can get it to work. Get some sleep or at least take a nap! :).

Comment 34

[gene smith]

Attachment: forcesel.png

Got the SetForceSelect() working and config editor is now working as you describe. I was able to set all the states on all 5 accounts and verified that force select works as we expect. No problems with reset. The attachment shows how it looks and the patch (next attachment) shows the changes. For changes in config editor to take effect a tb restart is needed. (With just the config editor and no UI it is not obvious which serverXX goes with which account. Have to look at string under mail.server.serverXX to see which account it is.)
To call SetForceSelect() I had to get a pointer to the server like where GetForceSelect() is called. I made the pointer a new member variable. There may be a better way to do this but I couldn't get anything else to work.
The next attachment is the proposed patch showing what I (and you) did. I went ahead and removed all the UI stuff.

Comment 35

[gene smith]

Attachment: forcesel.patch

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863097 [details] [diff] [review]
forcesel.patch

Excellent! This is exactly what I had in mind. The patch needs a bit of polishing, but I'd accept it in it's basic functionality. I hope everyone is happy with this: UI gone, auto-detect implemented, and you can see it at work an correct it if needed.

Please note: The access key was changed to "S" in bug 1359069:
https://hg.mozilla.org/comm-central/rev/f5a9ac6e50ac59cf0c07433abf92e66e3afe7ce4#l1.12
so you're patches don't apply to latest trunk.

Magnus, can you settle for this?

Comment 37

[gene smith]

Attachment: id1.png

I have one more suggestion that may or may not be feasible. I don't like the hardcoded strings that I put in the code to detect the bad server. Maybe a new string like in this attachment could be used. Something like this could be used to determine the ID strings of servers that require the extra select. In the attachment it includes the ID strings for charter and another hypothetical  server (e.g., yahoo) that also might have a problem. It is "comma separated" with a given server and the servers are "semicolon" separated so the string could be as long as necessary and specify any ID item for comparison.

I have haven't tried to figure out how this would/could be decoded and used, but something like this, or something maybe much simpler could avoid hardcoding the ID strings.

Comment 38

[Jorg K (CEST = GMT+2)]

Yes, you can do another preference: mail.server.default.forceSelectDetectString or should that be mail.imap.forceSelectDetectString?

Let me know if you need help with string processing. But let's not over-engineer the solution. Do you really think, there are users who use two different servers which both need "force select" but detect differently? Yes, it's a possibility.

Comment 39

[Magnus Melin [:mkmelin]]

Regarding that a value matching default doesn't show up in the prefs, well that's how the prefs system works.

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 8863097 [details] [diff] [review]
forcesel.patch

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

::: mailnews/imap/public/nsIImapIncomingServer.idl
@@ +24,5 @@
>  [scriptable, uuid(ea6a0765-07b8-40df-924c-9004ed707251)]
>  interface nsIImapIncomingServer : nsISupports {
>  
>    attribute long maximumConnectionsNumber;
> +  attribute ACString forceSelect;

Not sure why you wanted it to be a string, but that's not really used for enumerated values in prefs. Someone will spell it wrong, another add a space or uppper case/mix case it.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8250,5 @@
> +          {
> +            // set preference value to "auto-no".
> +            statusString.Assign("auto-no");
> +          }
> +          m_aServer->SetForceSelect(statusString);

I don't understand, what would you'd do with this string?

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #40)
> Not sure why you wanted it to be a string, but that's not really used for
> enumerated values in prefs. Someone will spell it wrong, another add a space
> or uppper case/mix case it.
I want a string since as was pointed out in comment #16, resetting an int preference makes it a string and you have no way to ever set it again without UI. 0=auto, 1=yes, 2=no is totally unintuitive, and you can mix up those values, too. I suggest to only test the first character and we should extend the switch to accept upper and lower case. Besides, with about:config you're in dragon land, so you have to type it correctly. Otherwise we need UI which you don't want. I think opting for an obscure int because advanced users seeking a very advanced feature allegedly can't type is not a good argument.
 
> I don't understand, what would you'd do with this string?
I want to read it. We don't know what the server will answer in the future, so it's good to know whether it was detected or not. I agree that setting a pref value is a little unusual, but the call SetForceSelect() is there. The alternative would be some logging.

Comment 42

[gene smith]

Attachment: forceSelectDetectStrings.patch

This removes the hardcoded ID strings. It allows multiple servers to have the select problem if ever needed. Also within each server there can be multiple strings that match the ID response. This is configured with one string at mail.imap.forceSelectDetectString with servers semicolon delimited and strings within a server comma delimited.
When all strings within a server match the ID response it means do the extra select. 
Right now, you will see the strings for Yahoo included. This is just for test purposes to verify that it works for more that one server needing the extra select, which Yahoo really doesn't.
Also, again this is only a prototype and not a formal patch (I have still left in some printf's) and I haven't updated to the latest.
It seems to work fine right now. My only concern is maybe the comma and semicolon delimiters might appear in the actual ID strings. Maybe I should use more unlikely delimiters like ~ or ^ ??  (Not sure if you can have double delimiters like ;; or ,, if so that would be nice.)

Comment 43

[Jorg K (CEST = GMT+2)]

I like this solution. I think ';' and ',' are good delimiters, if you need ';' in the string it should be escaped, so you need to use 'unescape' before the comparison. I hope MsgUnescapeString() won't add any awkward string processing.

Once you're done, please ask for feedback or review and also please supersede all previous patches when attaching a new one.

Comment 44

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #41)
> I want a string since as was pointed out in comment #16, resetting an int
> preference makes it a string and you have no way to ever set it again
> without UI. 

I think this was a misunderstanding. Resetting it will remove it if it matches default.

> I think opting for an obscure int because advanced
> users seeking a very advanced feature allegedly can't type is not a good
> argument.

Well that's what's used elsewhere.

> > I don't understand, what would you'd do with this string?
> I want to read it. We don't know what the server will answer in the future,
> so it's good to know whether it was detected or not.

Good for what purpose? 

> I agree that setting a pref value is a little unusual, 

Yes you only need to set the member value, not the pref.

Comment 45

[gene smith]

(In reply to Jorg K (GMT+2) from comment #43)
> I like this solution. I think ';' and ',' are good delimiters, if you need
> ';' in the string it should be escaped, so you need to use 'unescape' before
> the comparison. I hope MsgUnescapeString() won't add any awkward string
> processing.

Adding that function is not a problem, still works fine. I did struggle a bit with what you meant by "escaped". I thought it meant precede the char with a backslash, like \; or \,. I finally figured out it means for comma use %2c and semicolon %3b in the strings entered via config editor. (Told you I didn't know much about strings.)

> 
> Once you're done, please ask for feedback or review and also please
> supersede all previous patches when attaching a new one.

Ok, will clean it up and submit the patch this evening.

Comment 46

[gene smith]

Attachment: 1360117-improve-forceSelect-detect.patch

I made a few more changes to avoid redundant calls to IsExtraSelectNeeded() string processing function. It helped to change strings auto-yes/auto-no to yes-auto/no-auto. 
Also, I noticed that being able to set forceSelect true or false was bypassed if the server doesn't support ID capability, so I fixed that. However all of my servers I access do support ID so haven't tested that yet.
Finally, I can't seem to get hg to update those files that changed the accesskey from F to S. I did "hg pull -u" and "hg update". I even reverted my changes on those files and re-did the pull and update but my diffs/patches still show the old key as F. What's the trick? What am I doing wrong? Anyhow, I cheated and manually edited this patch to changed the F to S (for two files). Sorry, not sure if that is legal.

Comment 47

[Jorg K (CEST = GMT+2)]

Sure, you can edit patches, but you need to work out how you got yourself into a knot ;-)

Do you word with Mercurial queues like most of us? So "hg qnew/qpop/qpush"? First start with a clean slate:
hg revert --all to remove all local changes
hg qpop - until the last patch is popped.
hg out - to see the differences with the repository
hg strip -r <changeset> everything you see.
hg pull
hg update -C default.
That should get you clean. You can check that the shortcut key is "S".
Then hg qpush your patch, make sure it's first in the series file.

I'll look at the patch later today.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1360117-improve-forceSelect-detect.patch (v7)

OK, I fiddled with about everything ;-)
Mostly white-space and comment issues. I've also changed some string types. nsAutoCString is generally used for local variables:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Local_variables
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Member_variables

There already was an 'm_server' member variable, so I used it.

I haven't tried it, so please let me know whether it still works, especially with the change to 'm_server'.

Try the interdiff to see what I've changed, ... if it works.

Comment 49

[Jorg K (CEST = GMT+2)]

Yep:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8863309&action=interdiff&newid=8863371&headers=1
I'm happy with my changes, I hope you are, too. I think it's quicker than having a long list of nits.

Comment 50

[gene smith]

Attachment: 1360117-improve-forceSelect-detect.patch (v8)

Thanks for the procedure to untie the knot. I will save it for future reference. Your mods to the patch look fine to me. The pointer to server code you changed seems to work correctly.
I did find one problem regarding the new thing I added to handle the case where the server doesn't support ID or returns an empty ID. In testing this I found that any server with that kind of ID issue will not appear in config editor because the forceSelect value remains at default value "auto". It is necessary, in that case, to set it to non-default "no" so it is visible and can be set to "yes" by the user later if desired. This patch adds that.

Comment 51

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863506 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v8)

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

I fixed plenty of nits in (v7) myself, but now I'll leave it to you.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8308,5 @@
> +  {
> +    switch (m_forceSelectValue.get()[0])
> +    {
> +    case 'a':
> +      {

Nit: Brace not required, please fix indentation below.

@@ +8309,5 @@
> +    switch (m_forceSelectValue.get()[0])
> +    {
> +    case 'a':
> +      {
> +        // if default "auto", set to "no" so visible in config editor

Nit: Upper case.

@@ +8312,5 @@
> +      {
> +        // if default "auto", set to "no" so visible in config editor
> +        // and set/keep m_forceSelect false.
> +        nsAutoCString statusString;
> +        statusString.Assign("no");

Maybe "no-auto", since this is something we're setting automatically. It's not the yes/no the user should set.

Comment 52

[gene smith]

(In reply to Jorg K (GMT+2) from comment #51)
> Comment on attachment 8863506 [details] [diff] [review]
> 1360117-improve-forceSelect-detect.patch (v8)
> > +    switch (m_forceSelectValue.get()[0])
> > +    {
> > +    case 'a':
> > +      {
> 
> Nit: Brace not required, please fix indentation below.

Unless I misunderstand what you are referring to, since I have variables declared inside the case, you have to have braces around it. Apparently, you don't need braces if its the last case: or default: of the switch (like we did right above this point).
Around line 2645 of the same file also uses them with the same indention style.
Re: http://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement

I included your other comments in patch v9.

Comment 53

[gene smith]

Attachment: 1360117-improve-forceSelect-detect.patch (v9)

Comment 54

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863556 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v9)

Thanks for the updated patch. Sorry, I was all wrong. (And please, always supersede previous versions, even if they're mine.)

I'm happy with the patch but let's give Magnus the chance to veto it ;-(

Comment 55

[Jorg K (CEST = GMT+2)]

OK, one last thing. Most IMAP server preferences are actually "snake_case" and not "camelCase":
https://dxr.mozilla.org/comm-central/rev/a7d9e926d66e7d79b2292945d7897cb5c9efe553/mailnews/imap/src/nsImapIncomingServer.cpp#279

Since we're also changing the meaning of the preference, I think we should abandon "forceSelect" (only) in the preference name and replace it with "force_select". OK? Oh, and there is also "forceSelectDetectString".

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: 1360117-improve-forceSelect-detect.patch (v10).

OK, I've changed the preference name. I've also tried to start TB and it crashed immediately on:
  // Set preference value to "no-auto".
  statusString.Assign("no-auto");
}
nsresult rv;
nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryReferent(m_server, &rv); <<=== crash here.

Hit MOZ_CRASH(nsWeakReference not thread-safe) at c:/mozilla-source/comm-central/mozilla/xpcom/base/nsISupportsImpl.cpp:36

Has that not crashed for you, Gene? In IMAP we have a UI thread and and IMAP thread and something went wrong here.

Comment 57

[Jorg K (CEST = GMT+2)]

I'm sorry, of course I contributed the crashing code, the do_QueryReferent() stuff. In my defence I'd like to say that sticking the server that gets passed in into a handy member variable wasn't correct either.

Kent replaced uses of do_QueryReferent() calls in bug 1299116, so that would be the place to study this. If you look at the uses of do_QueryReferent() in nsImapProtocol.cpp, they mostly have a comment that it runs on the UI thread only.

Comment 58

[gene smith]

(In reply to Jorg K (GMT+2) from comment #56)
> 
> Has that not crashed for you, Gene? In IMAP we have a UI thread and and IMAP
> thread and something went wrong here.

No, I have never seen a crash at all. Does it matter the I am not building with debug enabled? The error you show "nsWeakReference not thread-safe" sounds like a warning that maybe I don't have enabled?

Comment 59

[Jorg K (CEST = GMT+2)]

Yes, MOZ_ASSERT() only triggers in debug builds which you should use for development.

Comment 60

[gene smith]

Attachment: use-sink.patch (informal)

Didn't know I was supposed to *always* run with debug. Last time I did seemed like it constantly printed tons of stuff that didn't seem important so I turned it off.
Anyhow, this patch (against my latest changes, not your patch from today, fixes what that other bug you pointed me to fixes (I think). 
It compiles and runs but I haven't tested it in detail, but it does set the appropriated accounts to yes-auto and no-auto so it seems to work OK. 
If it looks OK to you, I will clean it up and merge with your patch from today and submit a new formal patch tomorrow (and also rebuild and test with debug enabled!).

Comment 61

[Jorg K (CEST = GMT+2)]

Yes, I think this is right, it uses the technique from bug 1299116.

Comment 62

[gene smith]

Attachment: 1360117-improve-forceSelect-detect.patch (v11)

Comment 63

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864395 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v11)

OK, let's go with this. I can't do a lot of testing, but I started TB and all IMAP servers I have in my profile are set to "no-auto" as expected.

Comment 64

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/edc74844fc0aa021de8276df7305ed1e445f5768

Comment 65

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864395 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v11)

Let's get this into beta quickly so the previous solution from bug 1231592 will never be published.

In due course, let's consider for TB 52. I hope Ben agrees ;-)

Comment 66

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/77f134f6ddb895b67f19706010f8def09418c4ab

Comment 67

[Ben Bucksch (:BenB)]

Thanks, everybody!

Comment 68

[:aceman]

Jorg, why is the wrapper function SetServerForceSelect needed (not just the automatic SetForceSelect)? Is it something like needed a void function for NS_SYNCRUNNABLEMETHOD1() ? Then on the other hand, you return something from SetServerForceSelect() even though it is declared void.

Comment 69

[Jorg K (CEST = GMT+2)]

You need to read comment #57 and attachment 8864008 [details] [diff] [review] which shows this additional tweak well. Basically we're adapting the solution from bug 1299116 here to avoid the do_QueryReferent() calls:
https://hg.mozilla.org/comm-central/rev/e5fd1b18853f

The code here is adapted from
  void setServerDoingLsub(in boolean aDoingLsub);
https://hg.mozilla.org/comm-central/rev/e5fd1b18853f#l2.25
https://hg.mozilla.org/comm-central/rev/e5fd1b18853f#l3.50
https://hg.mozilla.org/comm-central/rev/e5fd1b18853f#l4.184

Surely the function is declared 'void' in the IDL, however, of course, it returns a success/failure return code.

Gene's change is 100% correct as far as I can see.

Comment 70

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864395 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v11)

As much as I'd like to ship this in TB 52 ESR, I've been criticised for uplifting too many functional changes to the "stable" branch. This bug causes a change in behaviour, see bug 1352859, so I'm afraid the critics have won and users need to wait for TB 59 ESR or use a beta.

Comment 71

[gene smith]

Attachment: Proposed patch with behavior change only for charter

Hi Jorg -
I don't know if something like this is acceptable for minimal behavior change. The proposed patch would cause the behavior change only for charter users. Other servers like yahoo, outlook, gmail etc will *not* see a behavior change. Unless the user tweaks the force select value to or from "yes-auto", which only charter users (actually openwave) will initially see, no UidExpunge() will be called that causes the behavior change.

I have tested the proposed change with charter, outlook and yahoo and UidExpunge is only called for charter.

The other option for no behavior change is just to skip the call to UidExpunge() entirely. But this causes the charter bug where you can't restore deleted emails; but the other charter fix for new emails not being seen is still OK since the forced select is not affected by this patch.

So hopefully something like this would be acceptable to allow fixing the charter issues in a public release before 2019 that were reported 2 years ago.

Comment 72

[Jorg K (CEST = GMT+2)]

Gene, I see you care deeply about Charter users and the reputation of Thunderbird. So do I, at least for the latter part.

So this patch basically fixes bug 1352859, in fact let's not call it a bug but a behaviour change that's in fact an improvement.

Why don't you attach the patch over in that bug, here we're well and truly done. We should discuss the behaviour change over in the other bug.

What I don't understand is what should happen with that patch. We apply the patch from bug 1231592, then this bug here, then your new idea which should go into bug 1352859. We land all this on TB 52 (and my critics will have me fired, but that's a different story)? And then what? Leave it in the product? So bug 1352859 is fixed for all non-Charter users? I'd against that.

Anyway, let's move the discussion to bug 1352859.

Comment 73

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914202 [details] [diff] [review]
Proposed patch with behavior change only for charter

I'm generally in favour of this, but in bug 1352859 please and and with a plan to move forward.

Comment 74

[gene smith]

Comment on attachment 8914202 [details] [diff] [review]
Proposed patch with behavior change only for charter

This obsoletes the patch. I will formalize the patch and attach it at bug 1352859 with further discussion.

Comment 75

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864395 [details] [diff] [review]
1360117-improve-forceSelect-detect.patch (v11)

By popular demand ;-)
Depends on bug 1352859.

Comment 76

[Jorg K (CEST = GMT+2)]

TB 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/06c1061099b79e48ff103bc811fc3d7f95502368
Uplifted beta version without the string changes.