Keep sent folder offline store up to date

RESOLVED FIXED in Thunderbird 3.3a3

Status

MailNews Core
Database
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 3.3a3
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
+++ This bug was initially created as a clone of 562115 +++

In order to allow gloda to index replies in a timely manner, we need to keep the offline store for imap sent folders up to date as much as possible. I believe we can do this by essentially fcc'ing the offline store at the same time as we append to the imap server, and hookup the offline copy when we create the header for the newly appended message.
David, any hope we can do the same for the Drafts folder? If we are to index replies in a timely manner, we might as well index drafts we just saved in a timely manner too!
(Assignee)

Comment 2

7 years ago
(In reply to comment #1)
> David, any hope we can do the same for the Drafts folder? If we are to index
> replies in a timely manner, we might as well index drafts we just saved in a
> timely manner too!

Yes, good point, we'd get that for free, most likely, since the code is the same.
(Assignee)

Comment 3

7 years ago
I could have sworn that I saw code to do exactly this but I can't seem to find it...keeping the offline store up to date would only solve half the problem.  We wouldn't need to download the new message body, but we'd still need to update the sent folder for gloda to know about the new message. What we'd need to do is add a fake header like we do for offline message move/copies, and an offline operation that would remove the fake header before downloading the real header, and tell gloda about the fake header.

There's still the awkward step where we remove the fake header and replace it with the real header. We could send a notification to gloda about this, once we invent such a notification. But this might just be a performance optimization, because to gloda, it should just look like we removed a message, and then added almost exactly the same message almost immediately, which might cause a bit of churn, but should result in gloda being up to date for all but a teeny bit of time.
(In reply to comment #3)
> There's still the awkward step where we remove the fake header and replace it
> with the real header. We could send a notification to gloda about this, once we
> invent such a notification. But this might just be a performance optimization,
> because to gloda, it should just look like we removed a message, and then added
> almost exactly the same message almost immediately, which might cause a bit of
> churn, but should result in gloda being up to date for all but a teeny bit of
> time.

The one potential glitch here is that if the message key changes and the Gloda messages were retrieved using Gloda.getMessageCollectionForHeader or Gloda.getMessagesCollectionForHeaders then the query that defines the collections explicitly references the message key.  So the fake message will get removed from the collection but the real message will not get re-added.

This does not pose a problem for queries against a conversation id, which I think covers all the existing UI cases.  Specifically, clicking on a message in faceted search does a conversation query, as does the "show in conversation" option of the stock message reader.  And I believe the Thunderbird Conversations extension uses conversation queries too.
Yes, but Thunderbird Conversations requires gloda messages to have a corresponding folderMessage, that is, to be backed by a valid nsIMsgDbHdr. If the only message that gets attached to the conversation is the fake header, will it have a valid msgHdr?
(In reply to comment #5)
> Yes, but Thunderbird Conversations requires gloda messages to have a
> corresponding folderMessage, that is, to be backed by a valid nsIMsgDbHdr. If
> the only message that gets attached to the conversation is the fake header,
> will it have a valid msgHdr?

As I understand it, a fake header still looks like a header.  The only problem I could see cropping up is if you were to get into territory where you were holding onto an invalidated gloda message instance or invalidated nsIMsgDBHdr.

A quick grep of Thunderbird Conversations shows that all the onItemsRemoved methods are stubs, so depending on whether gloda perceives the remove/add pair as a modification or two independent operations your gloda message may remain valid or it may not.  If your gloda message gets modified, you should be fine as long as you forget about all the returned nsIMsgDBHdrs whenever you yield flow control (which includes doing anything that can spin a nested event loop.)

The bad news is that if gloda hears about the deletion of a header and it finds a valid gloda id on that header, it is going to check all live collections and kill those messages dead.  Gloda would actually be somewhat happier if it didn't hear about the removal at all and just heard about the new addition because when it goes to index the added message it will find that it already knows about a message with the same message-id in the folder and will try and retrieve it by its message key, find it gone, then declare that it is a modified message and will generate a notification along those lines.

As such, I would say we definitely want to tell gloda about the transition...

** Goodish News **
If we just generate a nsIMsgFolderListener::msgsMoveCopyCompleted notification where we provide the destination headers, gloda will do basically the right thing.  This is because we fast-path the "local move" case, where we define any situation where we have the destination headers as a "local move" case.

The "meh" news is that currently the check logic does a little bit more work than it needs to because it is assuming that the source and destination folders will be different and we will generate onItemsModified notifications even though we probably don't need to be generating those.  Gloda could obviously detect the "hey, same source and target folder" case to deal with this.

Having said that, gloda can also handle a new type of notification, but the local move has certain things to recommend it.  This need not impact what we do in terms of DB change listeners or nsIFolderListeners.
(Assignee)

Comment 7

7 years ago
A fake header looks exactly like a real header to you - the only thing that's fake about it is that doesn't actually exist on the imap server, in the sense that we don't know the IMAP UID of the corresponding message. So we could run into trouble if we try to manipulate the message on the server before we know the correct UID. Currently, we're protected from this because we always update a folder when the user selects it, and delete the fake header and replace it with the real header. Cross-folder saved searches could currently potentially have an issue, but gloda conversations by their nature will definitely have an issue if we go down this road, because moving a message doesn't logically remove it from the conversation. E.g., if you archive a message in a conversation, and the fake msg is put in the conversation, and then you delete the fake msg, we'll very likely delete the wrong message on the imap server.

I'll have to think about this - we always update a folder before manipulating any of the messages in it, so we always know the correct uid at the time we actually send the command to the imap server. But we convert message header to a uid when we construct the url, which is long before we know the correct uid. We could either fix up the url on the fly when we discover the correct uid, or we could prevent urls from getting run on fake message headers.
(Assignee)

Comment 8

7 years ago
Created attachment 499201 [details] [diff] [review]
wip to keep move targets and sent/drafts, etc up to date for gloda

this implements part 1 of the solution - gloda should know about moved messages and fcc'ed messages immediately. It doesn't deal with sending a notification that a message hdr's key has changed, nor does it deal with the fact that we need to prevent imap urls from getting run on the fake headers. I think we also need to take advantage of UID plus for servers that support it to use the actual UID the server is going to assign for the real header, for the fake header uid.
Assignee: nobody → bienvenu
(Assignee)

Comment 9

7 years ago
Created attachment 502128 [details] [diff] [review]
wip with new notification

This adds the notification that a hdr has changed (note that the key might not really have changed, but the header is a real header now - perhaps I should name the notification differently). Listeners should know that they are going to get a msgAdded notification for the changed header, which they probably want to ignore, if they already knew about the new header. The thinking is that some listeners will pay attention to the destMsgHdr array in the msgMoveCopied notification, and some won't. The latter will want to know about the msgAdded notification, and can ignore the keyChanged notification.
This patch seems to basically work; I need to clean up the comments a bit. I'm hoping someone else will make whatever changes are needed to gloda to handle the new notification correctly.
Attachment #499201 - Attachment is obsolete: true
So to summarize the changes, they are:

1) Offline IMAP moves will now tell us about the (fake) message header(s) in the target location.  Those message headers will be marked with a pseudo key.

2) When IMAP hears about a new header, it will (continue to) check if the message corresponds to a (fake) message we already know about.  If it is, a new msgKeyChanged notification will be generated.  The logic that generates a msgAdded notification will continue to generate the notification, and the message header will continue to mark the message as not classified.

Because of that added notification and claiming the message is not classified, I presume the classification logic will be run on the message again.  I presume the classification logic will see prior markings on the message and so decide to not re-classify the message, but still report msgsClassified on the message (just via the fast-path for things that don't get classified.)

This raises a question which is why I'm copying rkent... if we know the message was a (fake) message that came from elsewhere, should we still mark it as not classified?  If it's possible for the message to have been moved prior to classification (like by a filter or a super fast user), I presume it's proper to still tell the classifier.  (It would be undesirable for the IMAP code to have to know about the classifier's decision making process for whether something is classified, I presume.)

The current net effect to gloda (without changes) is that gloda will be able to produce a message header for a message during the time between the move and when the true header is realized.  However, because we will still receive a classification event later, gloda will reindex the message.  That's currently good because we don't do anything with the message key change.

However, when we make use of the msgKeyChanged notification, we still need to contend with the the msgsClassified notification that will happen later.  Someone needs to maintain some state that says "hey, this classification is only happening because of a move update".  Maybe we could add a new processing flag that we clear after the msgsClassified notifications go out?
Actually, it looks like the "pseudoHdr" message attribute is never being removed, but I presume that's a bug...

Also, m_pseudoHdrs does not appear to be persistent, but the documentation for the message key notification change would appear to be strong enough that it should be (or that it should be rebuilt from the .msf file.)  Should its contents be persisted in whatever an additional mork-value-space is, or perhaps the algorithm changed to just look for that "pseudoHdr" key and use that as the source of the value rather than the hash-map?
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> Actually, it looks like the "pseudoHdr" message attribute is never being
> removed, but I presume that's a bug...
The whole header is getting deleted from the db, so I don't think that's a bug.
> 
> Also, m_pseudoHdrs does not appear to be persistent
m_pseudoHdrs has a fairly short lifetime - when we update a folder, we go through the offline operations for the folder, and add entries for each offline move result, and then we immediately update the folder, which downloads the headers, etc.

True, you could delete the .msf file, but you'll get a lot of "extra" notifications when that happens, not just this one.

, but the documentation for
> the message key notification change would appear to be strong enough that it
> should be (or that it should be rebuilt from the .msf file.)  Should its
> contents be persisted in whatever an additional mork-value-space is, or perhaps
> the algorithm changed to just look for that "pseudoHdr" key and use that as the
> source of the value rather than the hash-map?

No, because we've deleted the header already, per above. I initially coded it the way you proposed, so the headers stay around until just before/after the msgKeyChanged notification, but it was very problematic, because the db expects msgKeys to be unique, and we were overwriting the pseudo header with the real header, etc.  I could hand out real fake msg keys so that there's a lot less chance of collision, but that has its own problems.

I forgot that gloda uses the msgsClassified notifications. You will get an additional notification for these headers. I don't want to *not* do that, because some listeners may not be paying attention to all notifications.
(In reply to comment #12)
> m_pseudoHdrs has a fairly short lifetime - when we update a folder, we go
> through the offline operations for the folder, and add entries for each offline
> move result, and then we immediately update the folder, which downloads the
> headers, etc.

Ah, I misunderstood where this was happening; I thought it was happening as part of the initial move/copy operation.  Thank you for the clarification.  It seems like this is safe if we can guarantee that the deletion of the offline operations happens in the same mork database "transaction" as the addition of the new headers.
 
> I forgot that gloda uses the msgsClassified notifications. You will get an
> additional notification for these headers. I don't want to *not* do that,
> because some listeners may not be paying attention to all notifications.

Yes, I agree that the msgAdded/msgsClassified notifications still make sense.  So the question is mainly how to make sure gloda can say "aha, this is the message header whose message key I just touched!".  The easy options are:

a) Gloda adds to a list/map when it hears about a message key change and removes them when it gets the msgsClassified.

b) When the IMAP dude generates the msgKeyChanged notification before the msgAdded notificiation it adds a processing flag.  We clear the processing flag after the msgsClassified notification.

The big question is whether anyone other than gloda cares about this case.  The answer is probably no, so the solution is probably "a".  In a pathological case where it turns out we are generating msgKeyChanged notifications without the follow-on messages, both solutions will result in a memory leak but it's a lot easier to write the paranoia code that detects that and reports the error in JS space.

Consider gloda cool with this fix.
Unless you want the gloda unit test cases to provide the test coverage for this change, I'll write-up the gloda patch once the patch has passing IMAP tests.
(Assignee)

Comment 15

7 years ago
(In reply to comment #14)
> Unless you want the gloda unit test cases to provide the test coverage for this
> change, I'll write-up the gloda patch once the patch has passing IMAP tests.

I was going to add tests for this to the existing folder notification tests, nsIMsgFolderListenerIMAP.js
(Assignee)

Comment 16

7 years ago
(In reply to comment #15)

> I was going to add tests for this to the existing folder notification tests,
> nsIMsgFolderListenerIMAP.js

I've been working on the folder notification tests, but gloda should have a test that it does the right thing with the notifications...
(Assignee)

Comment 17

7 years ago
While working on the tests, I realized that I'm only passing the dest msg hdrs to the msgsMoveCopied notification if the move/copy is done as an offline operation, i.e., through the UI, with a msgWindow. Since that's the case we care most about, I'm inclined to not worry about the non-UI case, as long as gloda maintains the ability to deal with null dest msgs going forward.
(Assignee)

Comment 18

7 years ago
Created attachment 502818 [details] [diff] [review]
wip with unit test fixes

this adds unit tests for the new notification, and fixes an existing test
Attachment #502128 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Created attachment 502820 [details] [diff] [review]
fix for review
Attachment #502818 - Attachment is obsolete: true
Attachment #502820 - Flags: superreview?(bugzilla)
Attachment #502820 - Flags: review?(bugmail)
(In reply to comment #17)
> While working on the tests, I realized that I'm only passing the dest msg hdrs
> to the msgsMoveCopied notification if the move/copy is done as an offline
> operation, i.e., through the UI, with a msgWindow. Since that's the case we
> care most about, I'm inclined to not worry about the non-UI case, as long as
> gloda maintains the ability to deal with null dest msgs going forward.

Yes, I will leave that gloda handling in place.  I will make sure the gloda test performs a move with and without a MsgWindow.  I presume we fixed whatever the problem was previously where we were leaking offline operation things under xpcshell?  (Or maybe my memory is faulty...)

One note I have in my existing unit tests is a big "???" over what UIDPLUS means to this scenario.  With UIDPLUS, will I get accurate message keys immediately and then no msgKeyChanged notification?
(Assignee)

Comment 21

7 years ago
(In reply to comment #20)
> I will make sure the gloda
> test performs a move with and without a MsgWindow.  I presume we fixed whatever
> the problem was previously where we were leaking offline operation things under
> xpcshell?  (Or maybe my memory is faulty...)
I think I was mistaken about the msgWindow being the determining argument - it's actually the canUndo arg that makes a copyMessages call perform the copy with an offline operation.

I don't remember the problem with leaking offline operations under xpcshell, but the test does look pretty leaky. I'd been running it under check-interactive trying to get it working, so I hadn't noticed the leaks. I'll see how many of them I can clear up.
> 
> One note I have in my existing unit tests is a big "???" over what UIDPLUS
> means to this scenario.  With UIDPLUS, will I get accurate message keys
> immediately and then no msgKeyChanged notification?

No, UIDPLUS has no effect on the notifications you get for msg copies, because we're doing the copy offline, and notifying from there, before we playback the offline operation and get the response from UIDPLUS. UIDPLUS might come into play for operations that use IMAP APPEND, e.g., fcc'ing the sent folder. But I always send a msgKeyChanged notification, even if the key hasn't changed, so that you can know the header is "real".
(Assignee)

Comment 22

7 years ago
Created attachment 502955 [details] [diff] [review]
fix memory leak

thx for bringing up the memory leak issue - I don't know if this was the leak you were referring to, but this patch makes the imap folder listener pass w/o any leaks, and I don't see any imap offline ops in any of the other leak logs w/ this patch.
Attachment #502820 - Attachment is obsolete: true
Attachment #502955 - Flags: superreview?(bugzilla)
Attachment #502955 - Flags: review?(bugmail)
Attachment #502820 - Flags: superreview?(bugzilla)
Attachment #502820 - Flags: review?(bugmail)
(In reply to comment #21)
> I think I was mistaken about the msgWindow being the determining argument -
> it's actually the canUndo arg that makes a copyMessages call perform the copy
> with an offline operation.

I'm looking at nsImapMailFolder::CopyMessages, and this does appear to be the case.  Unfortunately, it also seems to be the case that it tells the playback timer to initiate playback 500ms in the future.  That is annoying from a unit test perspective because of the fixed timeout and the potential for stuff happening in the background of the test that test logic is not explicitly triggering.

It looks like I can fake things out by declaring that WeAreOffline, in which case we take the non-pseudo-offline case but still invoke CopyMessagesOffline.

Does that seem like a reasonable approach?
Hm, of course, gloda will, by default, interpret that as an invitation to stop indexing.  But I can make sure gloda ignores all online/offline directives except when we're testing gloda's response to such things.
(Assignee)

Comment 25

7 years ago
(In reply to comment #23)

> It looks like I can fake things out by declaring that WeAreOffline, in which
> case we take the non-pseudo-offline case but still invoke CopyMessagesOffline.
> 
> Does that seem like a reasonable approach?

You'll have to go back online in order to do the update on the dest folder to test the msgKeyChanged notification. That can be a bit tricky because you don't want to start playback of offline operations when you go online. 

My inclination would be to deal with the half second delay playing back, because it would be better to test the really common code path. I'll look and see if there's an easy event to wait for.
(Assignee)

Comment 26

7 years ago
One possibility is to wait for the source folder not to have any offline events left, when (gIMAPInbox.getFlag(Ci.nsMsgFolderFlags.OfflineEvents)) is false. That flag is cleared when playback is finished, I believe.
Right now, the async_move_messages logic is basically:

- Kick off a copy via the copy service
(wait for the async listener from that to complete)
- Force an update of the source folder
(wait for the source folder update to complete)
- Force an update of the target folder
(wait for the target folder update to complete)

Will this mechanism drain the offline operations automatically so even if the timer fires it has no work to do?
(Assignee)

Comment 28

7 years ago
(In reply to comment #27)
> Right now, the async_move_messages logic is basically:
> 
> - Force an update of the source folder
> (wait for the source folder update to complete)
This forces a playback of the copy, so yes, the timer shouldn't have any work to do. I'm not sure if you get the update notification before the playback starts or not, since playback updates the folder first (are you listening for folder loaded, or using the url listener passed into UpdateFolderWithListener?). This could be important, because fakeserver only allows one connection to the server, so every imap operation has to wait for the previous one to finish, so if you kick off the update of the target folder before offline playback gets a chance to run, offline playback will have to wait for that. So if you asynchronously start the update of the target folder, that'll give the offline playback code a chance to sneak the playback in before the target is updated.
async_move_messages:
  http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/messageInjection.js#862

It uses updateFolderAndNotify from mailTestUtils, which currently performs an updateFolder(null) and waits for the OnItemEvent that tells us the update has completed:
  http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/mailTestUtils.js#462
(Assignee)

Comment 30

7 years ago
(In reply to comment #29)

Ok, yeah, you're waiting for folder loaded. So, I believe you're OK. From my tests, we've actually finished offline playback by the time the folder loaded notification is sent.
Comment on attachment 502955 [details] [diff] [review]
fix memory leak

The gloda patch and tests say hooray for this patch.  I will attach that patch to the dependent gloda bug.
Attachment #502955 - Flags: review?(bugmail) → review+
Status: NEW → ASSIGNED
Comment on attachment 502955 [details] [diff] [review]
fix memory leak

>diff --git a/mailnews/imap/public/nsIMsgImapMailFolder.idl b/mailnews/imap/public/nsIMsgImapMailFolder.idl

nit: this file should probably have a uuid bump somehwere

>-
>+  void addMoveResultPseudoKey(in nsMsgKey aMsgKey);

Nit: please add a short description about what this is doing.
Attachment #502955 - Flags: superreview?(bugzilla) → superreview+
(Assignee)

Comment 33

6 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/7cad2968cbb7

Marking in-testsuite+, since I added unit tests for the new notification, and asuth is adding tests that gloda does the right thing with the notification in bug 534449
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
David, I dare to reopen =). The issue I described to you on IRC keeps hitting me: when I send a reply, it appears instantly in my grand unified inbox (that also searches sent folders). That's great, but the conversation view then updates, and tries to stream the message (because the view update kicks in before gloda indexes the message), and that's where it fails.

Now the interesting part is that the standard message reader can't display this message either (although with a different error). If I select the faulty message, and then double-click on it to open it with the standard message reader in a new tab, the contents stay blank.

So I think the problem is more important than it initially seemed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 35

6 years ago
I'm not sure that re-opening this is right - the sent folder offline store is kept up to date now; I believe the fix is that the view code needs to respect the message key changed notification, so that the view doesn't have a stale key. And your code might need to also listen for the notification, if you hold onto msg hdrs...it would be great if we could just change the key of the existing hdr but that is tricky. I don't want two header with the same key floating around, and re-using the existing header might be hard.
Ok, closing then!
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

6 years ago
Bug 628389 filed for remaining issue.
Depends on: 646226
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.