556728 - [autoconfig] config file <leaveMessagesOnServer>true</> doesn't do what it says

Status: NEW

(Thunderbird :: Account Manager, defect, P3)

Description

[Ben Bucksch (:BenB)]

The autoconfig file <https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat> was defined with a <pop3><leaveMessagesOnServer>true</leaveMessagesOnServer></pop3> option (optional, default false). That was intended for services like Gmail.

Later, somebody added <daysToLeaveMessagesOnServer>. If leaveMessagesOnServer and daysToLeaveMessagesOnServer=14, the messages would be left on the server at first, but deleted after 14 days. In fact, that's the default. While that makes sense in the narrow sense, in combination it's bad.

It breaks the original purpose of leaveMessagesOnServer. If you set <pop3><leaveMessagesOnServer>true</leaveMessagesOnServer></pop3> as originally defined, the messages will be deleted anyways (after 14 days).

The spec also said:
<!-- remove the following and leave to client/user? -->

That we leave the messages on the server for a some time at first is a *client* UX decision. It has nothing to do with the ISP and therefore should not be in - nor have any effect whatsoever - on config files.

So, <leaveMessagesOnServer>true</leaveMessagesOnServer> should just leave the messages on the server *forever*. That's what the name implies, and that's what it should just do.

Implementation would be to set .leaveMessagesOnServer = true and .daysToLeaveMessagesOnServer = -1, but that's a pure implementation detail.

ISPs have no reason to influence daysToLeaveMessagesOnServer, given that it was an client UX decision, so that shouldn't be exposed at all.

Comment 1

[Ben Bucksch (:BenB)]

Currently, the code doesn't do what the format spec says. To stabilize the autoconfig file format v1.1, we must fix this in TB 3.1. Patch is trivial, will attach soon.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

It seems like the current file format makes it too easy for users to shoot themselves in the foot because two different fields can contradict each other, so if it's possible, I'd prefer to fix that problem.

If one were designing from scratch, I suspect the "right" fix would be to remove leaveMessagesOnServer entirely, and allow daysToLeaveMessagesOnServer to have two "special" values: 0 and forever.  What's unclear to me is what the backward / forward compatibility effects of that change would be.

Thoughts?

Comment 3

[Ben Bucksch (:BenB)]

Personally, I don't like "special values", but I see your idea.
For me, this is a matter of server perspective vs. client perspective. I don't why the server should set daysToLeaveMessagesOnServer = 14, unless it's very special circumstances. I could see why the server would set leaveMessagesOnServer = true, if it's not as smart as GMail and wants to prevent one POP3-fetch to empty the IMAP inbox. So, from a server perspective, only leaveMessagesOnServer = true makes sense. The whole rest, including daysToLeaveMessagesOnServer is merely a client UX decision and not relevant to the mailbox, but to our UI. Therefore, I believe only leaveMessagesOnServer belongs in the file and daysToLeaveMessagesOnServer shouldn't even be supported, and if it is for some obscure cases, it should be hidden under the carpet somehow :).

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Perspectives are merely perspectives; they don't necessarily imply separability in the real world.  In this case, both client and server are part of a larger system, and ISPs may want to set the days-on-server default based on their disk-space constraints.

However, rather than basing any blocking decision on my speculation here, I'd like to read the reasoning / messages / bugs behind adding the daysToLeaveMessagesOnServer in the first place.  When/where did that discussion happen?

Comment 5

[Ben Bucksch (:BenB)]

> I'd like to read the reasoning / messages / bugs behind adding the
> daysToLeaveMessagesOnServer in the first place.

I fear there was no (explicit) reasoning or bugs at all. The change which introduced it was (on the branch before commit):
<https://hg.mozilla.org/users/dascher_mozilla.com/autoconfig/rev/4c4cffea681f>
All it says is the commit message "default to leaving pop3 messages on server". I suspect that bienvenu just threw it without any concrete reason.

In fact, there's no config file at all which uses it. All config files are on the ISPDB server, and the only one with <daysToLeaveMessagesOnServer> are inbox.lv and inbox.lt (see also dependent bug), and they use a value of 999 days:
inbox.lt:        <daysToLeaveMessagesOnServer>999</daysToLeaveMessagesOnServer>
inbox.lv:        <daysToLeaveMessagesOnServer>999</daysToLeaveMessagesOnServer>
Now, that doesn't make any sense, does it? ;-)

In other words, the only user of <daysToLeaveMessagesOnServer> actually wants "never". Exactly what I specced <leaveMessagesOnServer> = true (without any <daysToLeaveMessagesOnServer>) should mean.

Comment 6

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

I suspect it was there because it's there when we create the account.  But I would believe that it shouldn't be in the XML.

I don't know that it's blocking, but I think we want it, for what that's worth.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Rather than speculating, let's ask the author!  David, care to chime in?

Comment 8

[David :Bienvenu]

The reason was that we thought ISP's might want to set it. I understand that people think ISP's shouldn't have control over that setting, but enterprises and universities could have a legitimate reasons for having that setting.

Comment 9

[Ben Bucksch (:BenB)]

I think most enterprises use IMAP. I don't see why they'd need this even when using POP3, and I'd rather wait until we have a good reason.

As-is, it does actually go wrong, as you can see with inbox.lv, and the fact that <leaveMessagesOnServer> = true doesn't do what it says.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with drivers, we wouldn't hold 3.1 for this.  

I still suspect that having a single field rather than two separate fields is the right fix, because it makes it significantly harder for users editing this functionality to shoot themselves in the foot.  But I'd like bwinton to make that call.

Comment 11

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

I'ld like to get it in, and would give it some priority in my review queue, because I don't want to have _too_ many versions of the autoconfig xml floating around out there, but yeah, I don't think it actually blocks.

Comment 12

[Ben Bucksch (:BenB)]

Attachment: Fix, v1 - first pass

Comment 13

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

Comment on attachment 443450 [details] [diff] [review]
Fix, v1 - first pass

>+++ b/mailnews/base/prefs/content/accountcreation/createInBackend.js
>@@ -103,18 +103,19 @@ function createAccountInBackend(config)
>     prefs.setBoolPref(leaveOnServerPref,
>                       config.incoming.leaveMessagesOnServer);
>-    prefs.setIntPref(daysToLeaveOnServerPref,
>-                     config.incoming.daysToLeaveMessagesOnServer);
>+    if (config.incoming.daysToLeaveMessagesOnServer > 0)

I wonder if this should be ">= 0" instead?

Also, doesn't this mean that we would never set it to -1, and so your change below won't ever write the correct value, and it'll always get the default of 14 days?

Maybe it should just be "if (config.incoming.daysToLeaveMessagesOnServer != null)".

>+++ b/mailnews/base/prefs/content/accountcreation/readFromXML.js
>@@ -120,21 +120,23 @@ function readFromXML(clientConfigXML)
>           if ("leaveMessagesOnServer" in iX.pop3[0])
>+          {
>+            if (sanitize.boolean(iX.pop3.leaveMessagesOnServer))
>+            {
>+              iO.leaveMessagesOnServer = true;
>+              iO.daysToLeaveMessagesOnServer = -1;
>+            }
>+          }

I think this would be better expressed as:
          if ("leaveMessagesOnServer" in iX.pop3[0] &&
              sanitize.boolean(iX.pop3.leaveMessagesOnServer))
          {
            iO.leaveMessagesOnServer = true;
            iO.daysToLeaveMessagesOnServer = -1;
          }

And, as always, I'ld like to see a test to make sure that the correct values are getting into the correct places.

Thanks,
Blake.

Comment 14

[Ben Bucksch (:BenB)]

> Maybe it should just be
> "if (config.incoming.daysToLeaveMessagesOnServer != null)".

The default per accountConfig.js is 14 days, so this would never be true.

> Also, doesn't this mean that we would never set it to -1, and so your
> change below won't ever write the correct value, and it'll always
> get the default of 14 days?

Frankly, I don't know how to say "infinite" here.

http://mxr.mozilla.org/comm-central/search?string=num_days_to_leave_on_server
We have pref("mail.server.default.num_days_to_leave_on_server", 7); , but I don't know whether we fall back to that. I'd assume we do.

I tried setting the pref to -1, because I saw that as initialization value for the variable in the backend, but that didn't work, at least not in the AccountManager UI, it showed "[x] days to leave on server: [  -1 ]". (Maybe the UI implementation is just bad and wants a separate pref for the checkbox just for the UI, I don't know.)

bienvenu, how do I say "never expire"?

Ben

Comment 15

[Ben Bucksch (:BenB)]

FWIW, the AccountManager UI showed the correct value with my patch. I just don't know whether the backend will also check "mail.server.default.num_days_to_leave_on_server". I actually think it does not, so the patch should work. It's kind of hard to test :).

Comment 16

[David :Bienvenu]

(In reply to comment #14)
>> 
> bienvenu, how do I say "never expire"?

I believe you would set the boolean per-server pref delete_by_age_from_server to false

Comment 17

[David :Bienvenu]

or poke the server attribute deleteByAgeFromServer, which will set the pref for you.

Comment 18

[Ben Bucksch (:BenB)]

bwinton, allowing 0 as value a) doesn't make sense and b) is not supported by the backend.
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#1039
  if (m_pop3ConData->uidlinfo && numDaysToLeaveOnServer > 0)
  {
    PRUint32 nowInSeconds = TimeInSecondsFromPRTime(PR_Now());
    PRUint32 cutOffDay = nowInSeconds - (60 * 60 * 24 * numDaysToLeaveOnServer);
We also need to set server.deleteByAgeFromServer = true :
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#1009
That's mirrored to a pref and I guess that explains the checkbox, too.

Looking into it right now.

Comment 19

[Wayne Mery (:wsmwk)]

related to Bug 380583 - when ISPdata wizardSkipPanels set to false, leaveMessagesOnServer does not work ?

Comment 20

[Ben Bucksch (:BenB)]

No, when bug 380583 was filed, the config file format here didn't even exist, so can't possibly be related. That bug is about the old format.

Comment 21

[Ludovic Hirlimann [:Usul]]

Ben as this been fixed by the rework on the account manager ?

Comment 22

[Ben Bucksch (:BenB)]

No.

Comment 23

[Ben Bucksch (:BenB)]

bwinton, you r- this patch almost 4 years ago, with relatively minor problems (just code style preferences and a test). Then I must have forgotten about it. I don't feel like touching this again.

Comment 24

[Ben Bucksch (:BenB)]

I still think this should be fixed, as Thorben shows.

Comment 25

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

While I agree this should be fixed, all of my spare Thunderbird time is taken up by reviews.  Perhaps Thorben or someone else can pick this one up and get it across the finish line?