436880 - IMAP itemDeleted and itemMoveCopyCompleted notifications quite broken

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: v1, still needs fixing (c0 for details)

There are several problems currently with IMAP itemDeleted notifications, in moving messages to other folders, etc. This bug is to fix them.

Please note: itemMoveCopyCompleted hasn't been used for any of these, as itemAdded is fired when the destination folder is opened, and it doesn't seem to be easy to detect whether a new message is the result of a move/copy, or whether it was added externally.

For messages...
the expected behaviour is:
IMAP folder to IMAP folder copy: itemAdded when destination folder is opened.
IMAP folder to IMAP folder move: itemDeleted + itemAdded when destination folder is opened.
IMAP delete to trash: same as IMAP folder to IMAP folder move
IMAP delete (IMAP style): itemDeleted
IMAP undelete (IMAP style): itemAdded
IMAP delete (immediate): itemDeleted
(All this seems to work fine, at least within the same mailbox)

IMAP folder to local folder copy: itemAdded in local folder
IMAP folder to local folder move: itemAdded in local folder + itemDeleted in IMAP folder
(The above two are still a problem as of this patch -- itemAdded gets passed a null pointer, have to check this)

Local folder to IMAP folder copy: itemAdded when dest folder is opened
Local folder to IMAP folder move: itemDeleted in local folder + itemAdded in IMAP folder.
(Currently this does the itemDeleted notification from within the IMAP folder -- this should ideally be done from within the local folder)

I haven't tested or fixed folders yet, but they seem to need fixing too.

I'm asking for review in case David would like the expected behaviour to be changed, or has some better implementation ideas.

Comment 1

[Rain [:rain1] (inactive)]

That extra comment between lines 6308 and 6323 in nsImapMailFolder.cpp shouldn't be there, and there's now a newline at the end of nsMsgUtils.cpp :)

Comment 2

[David :Bienvenu]

My preference would be that we would keep using move copy notifications for IMAP. For example, with Spotlight, when an imap message is moved from one folder to another, we tell Spotlight, and if the original message is already indexed, it moves the .emlx file to the .mozeml folder for the target folder. Then, when we, or the user, selects the target folder of the move, we do get the itemAdded notification, but we also notice that we already have the .emlx file for the new message, and we then know that we don't have to re-index it. I would think the Vista Search integration could work the same way.

This relies on message-id's really being unique, but we're already relying on that since we use the message-id as the .emlx file name.

Comment 3

[Rain [:rain1] (inactive)]

Yes, I see this, but an objection to this in my mind is that third party extension developers would like accurate notifications. While we can deal with this, they might not be able to do so.

Comment 4

[David :Bienvenu]

The itemMovedCopied notification *is* accurate. Getting the extra itemAdded notification is not accurate, but I'm not convinced that always sending the innacurate notification is an improvement :-) You could definitely argue that it's more consistent. But for me the deciding factor is that the item moved notification allows the listener to optimize - if they just have to move a pointer instead of re-indexing, that's a win.

You're right that this is an issue, but I think addressing it with comments in the IDL would be better than just not using the itemMoved notification.

If this really turns out to be a big issue, we could keep track of this information in the .msf file, and avoid sending the itemAdded notification for the single client case. There will always be the multiple clients accessing the same imap account case, though, so listeners are going to have to handle this case anyway.

Comment 5

[Rain [:rain1] (inactive)]

Right, and this will also take care of the case when items are moved into an IMAP archive folder, and the archive folder itself isn't opened for ages.

Comment 6

[David :Bienvenu]

(In reply to comment #5)
> Right, and this will also take care of the case when items are moved into an
> IMAP archive folder, and the archive folder itself isn't opened for ages.
> 
Heh, right, I forgot about that - that's a very good point. Which reminds me, I think indexers are going to need to somehow force an update of imap folders for the multiple client case, or perhaps we can provide a service that will do an idle time update of imap folders that indexers can use.

Comment 7

[David :Bienvenu]

Comment on attachment 323388 [details] [diff] [review]
v1, still needs fixing (c0 for details)

minusing based on discussion.

Comment 8

[Rain [:rain1] (inactive)]

Attachment: v1.5, WIP, not completely tested yet

There's still a bit of testing to do.

Comment 9

[Rain [:rain1] (inactive)]

Attachment: v2, now uses itemMoveCopyCompleted

Now, the expected behaviour is:
IMAP folder opened, new headers present: itemAdded
IMAP folder opened, headers now not present (eg another client deleted them): itemDeleted
IMAP folder to IMAP folder copy/move: itemMoveCopyCompleted + itemAdded when destination folder is opened.
folder is opened.
IMAP delete to trash: same as IMAP folder to IMAP folder move
IMAP delete (IMAP style): nothing
IMAP undelete (IMAP style): nothing
IMAP delete (immediate/purge): itemDeleted

IMAP folder to local folder copy/move: itemMoveCopyCompleted
Local/IMAP folder on different server to IMAP folder copy/move: itemMoveCopyCompleted + itemAdded when dest folder is opened

Saving drafts/sent messages is a bit of a problem though, as we only get the header when the Drafts/Sent folder is open.

Comment 10

[Emre Birol]

Doesn't TB fire an itemDeleted event for Move operations - both for message and folder?

On a side note: if the operation is stored offline, it won't be played-back until the SOURCE imap folder is selected when online. 

Comment 11

[Rain [:rain1] (inactive)]

> Doesn't TB fire an itemDeleted event for Move operations - both for message and
> folder?

No, it doesn't. At least it shouldn't. itemMoveCopyCompleted is a better choice. It isn't fixed for folders yet -- they still have an itemDeleted notification on move -- but that's for part 2.

> On a side note: if the operation is stored offline, it won't be played-back
> until the SOURCE imap folder is selected when online. 

Hmm, thanks for this information, I haven't yet looked at offline operations.

Comment 12

[David :Bienvenu]

Comment on attachment 323907 [details] [diff] [review]
v2, now uses itemMoveCopyCompleted

why is this needed?

+template<class E> class nsTArray;

Wouldn't including nsTArray.h suffice?

+    // The function doesn't fail when a key is not found, it just silently skips.
+    // Should this change?

No, I think we need to do this for the case of handling expunged imap messages, right, which is an expected case.

This looks reasonable but there's a complicated text matrix - it would be nice to have tests for this, though we'd really need a fake imap server to do them. We could get a start with local messages, however.

Comment 13

[Emre Birol]

Also, can we document this event mechanism somewhere on wiki page. This is one of the aspects of TB that extension developers use most - similar to comment #9

Following would suffix about each event I guess:
o Operation and operands (message, folder, etc)
o Event 
o Context of the event (account, folder, etc)
o Originator (source folder, destination folder, etc)
o Arguments
o Sequence number (if the operation generates more than one event)

thanks

 

Comment 14

[Rain [:rain1] (inactive)]

Attachment: Nits fixed + unit tests added

Two unit tests have been added: one to test generic functionality, one to test notifications for local messages.

The scope of the patch is really larger than the scope of the bug right now.

Mark, could you please see if there's any scope for improvement in the tests? The setup js file is built so that it would work for IMAP tests too without too much trouble.
In particular:
1. Is that 250ms timeout before the next test is started all right? Is there a better way to do this?
2. What about the 3 second error timeout?

The only change to the non-test code is that listeners now get an array on itemDeleted instead of individual headers, as suggested by David. (I'm asking for another sr for this)

One point that may be of interest is the commented out test19. David, now that itemMoveCopyCompleted notifications are only for the parent folder and not for any subfolders, what do you think of changing the code to do the same for itemDeleted?

Comment 15

[Rain [:rain1] (inactive)]

Attachment: license changed for a test

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #14)
> 1. Is that 250ms timeout before the next test is started all right? Is there a
> better way to do this?

I think you'll probably get away with a 0ms timeout. That will still cause the listener threads to end and a new test to be fired off. Unless of course, there's a bit extra of processing that we need to be careful of (in which case, is there a way to wait for that to happen?).

I'll have to look at the rest later when I get time.

Comment 17

[Rain [:rain1] (inactive)]

(In reply to comment #16)
> (In reply to comment #14)
> > 1. Is that 250ms timeout before the next test is started all right? Is there a
> > better way to do this?
> 
> I think you'll probably get away with a 0ms timeout. That will still cause the
> listener threads to end and a new test to be fired off. Unless of course,
> there's a bit extra of processing that we need to be careful of (in which case,
> is there a way to wait for that to happen?).

Well, I guess I'm already waiting for all that I can wait for -- up to three different things, before the next test is executed. There *might* still be stuff to execute after all of this, I can't be sure. It seems to be succeeding with 0ms for this test, but IMAP might be a different story.

However: right now, with the 18 subtests, the test takes around 5 seconds. I'd expect the total number of subtests (with IMAP and IMAP <-> local) to be around 3-4 times this. I'm not sure 15-20 seconds sounds like a very good idea.

Comment 18

[Rain [:rain1] (inactive)]

Ugh, this is broken a bit by the pseudo offline move/delete patch. I'll post an updated version after testing it out.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 324778 [details] [diff] [review]
license changed for a test

I know you said you wanted to update this, but I thought I'd add in some comments, in case you want to include them as well. Note I haven't tried running this yet.

>+const nsIMsgDBHdr = Components.interfaces.nsIMsgDBHdr;
>+const nsIArray = Components.interfaces.nsIArray;
>+const nsIMsgFolder = Components.interfaces.nsIMsgFolder;

We do have Cc, Ci, Cr defined globally for the tests now, so you can use those if you want.

>+const gMFNService = Components.classes["@mozilla.org/messenger/msgnotificationservice;1"]
>+                      .getService(Components.interfaces.nsIMsgFolderNotificationService);
>+const copyService = Components.classes["@mozilla.org/messenger/messagecopyservice;1"]
>+                      .getService(Components.interfaces.nsIMsgCopyService);

Can you be consistent with the prefixes? Lower down where you define other global constants, its normal to use the k prefix rather than the g one. I think the constanst above can remain as they are with g once you've made them consistent.

>+// For CopyFileMessage: this stores the header that was received
>+var gHdrsReceived = new Array();
>+
>+var gMsgHdrs = new Array();
>+var gMsgIds = new Array();
>+
>+// Our listener, which captures events and verifies them as they are received.
>+var gMFListener =
>+{
>+  itemAdded: function(aItem)
>+  {
>+    verify([gEvents.itemAdded, aItem]);
>+    var hdr = gHdrsReceived.pop();
>+    gMsgHdrs.push(hdr);
>+    gMsgIds.push(hdr.messageId);

Here you could have just one array, gMsgHdrs and you could then do:

gMsgHdrs.push({ hdr: hdr, ID: hdr.messageId);

Later you can then access this as gMsgHdrs[n].hdr or gMsgHdrs[n].ID

>+function resetStatusAndProceed()
>+{
>+  gHdrsReceived.length = 0;
>+  gCurrStatus = 0;
>+  // Ugly hack: make sure we don't get stuck in a JS->C++->JS->C++... call stack
>+  // This can happen with a bunch of synchronous functions grouped together, and
>+  // can even cause tests to fail because they're still waiting for the listener
>+  // to return
>+  do_timeout(250, "doTest(++gTest)");
>+}
>+
>+// 'contains' function for array
>+Array.prototype.contains = function(element)
>+{
>+  for (var i = 0; i < this.length; i++)
>+    if (this[i] == element)
>+      return true;
>+  return false;
>+};

What's wrong with indexOf?


>+// Our listener, which captures events.
>+var gMFListener =
>+{
>+  mReceived: 0,
>+
>+  itemAdded: function (aItem)
>+  {
>+    this.mReceived |= gItemAdded;
>+  },

This won't fail if itemAdded is incorrectly called twice. Ditto for the other functions.

>+function run_test()
>+{
>+  // Test: Add a listener.
>+  gMFNService.addListener(gMFListener);
>+
>+  // Test: Notify the listener of all events.
>+  NotifyMsgFolderListeners();
>+
>+  do_check_eq(gMFListener.mReceived, (1 << numOptions) - 1);
>+
>+  // Test: Remove the listener.
>+  gMFNService.removeListener(gMFListener);

You should perhaps do an additional check to ensure that after we've removed the listener it gets no more notifications.


>+const gTestArray =
>+[
>+  // Copying messages from files
>+  test1,
>+  test2,
...

So I think you could actually do something along the lines of:

const gTestArray =
[
  // Copying messages from files
  function () { copyFileMessage(gMsgFile1, gLocalInboxFolder, false); },
  function () { copyFileMessage(gMsgFile2, gLocalInboxFolder, false); },
...
];

This would make it much easier to read and the file a bit shorter.

>+  // Set a limit of three seconds; if the notifications haven't arrived by then there's a problem.
>+  do_timeout(3000, "if (gTest == "+test+") \
>+             do_throw('Notifications not received in 3000 ms for operation '+gTest+', current status is '+gCurrStatus);");

I assume you've checked that this works?

With respect to the timing issue, I probably need to have a look at it once you've updated the patch. If 0ms works for local folders, then I think we ignore IMAP for the time being, and just accept that as the test and see if it is unstable or not (which hopefully it isn't). In theory, the code should be able to accept new commands without delay else otherwise we've got potential performance issues.

Comment 20

[Rain [:rain1] (inactive)]

Attachment: test review comments addressed, IMAP fixed

All the test review comments have been addressed, and the IMAP problem has been fixed by having a notification in nsMsgImapFolder::CopyMessagesOffline().

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 325404 [details] [diff] [review]
test review comments addressed, IMAP fixed

r=me if you:

- fix the bitrot with the test file names
- disable the failing tests, adding comments wrt what is failing and which bug(s) will fix them
- Ensure there's a comment on the appropriate bugs that they need to fix the tests.

Comment 22

[Rain [:rain1] (inactive)]

Attachment: Patch with new, less comprehensive tests

I've replaced the multiple message tests with single message ones for now.

Carrying forward r+sr=bienvenu for the non-test parts, r=Standard8 for the tests.

Comment 23

[Rain [:rain1] (inactive)]

Attachment: One more test disabled

This *should* now work on OS X, given no other problems, but I just can't be sure any more. (There's a lot of stuff held up because of this, so if there's a problem on OS X with this patch, too, I'd be in favour of disabling the whole suite and coming back to this later)

Comment 24

[Rain [:rain1] (inactive)]

Attachment: small cleanup

.hgignore creeped in. ;)

Comment 25

[David :Bienvenu]

the lowest level problem is that the specs for the folders aren't consistent:

matching /Users/davidbienvenu/tbirdtrunk/mozilla/obj-i386-apple-darwin9.3.0/_tests/mailtest/Mail/Local Folders/Inbox.msf with /Users/davidbienvenu/tbirdtrunk/mozilla/obj-i386-apple-darwin9.3.0/mailnews/base/test/../../../_tests/mailtest/Mail/Local Folders/Inbox.msf

this causes the db cache to break, which means that the mailbox protocol code doesn't know which folder the source header belongs to, which means it bails out when trying to advance to the next message. The db cache does a string compare of the native path name:

http://mxr.mozilla.org/seamonkey/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#728

nsMailboxProtocol calls http://mxr.mozilla.org/seamonkey/source/mailnews/local/src/nsMailboxUrl.cpp#224, which fails. Obviously, the second path hasn't been normalized. It's not shocking that this would differ between windows and mac. I'm not sure what code is at fault.  I can add an assertion to nsMailboxProtocol when it encounters this situation. I can probably make it less dependent on the db cache, but it would be good to figure out why we're getting two different string paths so that more tests don't break.

Comment 26

[David :Bienvenu]

it turns out that all are other paths aren't normalized. so the normalized one is the one out of sync. I think going through the io service to create the file uri is normalizing the file path.

http://mxr.mozilla.org/seamonkey/source/mailnews/local/src/nsMailboxUrl.cpp#370

I don't know if normalized is really the right word here...but this seems like an issue with test harness - it seems like the profile directory is leading us astray; I suspect it's what's not normalized.

Comment 27

[David :Bienvenu]

Attachment: [checked in]normalize the profile directory

this should make the test run.

Comment 28

[David :Bienvenu]

Attachment: [checked in]assert if this kind of thing happens again

assert if something similar happens in the future, which might save some poor sucker like me some time.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 326410 [details] [diff] [review]
[checked in]normalize the profile directory

I don't think there is any problem with this, it means we get a consistent path, which is what we would have with the profile directory normally.

r=me

Comment 30

[[e-mail]]

Sorry about the typo ;-)

Comment 31

[Rain [:rain1] (inactive)]

Attachment: [checked in] final patch for checkin, along with the other two

Thank you, David, for fixing this!

So, this is the final patch for checkin (r+sr=bienvenu for non-test parts, r=Standard8 for tests), with the full test suite enabled and bitrot fixed. Verified by Standard8 to run on Mac with bienvenu's patch applied.

Please note that bienvenu's patch needs to be checked in before this one.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 326455 [details] [diff] [review]
[checked in] final patch for checkin, along with the other two

Checking in mailnews/base/src/nsMsgFolderNotificationService.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgFolderNotificationService.cpp,v  <--  nsMsgFolderNotificationService.cpp
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/mailnews/base/test/resources/msgFolderListenerSetup.js,v
done
Checking in mailnews/base/test/resources/msgFolderListenerSetup.js;
/cvsroot/mozilla/mailnews/base/test/resources/msgFolderListenerSetup.js,v  <--  msgFolderListenerSetup.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListener.js,v
done
Checking in mailnews/base/test/unit/test_nsIMsgFolderListener.js;
/cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListener.js,v  <--  test_nsIMsgFolderListener.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js,v
done
Checking in mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js;
/cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js,v  <--  test_nsIMsgFolderListenerLocal.js
initial revision: 1.1
done
Checking in mailnews/base/util/nsMsgUtils.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.cpp,v  <--  nsMsgUtils.cpp
new revision: 1.149; previous revision: 1.148
done
Checking in mailnews/base/util/nsMsgUtils.h;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.h,v  <--  nsMsgUtils.h
new revision: 1.73; previous revision: 1.72
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.820; previous revision: 1.819
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.591; previous revision: 1.590
done
RCS file: /cvsroot/mozilla/mailnews/test/data/bugmail10,v
done
Checking in mailnews/test/data/bugmail10;
/cvsroot/mozilla/mailnews/test/data/bugmail10,v  <--  bugmail10
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/test/data/bugmail11,v
done
Checking in mailnews/test/data/bugmail11;
/cvsroot/mozilla/mailnews/test/data/bugmail11,v  <--  bugmail11
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/test/data/draft1,v
done
Checking in mailnews/test/data/draft1;
/cvsroot/mozilla/mailnews/test/data/draft1,v  <--  draft1
initial revision: 1.1
done

Comment 33

[Rain [:rain1] (inactive)]

Attachment: [checked in] fix CopyMessagesOffline to delete messages after the notification has been sent

So you can't send the notification after the message has been deleted from the source DB.

Comment 34

[Rain [:rain1] (inactive)]

Attachment: handle IMAP delete model moves, which are actually copies, not moves

Comment 35

[Rain [:rain1] (inactive)]

Attachment: changed the UUID at the right place

Sorry, I didn't realise there were two interfaces defined in this idl file.

Comment 36

[[e-mail]]

Comment on attachment 327085 [details] [diff] [review]
changed the UUID at the right place

>-  nsresult GetImapIncomingServer(nsIImapIncomingServer **aImapIncomingServer);
You'll need to change the implementation of this from nsresult to NS_IMETHODIMP

Comment 37

[Rain [:rain1] (inactive)]

Attachment: changed from nsresult to NS_IMETHODIMP

Thanks for catching that, Neil.

Comment 38

[David :Bienvenu]

Comment on attachment 327091 [details] [diff] [review]
changed from nsresult to NS_IMETHODIMP

+  // IMAP delete model means that a "move" isn't really a move, it is a copy.

a copy followed by storing the imap deleted flag on the message.

Comment 39

[Rain [:rain1] (inactive)]

Attachment: [checked in] added comment

(carrying forward r+sr from bienvenu)

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 326862 [details] [diff] [review]
[checked in] fix CopyMessagesOffline to delete messages after the notification has been sent

Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.823; previous revision: 1.822
done

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 327124 [details] [diff] [review]
[checked in] added comment

Checking in mailnews/base/public/nsIMsgFolderListener.idl;
/cvsroot/mozilla/mailnews/base/public/nsIMsgFolderListener.idl,v  <--  nsIMsgFolderListener.idl
new revision: 1.5; previous revision: 1.4
done
Checking in mailnews/base/src/nsMsgFolderNotificationService.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgFolderNotificationService.cpp,v  <--  nsMsgFolderNotificationService.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in mailnews/imap/public/nsIMsgImapMailFolder.idl;
/cvsroot/mozilla/mailnews/imap/public/nsIMsgImapMailFolder.idl,v  <--  nsIMsgImapMailFolder.idl
new revision: 1.38; previous revision: 1.37
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.824; previous revision: 1.823
done
Checking in mailnews/imap/src/nsImapMailFolder.h;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v  <--  nsImapMailFolder.h
new revision: 1.255; previous revision: 1.254
done