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



Account Manager
8 years ago
2 years ago


(Reporter: BenB, Unassigned)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(blocking-thunderbird3.1 -)



(1 attachment)



8 years ago
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.


8 years ago
Summary: [autoconfig] config file <leaveMessagesOnServer>true doesn't do what it says → [autoconfig] config file <leaveMessagesOnServer>true</> doesn't do what it says


8 years ago
Blocks: 536546


8 years ago
Assignee: nobody → ben.bucksch


8 years ago
Priority: -- → P1

Comment 1

8 years ago
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.
blocking-thunderbird3.1: --- → ?
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.


Comment 3

8 years ago
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 :).
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

8 years ago
> 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):
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.
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.
Rather than speculating, let's ask the author!  David, care to chime in?

Comment 8

8 years ago
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

8 years ago
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.
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.
blocking-thunderbird3.1: ? → -
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

8 years ago
Created attachment 443450 [details] [diff] [review]
Fix, v1 - first pass
Attachment #443450 - Flags: review?(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] &&
            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.

Attachment #443450 - Flags: review?(bwinton) → review-

Comment 14

8 years ago
> 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.

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"?


Comment 15

8 years ago
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

8 years ago
(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

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

Comment 18

8 years ago
bwinton, allowing 0 as value a) doesn't make sense and b) is not supported by the backend.
  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 :
That's mirrored to a pref and I guess that explains the checkbox, too.

Looking into it right now.

Comment 19

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

Comment 20

8 years ago
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.
Ben as this been fixed by the rework on the account manager ?

Comment 22

7 years ago


4 years ago
Blocks: 959967

Comment 23

4 years ago
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.
Assignee: ben.bucksch → bwinton

Comment 24

4 years ago
I still think this should be fixed, as Thorben shows.
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?
Assignee: bwinton → nobody
You need to log in before you can comment on or make changes to this bug.