406921 - Address book listener on nsIAddrBookSession.added notify flag receives multiple "onItemAdded" calls

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Siva]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20061201 Firefox/2.0.0.6 (Ubuntu-feisty)
Build Identifier: MOZ_CO_DATE="22 Nov 2007 00:00 PDT"

The address book directory structure I have is:
Parent Addr Book  
  -> List1
  -> List2

I added an address book listener on nsIAddrBookSession.added notify flag. When I add a card into "Parent Addr Book", "onItemAdded" function is called 3 times, with parent directory set to each one of "Parent Addr Book", "List1", "List2". 
Since the card is added only to "Parent Addr Book", "onItemAdded" notification should be done only for "Parent Addr Book" and not for the lists under it.

In the case of removal, when a card is removed from "Parent Addr Book", it is removed from "List1" (to which it belongs to). But the "onItemRemoved" is called 3 times including "List2" also as parent directory.

Cause:
------
In nsAddrDatabase::NotifyCardEntryChange, OnCardEntryChange() call is made on all the listeners. The mailing lists also register themselves as listeners to the database (nsAbMDBDirectory::GetAbDatabase()).
May be at nsAbMDBDirectory::OnCardEntryChange, the parent directory of the card should be verified before calling the Notify functions.

I can make corresponding code changes, if somebody confirms the expected behavior of notifications. 

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> The address book directory structure I have is:
> Parent Addr Book  
>   -> List1
>   -> List2
> I added an address book listener on nsIAddrBookSession.added notify flag. When
> I add a card into "Parent Addr Book", "onItemAdded" function is called 3 times,
> with parent directory set to each one of "Parent Addr Book", "List1", "List2". 

I've not tested this explicitly yet, but from bugs like bug 134590 and a quick look at the code, I'm not surprised.

> Cause:
> ------
> In nsAddrDatabase::NotifyCardEntryChange, OnCardEntryChange() call is made on
> all the listeners. The mailing lists also register themselves as listeners to
> the database (nsAbMDBDirectory::GetAbDatabase()).
> May be at nsAbMDBDirectory::OnCardEntryChange, the parent directory of the card
> should be verified before calling the Notify functions.
> I can make corresponding code changes, if somebody confirms the expected
> behavior of notifications. 

From what you've described, I think that your idea is reasonable, and what you expect is correct. It may help you to know that bug 134590 comment 29 has some test cases that I wrote for checking the notification system of the address book (we don't meet those test cases at the moment).

We have started work on a new sql back-end for the address book, which may obsolete this bug. However I think it would still be worth attempting to fix it here on the current code, as it will solidify what is really expected through the listeners. It would also mean that it is fixed if we don't get the new sql back-end fully operational by the new release.

So I'd really like it if you could work on this, as I think it has benefits whatever happens. I'm happy to try and anwser any questions that you may have as well.

Comment 2

[Siva]

Attachment: Proposed Fix

Please note that this patch contains only the changes which are directly affecting the notification mechanism. There might be more changes required which would be done after the approach is confirmed.

Currently, I changed nsIAddrDBAnnouncer.idl & nsIAddrDBListener.idl to include "in nsIAbDirectory parent" argument in the notification calls (notifyCardEntryChange & onCardEntryChange). Correspondingly changed nsIAddrDatabase.idl to include the "parent" argument into all Create, Edit & Delete functions.  

In nsAbMDBDirectory::OnCardEntryChange(abCode, card, parent), it is verified if the dirName of "parent" and of "this" are the same. Only when they are same, futher Notify* calls are made. If "parent" is not set, it proceeds in the default behaviour. 

With this patch, I did the tests 1-5 listed at [https://bugzilla.mozilla.org/show_bug.cgi?id=134590#c29]. Only 2) failed, the view does not get refreshed. After switching to other addr book and coming back, the new card is visible.

Comment 3

[Siva]

> it is verified if the dirName of "parent" and of "this" are the same.
Since the comparison will happen only among the listeners of a single database, and its not possible to have same name for the address book and(or) its mailing list(s), this check should work fine. 

Comment 4

[Magnus Melin [:mkmelin]]

Marking new since we have a patch. (If it's ready for review, ask review by setting the r? flag on the attachment to the email of a suitable reviewer, like Mark.)

Comment 5

[Prasad Sunkari [:prasad]]

Siva, could you post a unified diff with a little more context.  The patch that you posted does not have any context!

Since all the files you changed are in mailnews, you could use 'cvs diff -u8p mailnews' to generate a good patch. Also when posting a patch you should set the review flag to "?" and type in the email address of the person who should review it (http://www.mozilla.org/owners.html) - This is addressbook so, it should be the email address of Mark Banner.

Comment 6

[Siva]

Attachment: Patch with context

Mark, can you review this?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 291918 [details] [diff] [review]
Patch with context

Firstly, thanks for the patch, its looking good and I think the basic idea is definitely sound, just needs a few improvements.

 [scriptable, uuid(0d8fe8e3-ad48-4dbd-b0c2-a1d374e39b93)] 

In all the idl files where you change functions/params etc you need to modify the uuid (see http://developer.mozilla.org/en/docs/Generating_GUIDs for more info). This helps extensions track what's changed.

Also please replace tabs by spaces on the lines you've changed. The general rule is two space indentation, except where four space helps to keep the look of the file better.

+  // It is not possible to create multiple address books/mailing lists
+  // with same name. So dirName can be used to match the listener
+  // call.
+  // If parent is set, 
+  //  check if the parent dirName and this dirName are same.
+  if (parent) {
+    nsString dirName, curDirName;
+    parent->GetDirName(dirName);
+    this->GetDirName(curDirName);
+
+    if (strcmp(NS_ConvertUTF16toUTF8(dirName).get(), NS_ConvertUTF16toUTF8(curDirName).get()))
+      return NS_OK;
+  }
+

Rather than doing this, you can just do:

if (parent != this)
  return NS_OK;

Although dirName is supposed to be unique, there are ways around that (and import especially allows it to be non-unique at the moment).

Please also put that if statement before the do_QueryInterface line - there's no point in doing the QI if we aren't going to use it.

nsIAddrDatabase also gets used in other places, e.g the palmsync extension http://mxr.mozilla.org/seamonkey/search?string=CreateNewCardAndAddToDB

and import:

http://mxr.mozilla.org/seamonkey/search?string=AddListCardColumnsToRow&find=&findi=&filter=&tree=seamonkey

I'm guessing you're building on linux or mac which is why your compiler didn't pick them up.

I did a bit of investigation about the second test case. I've got a debug build and when I try and add the card to the list I get:

WARNING: NS_ENSURE_SUCCESS(err, err) failed with result 0x80004002: file /mozilla/master/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp, line 2218

on the console. Looks like nsAddrDatabase::ContainsCard is failing because its not been passed an nsAbMDBCard (possibly got passed a nsAbCard instead?).

I did start to create a unit test for checking the notification system (which is why I think testcase 2 should work), would you like to extend it to take in all the cases for me? i.e check we get the right notifications during add/modify/remove. It doesn't need to check we get the right updates on the screen at this stage, just that the notifications from nsIAddrBookSession seem right (if not, I'll see if I can get time to do it this weekend).

Comment 8

[Mark Banner (:standard8)]

I think this bug will fix bug 134590 (and possibly others), which are all to do with mailing list add/remove display updates. Hence adding to the wanted-tb3.0a1 radar and taking. It'd be good to do this before bug 413260 as it'll enable us to have some more test cases to check we don't break things.

Comment 9

[Siva]

Attachment: Patch file including corrections in c7 and more fixes

With this patch, all cases in https://bugzilla.mozilla.org/show_bug.cgi?id=134590#c29 are tested OK. 
Apart from create/add cases, delete cases as below also tested OK, except Case 14).
Case 10) Delete card from address book with address book selected.
Case 11) Delete card from address book with mailing list selected.
Case 12) Delete card from mailing list with address book selected.
Case 13) Delete card from mailing list with mailing list selected.
Case 14) Delete card from mailing list using properties window with mailing list selected.
Case 15) Delete card from mailing list using properties window with address book selected.

In case 14), there is no delete notification generated since the 'nsAddrDatabase::AddListAttributeColumnsToRow' does not check for deleted cards in the mailing list. It seems the addresses are not actually deleted from the 'listRow'. Only the total number of addresses is set by 'SetListAddressTotal()'

Comment 10

[Siva]

Attachment: Patch file including corrections in c7 and more fixes, with context

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #9)
> Created an attachment (id=316407) [details]
> Patch file including corrections in c7 and more fixes

I think I've found another case, but I'm a bit surprised. With a mailing list selected, open an empty mailing list, add an email address, click ok and then you get the results view with two copies of that card rather than one.

Can you confirm this? We still might go for this patch initially at least - it appears that it'll be much better than what we have currently.

Comment 12

[Siva]

Attachment: Patch file to remove AddListener call from nsAbMDBDirectory::AddMailList

Yes, the problem exists. It seems to be because of the following code in nsAbMDBDirectory.cpp.

673     nsCOMPtr<nsIAddrDBListener> listener(do_QueryInterface(newList, &rv));
674     NS_ENSURE_SUCCESS(rv, rv);
675 
676     rv = mDatabase->AddListener(listener);
677     NS_ENSURE_SUCCESS(rv, rv);

I tried to check why the newly created directory has to be added as listener in AddMailList() call itself? It gets added as listener whenever the mail list is selected or a search is done.

After commenting this bit of code, the duplicates were not seen. I did some testing in adding cards/lists and those cases were also working fine.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> Created an attachment (id=316842) [details]
> Patch file to remove AddListener call from nsAbMDBDirectory::AddMailList
> 
> Yes, the problem exists. It seems to be because of the following code in
> nsAbMDBDirectory.cpp.

Thanks for that, I've actually put this change into bug 406921 as it was coming anyway, and I've finally decided it is the right change to solve that bug. I'll review your first patch again now that is done.

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> Thanks for that, I've actually put this change into bug 406921 as it was coming
> anyway, and I've finally decided it is the right change to solve that bug. I'll
> review your first patch again now that is done.
> 

Sorry, any chance you could update your patch to latest-trunk? There appears to be some bitrot caused by recent changes.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #13)
> Thanks for that, I've actually put this change into bug 406921 as it was coming

That should be bug 424570

Comment 16

[Siva]

Attachment: Patch file for latest trunk

Patch for latest trunk. 
Ignored the changes to remove "AddListener" call from "nsAbMDBDirectory::AddMailList.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> Created an attachment (id=317025) [details]
> Patch file for latest trunk

Thanks

> Ignored the changes to remove "AddListener" call from
> "nsAbMDBDirectory::AddMailList.

As you'll probably have seen already, the AddMailList change is now checked in on trunk.

(In reply to comment #9)
> In case 14), there is no delete notification generated since the
> 'nsAddrDatabase::AddListAttributeColumnsToRow' does not check for deleted cards
> in the mailing list. It seems the addresses are not actually deleted from the
> 'listRow'. Only the total number of addresses is set by 'SetListAddressTotal()'

I have just been playing around with the patch and I've verified it the same as you have. I'm happy that the change with this patch far outweighs the one minor problem that remains, I think it'll also give a much better basis for fixing that problem as well.

So, in short its looking good. I'll take a more detailed look through now and finish the review.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 317025 [details] [diff] [review]
Patch file for latest trunk

This is looking good, there's a few comments that I'd like addressed though (see below).

Would you be interested in helping expand our unit tests with some to test this code so that we don't regress it? See http://wiki.mozilla.org/MailNews:Xpcshell_tests for more info. Note that writing unit tests are not required for us to get this patch into the code base, and I'd like to get this patch in as soon as possible. Again I'm willing to give you help - I'd really like to see some tests for this to prevent regressions especially with the other changes we've got coming up.

>Index: mailnews/addrbook/public/nsIAddrDBAnnouncer.idl

> 	void notifyCardEntryChange (in unsigned long abCode, 
>-								in nsIAbCard card);
>+								in nsIAbCard card,
>+								in nsIAbDirectory parent,
>+								in boolean notifyParentOnly);

Please could you remove the tabs from these lines, and make it so the void line is 2-space indented, and the rest of the "in"s line up vertically.

Ditto with the tabs in the other idl files that you touch.


Please could you ensure that all the idl functions that you change the footprints of are documented in a similar fashion to http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/public/nsIAbManager.idl&rev=1.52&mark=65-82#65

Especially of interest is the fact that parent may be null. If you're not sure of what to say for them, ping me on irc.

>Index: mailnews/addrbook/src/nsAbMDBDirectory.cpp

>+NS_IMETHODIMP nsAbMDBDirectory::ContainsDirectory(nsIAbDirectory *parent, nsIAbDirectory *directory, PRBool *containsDir)

NS_IMETHODIMP should only be used for methods that implement functions in interfaces (idl). So this should just be nsresult. However, I think you could just return PRBool from this function and then you don't need to pass *containsDir.

>+{
>+  nsresult rv = NS_OK;
>+
>+  // If parent is a maillist, 'addressLists' contains AbCards.
>+  PRBool bIsMailList = PR_FALSE;
>+  rv = parent->GetIsMailList(&bIsMailList);

Please make this line nsresult rv = parent... and drop the earlier declaration.

>+  NS_ENSURE_SUCCESS(rv,rv);

nit: missing space

> NS_IMETHODIMP nsAbMDBDirectory::OnCardEntryChange
>-(PRUint32 abCode, nsIAbCard *card)
>+(PRUint32 abCode, nsIAbCard *card, nsIAbDirectory *parent)
> {
>   NS_ENSURE_ARG_POINTER(card);
>   nsCOMPtr<nsISupports> cardSupports(do_QueryInterface(card));
>   nsresult rv;
>+  PRBool isChild = PR_FALSE;
>+
>+  // Notify when
>+  // - any operation is done to a card belonging to this 
>+  //        => if <this> is <parent>, or
>+  // - a card belonging to a directory which is parent of this is deleted
>+  //        => if abCode is AB_NotifyDeleted && <this> is child of <parent>
>+  // - a card belonging to a directory which is child of this is added/modified
>+  //        => if abCode is !AB_NotifyDeleted && <this> is parent of <parent>, or
>+
>+  if (parent != this)
>+  {
>+    if (abCode != AB_NotifyDeleted)
>+    {
>+      nsCOMPtr<nsIAbDirectory> thisDir(do_QueryInterface((nsIAbMDBDirectory *)this, &rv)); 
>+      rv = this->ContainsDirectory(thisDir, parent, &isChild);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+    else
>+    {
>+      nsCOMPtr<nsIAbDirectory> thisDir(do_QueryInterface((nsIAbMDBDirectory *)this, &rv)); 
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = this->ContainsDirectory(parent, thisDir, &isChild);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+
>+    if (!isChild)
>+      return NS_OK;
>+  }

So I think you could simplify some of this at least (if you also include the PRBool return change I mentioned above):

if (parent != this)
{
  nsCOMPtr<nsIAbDirectory> thisDir(do_QueryInterface((nsIAbMDBDirectory *)this, &rv));
  NS_ENSURE_SUCCESS(rv, rv);

  PRBool isChild;
  if (abCode != AB_NotifyDeleted)
    isChild = this->ContainsDirectory(thisDir, parent);
  else
    isChild = this->ContainsDirectory(parent, thisDir);
}

You could almost use the ? : construct for the ContainsDirectory call, but then I think that might force the compiler into two if comparisons rather than one.

>Index: mailnews/addrbook/src/nsAddrDatabase.cpp

>-NS_IMETHODIMP nsAddrDatabase::NotifyCardEntryChange(PRUint32 abCode, nsIAbCard *card)
>+NS_IMETHODIMP nsAddrDatabase::NotifyCardEntryChange(PRUint32 abCode, nsIAbCard *card, nsIAbDirectory *parent, PRBool notifyParentOnly)
> {
>-  NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS(m_ChangeListeners, nsIAddrDBListener,
>-                                     OnCardEntryChange, (abCode, card));
>+  nsTObserverArray<nsIAddrDBListener *>::ForwardIterator iter(m_ChangeListeners);
>+  nsCOMPtr<nsIAddrDBListener> observer;
>+  while (iter.HasMore()) {
>+    observer = iter.GetNext();
>+
>+    if (notifyParentOnly) 
>+    {
>+      //Call OnCardEntryChange only on the parent.
>+      nsresult err = NS_OK;
>+      nsCOMPtr<nsIAbDirectory> observerDir(do_QueryInterface(observer, &err));
>+      NS_ENSURE_SUCCESS(err, err);
>+
>+      if (observerDir == parent)
>+        observer->OnCardEntryChange(abCode, card, parent);
>+    } 
>+    else
>+      observer->OnCardEntryChange(abCode, card, parent);
>+  }
>   return NS_OK;
> }

So, in some cases you want to notify only the parent (if its in the list). I'm not sure about this, but it seems to work. Let's make it a bit easier to understand, how about:

if (notifyParentOnly)
{
  nsresult rv;
  nsCOMPtr<nsIAddrDBListener> parentListener(do_QueryInterface(parent, &rv));
  NS_ENSURE_SUCCESS(rv, rv);

  // Ensure the parent is in the listener list (and hence wants to be notified)
  if (m_ChangeListeners.Contains(parentListener))
    parentListener->OnCardEntryChange(abCode, card, parent);
}
else
  NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS(m_ChangeListeners, nsIAddrDBListener,
                                     OnCardEntryChange, (abCode, card, parent));

(btw, rv is preferred to err).

Comment 19

[Siva]

Attachment: Patch with comments and other changes

Changed function arguments to have "a" at the start.
Implemented changes suggested in c18.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 318133 [details] [diff] [review]
Patch with comments and other changes

+  // Dont notify AbManager unless we have the parent
+  NS_ENSURE_ARG_POINTER(aParent);

nit: you missed the ' in don't.

r=me with that fixed.

Comment 21

[Siva]

Attachment: Fixed the comment in c20

Neil, can you superreview this patch? r=standard8

Comment 22

[[e-mail]]

Comment on attachment 318148 [details] [diff] [review]
Fixed the comment in c20

>+PRBool nsAbMDBDirectory::ContainsDirectory(nsIAbDirectory *parent, nsIAbDirectory *directory)
This function only uses its parameters, so it should be marked static. (You have two options: mark it static here, and remove it from the .h file, or mark it static in the .h file.)

>+  // Don't notify AbManager unless we have the parent
>+  NS_ENSURE_ARG_POINTER(aParent);
According to an earlier comment a null parent is expected, so in this case you don't want to warn on the console, and indeed you might want to return NS_OK.

>+  // Notify when
>+  // - any operation is done to a card belonging to this 
>+  //        => if <this> is <aParent>, or
>+  // - a card belonging to a directory which is parent of this is deleted
>+  //        => if aAbCode is AB_NotifyDeleted && <this> is child of <aParent>
>+  // - a card belonging to a directory which is child of this is added/modified
>+  //        => if aAbCode is !AB_NotifyDeleted && <this> is parent of <aParent>, or
I think you switched two lines around, and forgot to move ", or" back.

>+    nsCOMPtr<nsIAbDirectory> thisDir(do_QueryInterface((nsIAbMDBDirectory *)this, &rv)); 
>+    NS_ENSURE_SUCCESS(rv, rv);
You don't need to do that, just pass "this" to ContainsDirectory

>+      isChild = this->ContainsDirectory(thisDir, aParent);
>+    else
>+      isChild = this->ContainsDirectory(aParent, thisDir);
You don't need to qualify the call to ContainsDirectory (and in fact you can't once you mark it as static).

>+  if (aNotifyParentOnly) 
Nit: trailing space (I've not checked every line, there's a bot for that!)

>+      NotifyCardEntryChange(AB_NotifyInserted, aPCard, aParent, PR_TRUE);
It seems a waste to have that final parameter when it only gets used once... what's wrong with simply writing that special case out here?

Comment 23

[Siva]

Attachment: Patch with changes for c22

(In reply to comment #22)
All changes except below are done.
> 
> >+      NotifyCardEntryChange(AB_NotifyInserted, aPCard, aParent, PR_TRUE);
> It seems a waste to have that final parameter when it only gets used once...
> what's wrong with simply writing that special case out here?
> 

If we want to have another function for this case, there will be two functions for "NotifyCardEntryChange" in the idl also. It seems better to have one function in the idl & cpp and have a flag for the callers.

Comment 24

[Mark Banner (:standard8)]

Siva's the one doing the work on this...

Comment 25

[[e-mail]]

(In reply to comment #23)
> (In reply to comment #22)
> > >+      NotifyCardEntryChange(AB_NotifyInserted, aPCard, aParent, PR_TRUE);
> > It seems a waste to have that final parameter when it only gets used once...
> > what's wrong with simply writing that special case out here?
> If we want to have another function for this case, there will be two functions
> for "NotifyCardEntryChange" in the idl also. It seems better to have one
> function in the idl & cpp and have a flag for the callers.
No, I didn't say that. Just move the code for the special case into the caller.

Comment 26

[Siva]

Attachment: Removed extra argument to NotifyCardEntryChange

Moved the special condition handling to the caller. NotifyCardEntryChange does not have a fourth argument now.

Comment 27

[[e-mail]]

Comment on attachment 320906 [details] [diff] [review]
Removed extra argument to NotifyCardEntryChange

sr=me

Comment 28

[Siva]

Thanks Neil. 

Can someone commit for me?
r=standard8
sr=neil

Comment 29

[Mark Banner (:standard8)]

Checking in mailnews/addrbook/public/nsIAddrDBAnnouncer.idl;
/cvsroot/mozilla/mailnews/addrbook/public/nsIAddrDBAnnouncer.idl,v  <--  nsIAddrDBAnnouncer.idl
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/addrbook/public/nsIAddrDBListener.idl;
/cvsroot/mozilla/mailnews/addrbook/public/nsIAddrDBListener.idl,v  <--  nsIAddrDBListener.idl
new revision: 1.9; previous revision: 1.8
done
Checking in mailnews/addrbook/public/nsIAddrDatabase.idl;
/cvsroot/mozilla/mailnews/addrbook/public/nsIAddrDatabase.idl,v  <--  nsIAddrDatabase.idl
new revision: 1.54; previous revision: 1.53
done
Checking in mailnews/addrbook/src/nsAbLDAPReplicationData.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbLDAPReplicationData.cpp,v  <--  nsAbLDAPReplicationData.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in mailnews/addrbook/src/nsAbMDBDirectory.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,v  <--  nsAbMDBDirectory.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in mailnews/addrbook/src/nsAddrDatabase.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,v  <--  nsAddrDatabase.cpp
new revision: 1.165; previous revision: 1.164
done
Checking in mailnews/addrbook/src/nsAddrDatabase.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsAddrDatabase.h,v  <--  nsAddrDatabase.h
new revision: 1.73; previous revision: 1.72
done
Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.561; previous revision: 1.560
done
Checking in mailnews/extensions/palmsync/src/nsAbPalmSync.cpp;
/cvsroot/mozilla/mailnews/extensions/palmsync/src/nsAbPalmSync.cpp,v  <--  nsAbPalmSync.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in mailnews/import/oexpress/nsOEAddressIterator.cpp;
/cvsroot/mozilla/mailnews/import/oexpress/nsOEAddressIterator.cpp,v  <--  nsOEAddressIterator.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in mailnews/import/outlook/src/nsOutlookMail.cpp;
/cvsroot/mozilla/mailnews/import/outlook/src/nsOutlookMail.cpp,v  <--  nsOutlookMail.cpp
new revision: 1.48; previous revision: 1.47
done

Comment 30

[Siva]

Thanks Standard8 for checking-in.

Comment 31

[Mark Banner (:standard8)]

I've raised bug 434014 on the one remaining issue (comment 9, case 14).

Comment 32

[Wayne Mery (:wsmwk)]

This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.