765775 - "delete all but the most recent" deleted new messages first, retained oldest ones (with IMAP)

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Marco]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120601175215

Steps to reproduce:

In a new ubuntu 12.04 installation, I added an IMAP account to thunderbird.

While 16000 message headers were being downloaded, I closed Thunderbird for I wanted it to download only the most recent 2000 instead.

I re-open thunderbird, I Selected "Delete all but the most recent 2000 messages" in "Account Settings" - "Synchronization and Storage"


Actual results:

All but LAST recent 2000 messages have been deleted from the IMAP server. Now the IMAP server only contains the first and oldest 2000 messages. Newer, unread emails were deleted. Webmail only shows those old messages also.


Expected results:

The old messages should have been deleted, not the newer ones.

Also, no indication whatsoever that messages were deleted from IMAP server, rather than local copies. The delete action affected thousands of messages, without any confirm dialog. This destructive operation cannot be undone.

Deleting from server and deleting local copies are two logically distinct operations, they should not be linked together.
When a destructive operation is performed, there should be at least a warning.

Comment 1

[rsx11m]

(In reply to Marco from comment #0)
> All but LAST recent 2000 messages have been deleted from the IMAP server.
> Now the IMAP server only contains the first and oldest 2000 messages. Newer,
> unread emails were deleted. Webmail only shows those old messages also.

That indeed sounds like a bug. I'm not sure though if this is specific to that IMAP server (it shouldn't depend on the order in which the messages are presented, I hope?) or a general issue. In that case, someone with a POP account shouldn't be able to reproduce this (i.e., it would be IMAP specific).

> Also, no indication whatsoever that messages were deleted from IMAP server,
> rather than local copies.

There are two blocks in Synchronization & Storage: The first one affects synchronization behavior only (what you wanted is the "Synchronize the most recent [...]" function) whereas the second is for the retention period and is labelled to affect both local copies and originals on the server.

> Deleting from server and deleting local copies are two logically distinct
> operations, they should not be linked together.

You may want to do one or the other, thus both options are available. I agree that those are too easy to mix up, even after clarifying the labels as done.

> When a destructive operation is performed, there should be at least a warning.

That's bug 582170 and should help preventing such accidental deletions.

Comment 2

[J.J.J]

Dears helpers,
this problem makes me loose 630 new mails.

Bug 765775 (married with bug 582170) "delete all but the most recent" deleted new messages first, retained oldest ones (with IMAP). State:  "unconfirmed".

Forderung Warndialog-Fensters ("feature request"), sobald ein Nutzer in den Einstellungen für "Synchronisation & Speicherplatz" unter "Alle Nachrichten endgültig lôschen ..." einen der beiden Radio-Buttons "Alle außer den neuesten xxx Nachrichten" oder "Nachrichten, die älter sind als xxx Tage" aktiviert: "Need warning for destructive retention settings for POP (Disk Space) and IMAP (Synchronization & Storage) accounts".

Beta-Version Thunderbird 45.0b1 umgesetzt worden, und der Warndialog erscheint POP-Konten (> Speicherplatz) und in IMAP-Konten (> Synchronisation & Speicherplatz).

Comment 3

[ISHIKAWA, Chiaki]

I am not sure where the purging of messages occur exactly.
I checked "retention" using DXR, and found this function eventually.

At least this function does not seem to take the DATE order in consideration when marking
messages for deletion.
It only marks messages for deletion until we mark enough messages for deletion so that
the remaining number of messages in DB is equal or less than the specified number of messages.
(I.e., if the user says, let's keep 2000 messages: TB will mark messages for deletion until the
remaining messages is 2000 or less than it in the first place.)

Or maybe  I am mistaken: it may be that the hdrs->GetNext call gets the message  OLDER messages first ALWYAS?
Is there an implicit assumption about this GetNext() behavior 
across pop3/imap/news servers, etc?)

https://dxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#5378

nsresult nsMsgDatabase::PurgeExcessMessages(uint32_t numHeadersToKeep,
                                            bool applyToFlaggedMessages,
                                            nsIMutableArray *hdrsToDelete)
{
  nsresult rv = NS_OK;
  nsMsgHdr    *pHeader;
  nsCOMPtr <nsISimpleEnumerator> hdrs;
  rv = EnumerateMessages(getter_AddRefs(hdrs));
  if (NS_FAILED(rv))
    return rv;
  bool hasMore = false;
  nsTArray<nsMsgKey> keysToDelete;

  mdb_count numHdrs = 0;
  if (m_mdbAllMsgHeadersTable)
    m_mdbAllMsgHeadersTable->GetCount(GetEnv(), &numHdrs);
  else
    return NS_ERROR_NULL_POINTER;

  while (NS_SUCCEEDED(rv = hdrs->HasMoreElements(&hasMore)) && hasMore)
  {
    bool purgeHdr = false;
    rv = hdrs->GetNext((nsISupports**)&pHeader);  <=*** The order of messages fetched this manner counts!
    NS_ASSERTION(NS_SUCCEEDED(rv), "nsMsgDBEnumerator broken");
    if (NS_FAILED(rv))
      break;

    if (!applyToFlaggedMessages)
    {
      uint32_t flags;
      (void)pHeader->GetFlags(&flags);
      if (flags & nsMsgMessageFlags::Marked)
        continue;
    }

    // this isn't quite right - we want to prefer unread messages (keep all of those we can)
    if (numHdrs > numHeadersToKeep)
      purgeHdr = true;

    if (purgeHdr)
    {
      nsMsgKey msgKey;
      pHeader->GetMessageKey(&msgKey);
      keysToDelete.AppendElement(msgKey);
      numHdrs--;
      if (hdrsToDelete)
        hdrsToDelete->AppendElement(pHeader, false);
    }
    NS_RELEASE(pHeader);
  }

  if (!hdrsToDelete)
  {
    int32_t numKeysToDelete = keysToDelete.Length();
    if (numKeysToDelete > 0)
    {
      DeleteMessages(keysToDelete.Length(), keysToDelete.Elements(), nullptr);
      if (numKeysToDelete > 10)  // compress commit if we deleted more than 10
        Commit(nsMsgDBCommitType::kCompressCommit);
      else
        Commit(nsMsgDBCommitType::kLargeCommit);
    }
  }
  return rv;
}

Comment 4

[ISHIKAWA, Chiaki]

OK, in the above piece of code,
nsCOMPtr <nsISimpleEnumerator> hdrs;
  rv = EnumerateMessages(getter_AddRefs(hdrs));

So hdrs is initialized using |EnumarateMessages()|.

https://dxr.mozilla.org/comm-central/source/mailnews/db/msgdb/public/nsIMsgDatabase.idl#306
 /**
   * Returns all message keys stored in the database.
   * Keys are returned in the order as stored in the database.
   * The caller should sort them if it needs to.
   */
  void ListAllKeys(in nsIMsgKeyArray array);

  nsISimpleEnumerator EnumerateMessages();
  nsISimpleEnumerator ReverseEnumerateMessages();
  nsISimpleEnumerator EnumerateThreads();

So we SHOULD sort them  in the desired DATE order so that the OLD messages comes first
and get fetched with GetNext() first.
("OLD" being based on SENT date or RECEIVED date or whatever.)
Currently from the horror stories, it seems that  there are times, the
database is stored in the reverse order in that the latest message is fetched by GetNext first.
So we must make sure the desired sort order is used.

Anyone to take this?

Comment 5

[ISHIKAWA, Chiaki]

Now, it looks *IF* EnumerateMessages() ALWAYS return the newer messages first, then
probably we can use ReverseEnumerateMessages() and all is well...

Comment 6

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #5)
> Now, it looks *IF* EnumerateMessages() ALWAYS return the newer messages
> first, then
> probably we can use ReverseEnumerateMessages() and all is well...

I think we MUST sort explicitly: we can copy/move messages to a folder at will, which means that
the order in the database cannot be trusted at all when TB tries to delete the messages based on retention-setting.

Comment 8

[ISHIKAWA, Chiaki]

Bug 1215515

Presumably RSS enumaration has similar issues.

Educated Guess:
Enumeration happens in the order the RSS feeds are stored in database.
TB selects the first RSS feeds for deletion until the remaining number RSS feeds is
specified N (retention limit) instead of selecting feeds for deletion AFTER proper SORTING and marking feeds for deletion based on retention policy.

Comment 9

[Wayne Mery (:wsmwk)]

We have many retention bug reports https://mzl.la/2cMZp01
It would be great to see these dataloss issues resolved.
Group effort??

Comment 10

[ISHIKAWA, Chiaki]

I think we should mark this "NEW" as per comment 4.