1682620 - Fix some issues with the Outlook address book (card add/edit/delete)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[IT Support]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

We've taken a brief look at operations on the Outlook address book.

Deleting/Editing a card works, but the view isn't updated (similar to bug 465080). Maybe related to bug 1633998 and 1633607, or it never worked.

Adding a card doesn't work:
NS_ENSURE_SUCCESS(retCode, retCode) failed with result 0x80004005 (NS_ERROR_FAILURE): file c:/mozilla-source/comm-central/comm/mailnews/addrbook/src/nsAbOutlookDirectory.cpp:330

Adding a mailing list doesn't work:
NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file c:/mozilla-source/comm-central/comm/mailnews/addrbook/src/nsAbOutlookDirectory.cpp:236

We haven't looked at mailing list operations yet.

Comment 1

[IT Support]

(Can't the comments be edited to fix typos and omissions?) Above it should have been: Maybe related to bug 1633998 and bug 1633607, ...

Geoff, what is the right way to notify edit/delete/add of cards (once add works)?

Comment 2

[IT Support]

When adding a card, this call to GetProps() fails with MAPI_W_ERRORS_RETURNED, 0x00040380:
https://searchfox.org/comm-central/rev/b97629284ff9f582b11ab6b7a983356730ddb850/mailnews/addrbook/src/nsAbWinHelper.cpp#584
Since we only asked for one property, this must be in error. Ignoring the error leads to a failure on the next call CreateEntry(). There's a lot of joy (not!) to be had in MAPI programming, understandable that this has been neglected. Just puzzling whether this ever worked and under which circumstances. Creating a mailing list will likely have the same issue at line 648.

Comment 3

[Geoff Lankow (:darktrojan)]

I think you'd qualify for more permissions by now, so go ahead and apply: https://wiki.mozilla.org/BMO/UserGuide#Permissions_and_Groups

Notification is done by the observer service (example) and you can see all of the address book notifications we use here. At this point you'd be interested in "addrbook-contact-created", "addrbook-contact-updated", and "addrbook-contact-deleted", all of which take the nsIAbCard as the first argument and the owning directory's UID as the third argument. You should also implement "addrbook-contact-properties-updated" to keep some listeners happy.

Comment 4

[Geoff Lankow (:darktrojan)]

Perhaps this test would be a better reference: https://searchfox.org/comm-central/search?q=checkEvents&path=test_jsaddrbook.js
(Beware that the first and second arguments are not in the usual order here.)

Comment 5

[IT Support]

Attachment: 1682620-notification.patch - WIP

This works for edit and delete, thanks for the instructions. Getting card creation to work will take longer, so this was an easy win.

We noticed that the notification is called for cards and lists, so what needs to be done for mailing lists? Also, is there no API to modify a mailing list?

Can you also please clarify the "addrbook-contact-properties-updated" bit.

Comment 6

[Geoff Lankow (:darktrojan)]

(In reply to IT Support from comment #5)

We noticed that the notification is called for cards and lists, so what needs to be done for mailing lists? Also, is there no API to modify a mailing list?

Mailing lists are weird. They behave as both nsIAbCard and nsIAbDirectory depending on the circumstance. For what you're doing they're instances of nsAbOutlookDirectory with m_IsMailList set to true. (In the JS implementation I've pulled it out into its own class for clarity. And hopefully someday a much less confusing interface.)

Cards are added or removed using the nsIAbDirectory methods, but you'll need to send the addrbook-list-member-* notifications instead of the addrbook-contact-* ones.

I think you'll find that where a directory is passed to the notification methods that the directory is a mailing list and you'll have to send the addrbook-list-created/deleted notification. EditMailListToDatabase should fire addrbook-list-updated.

Can you also please clarify the "addrbook-contact-properties-updated" bit.

This is sent as well as addrbook-contact-updated to list the properties that changed. The data value is a JSON-encoded string of the names and values that changed, e.g.:

{
  "DisplayName": {
    "oldValue": "a new contact",
    "newValue": "updated contact"
  }
}

Use null where there wasn't a value or is no longer a value. I'm not sure what JSON utilities we have for C++ so I can't offer any advice on that.

Comment 7

[IT Support]

Comment on attachment 9193388 [details] [diff] [review]
1682620-notification.patch - WIP

Thanks. Writing JSON in C++ code is no problem:
https://searchfox.org/mozilla-central/source/mfbt/JSONWriter.h
Writing into a string:
https://searchfox.org/mozilla-central/rev/c7cf087b6e1384608ca3989f042f12f7cabd0a5f/ipc/mscom/RegistrationAnnotator.cpp#21-33

Comment 8

[IT Support]

We've now looked at mailing lists. Lo and behold, they "sort of work". When we access an contact in a mailing list, we run into:
JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 571: TypeError: can't access property "URI", directory is null

As we wrote in comment #0, trying to add a mailing list gives:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file c:/mozilla-source/comm-central/comm/mailnews/addrbook/src/nsAbOutlookDirectory.cpp:238, or on line 235 in the committed codebase:
https://searchfox.org/comm-central/rev/2dcfb56805811d2666aab8af0ddf03d049f10fc4/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#235

So it looks like the directories/mailing lists don't have a URI. Any hints for this?

Comment 9

[IT Support]

Attachment: 1682620-notification.patch

Sadly 90% of the time here was spent working out that although a birthday is clearly set in Outlook, the retrieval fails via MAPI (and there was a bug). If we used the old "defective" value to compare with, we'd issue a change notification every time if the someone sets/changes that date. We might as well remove the birthday code altogether or leave it as it is for further investigation.

Open to suggestions. My suggesting here would be to do this in parts: First the modification, then the addition, sigh, and then the mailing lists. There's an open question from the previous comment re. their UID.

The JSON stuff comes out like this (real data, not made up):

{
 {
  "FirstName": {
   "oldValue": "santaa",
   "newValue": "santa"
  },
  "LastName": {
   "oldValue": "clauss",
   "newValue": "claus"
  }
 }
}

Comment 10

[IT Support]

Comment on attachment 9193643 [details] [diff] [review]
1682620-notification.patch

Review of attachment 9193643 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbWinHelper.cpp
@@ +388,5 @@
>    if (valueCount == 1 && values != NULL &&
> +      // The type is in the lower two bytes of the tag.
> +      // Mostly we get 0x000A (PT_ERROR==10) instead of the desired 0x0040
> +      // (PT_SYSTIME==64). See MAPIDefS.h and MAPITags.h for details.
> +      (PROP_TYPE(values->ulPropTag) & 0xFFFF) == PT_SYSTIME) {

Damn, there is a PROP_TYPE() which will do the &0xFFFF. Anyway, it returns PT_ERROR all the time :-(

Comment 11

[IT Support]

OK, getting all those properties from MAPI appears to be wishful thinking. At least the Nickname and the personal/home webpage are also lost after setting them. To be continued.

Comment 12

[IT Support]

Attachment: 1682620-fix-logging.patch

Before we do anything else, let's fix the conversion specifiers and improve the ability to debug this. We'll resubmit the other patch.

Comment 13

[IT Support]

Attachment: Part 1: 1682620-fix-logging.patch

Reformatted.

Comment 14

[IT Support]

With the patch we're about to submit, we see these errors for MAPI properties which can't be retrieved:
Unexpected return value for property 3a4f000a (x0A is PT_ERROR). --> PR_NICKNAME
Unexpected return value for property 3a50000a (x0A is PT_ERROR). --> PR_PERSONAL_HOME_PAGE
Unexpected return value for property 3004000a (x0A is PT_ERROR). --> PR_COMMENT
Cannot retrieve PT_SYSTIME property 3a42000a (x0A is PT_ERROR). --> PR_BIRTHDAY
That matches our earlier comment.

Comment 15

[IT Support]

Attachment: 1682620-notification.patch

This does the trick. We needed to improve error handling to be able to send meaningful notifications. And there are little issues everywhere, like not stripping the CR when splitting the address :-(

Comment 16

[IT Support]

Attachment: 1682620-notification.patch

Use .IsEmpty() instead of .get()[0] to check for empty string. Still learning Mozilla strings.

Comment 17

[IT Support]

Attachment: 1682620-notification.patch

While looking at adding a new contact we noticed that the error handling needed more adjustments. Sorry about the stream of review requests/cancellations.

Comment 18

[IT Support]

To fix the issue mentioned in comment #2, we've searched far and wide for examples for creating new contacts in the Outlook address book via MAPI and didn't find anything. That's different to bug 1681824 where some sample code led to the search via PR_ANR. So we need to see what the forums yield. Here's the first post in the MFCMAPI Github project:https://github.com/stephenegriffin/mfcmapi/issues/427. The sample code supplied there does all sorts of things including PR_ANR restrictions, but no contact creation.

Also note the the heading "MAPI - Not dead yet" at https://docs.microsoft.com/en-us/archive/blogs/stephen_griffin/ (the guy from the MAPI Github project). That's not totally encouraging.

Comment 19

[IT Support]

https://github.com/stephenegriffin/mfcmapi/issues/427 and https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=ind2012&L=MAPI-L&P=R265 have the yielded the following result: Adding a card via PR_DEF_CREATE_MAILUSER is no longer supported by "modern" versions of Outlook, so adding a card/mailing list would have to be completely reimplemented.

In bug 1681824 comment #28 your chief engineer is stating that the "dinosaur" of access to the Outlook address book via MAPI may be removed altogether although it's been decades in the making, incl. bug 459956. That seems to contradict bug 1680952 comment #7.

Please let us know how to proceed here. Implementing/fixing things which will ultimately be removed is not the best use of our time.

Comment 20

[IT Support]

Attachment: 1682620-notification.patch

Fixed logic error in GetPropertiesUString() introduced during reshuffling.

Comment 21

[IT Support]

Attachment: 1682620-open-contact.patch - WIP

As stated in comment #14, there are problems accessing some properties. We got some help from some people on the [e-mail] mailing list, who BTW also gave us a solution to add a new card/contact. The summary is that the Outlook address book has underlying contacts which are stored elsewhere. So instead of opening the address book entry (class IMailUser), the contact (class IMessage, yes, the contact is stored in a message object) needs to be opened. The patch shows a bit of this in GetMAPIProperties(). Turns out that the contact can't be opened without opening its message store first. There is quite a bit of work involved to get access to the missing properties. As we said in comment #19, there's no point implementing something that's not going to be accepted or going to be removed.

BTW, Outlook import has some code for message stores here:
https://searchfox.org/comm-central/search?q=CMsgStore&path=&case=false&regexp=false
but the address book code has not.

Comment 22

[IT Support]

Attachment: 1682620-open-contact.patch - WIP

This now opens the message store to get access to the contact, it now retrieves nickname and birthday. Turns out that the e-mail address in not stored on the contact and needs to be retrieved in the original way (not implemented yet). We also didn't test editing the properties. We rest our case now until we here from you. Since we were engaged in some discussion on the said mailing list, we wanted to drive it forward to the point where something was working. That's why we didn't stop earlier. The patch looks simple, but getting there wasn't.

Note to self: In the future when all properties can be retrieved, failure to fetch one should be interpreted as null value of the notification, so this code can be removed:

+    rv = aOld->GetPropertyAsAUTF8String(CardStringProperties[i], oldValue);
+    if (NS_FAILED(rv)) continue;
+    rv = aOld->GetPropertyAsUint32(CardIntProperties[i], &oldValue);
+    if (NS_FAILED(rv)) continue;

Comment 23

[IT Support]

Attachment: 1682620-open-contact.patch - WIP

Improved version. Retrieves email. Doesn't retrieve comment any more, it doesn't seem to be available on the address book entry or the associated contact. Writing back of modified fields not tested, will certainly not work for the birthday since that needs to be stored on the contact where it came from.

Comment 24

[IT Support]

Attachment: 1682620-open-contact.patch - WIP

Looking at bug 1684401 we realised that different Outlook address book directories behave differently. So the nsAbWinHelper needs to know which directory it is helping. The easiest way to do so is to pass the entry ID of the directory into the CTOR of the helper. That also fixes the ugly hack in the previous patch. We noted a few TODOs in the code. (Writing back of modified fields not tested, will certainly not work for the birthday since that needs to be stored on the contact where it came from.)

When can we expect a general decision of whether the Outlook address book is going to be supported in the future? Somehow this area has fallen very silent. Is everyone still on holidays?

Comment 25

[IT Support]

Attachment: 1682620-fix-edit-add-notifications.patch

OK, here is the final solution for edit. This patch fixes the edit, so all the properties which used to fail now work, including nick name, personal home page and birthday. Notifications are done as required.

What's missing now is fixing add card. We'll file another bug for the mailing lists.

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9194904 [details] [diff] [review]
1682620-fix-edit-add-notifications.patch

Review of attachment 9194904 [details] [diff] [review]:
-----------------------------------------------------------------

This looks okay to me (apart from the fact it's all ancient code using BOOL and NULL etc. – not a problem for this bug), but I'm going to get Ben to cast his more C++-experienced eye over it.

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +789,5 @@
> +  w.End();
> +
> +#if PRINT_TO_CONSOLE
> +  printf("%s", static_cast<CStringWriter*>(w.WriteFunc())->Get().get());
> +#endif

After the logging patch, you want PRINTF, surely.

Comment 27

[Geoff Lankow (:darktrojan)]

Comment on attachment 9193682 [details] [diff] [review]
Part 1: 1682620-fix-logging.patch

Review of attachment 9193682 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this review took an age.

Comment 28

[IT Support]

Thanks for the review. We'll carry on with the patch for fixing "add card" and file a new bug for the mailing lists.

(In reply to Geoff Lankow (:darktrojan) from comment #26)

#if PRINT_TO_CONSOLE
printf("%s", static_cast<CStringWriter*>(w.WriteFunc())->Get().get());
#endif

After the logging patch, you want PRINTF, surely.

No. Please remember that PRINTF is is going to the log if PRINT_TO_CONSOLE is 0 (zero). So we only want this for debugging purposes as print, not on the log.

NULL and BOOL are MS-types/defines, like HRESULT, BYTE and WORD, which are also used throughout this code.
https://docs.microsoft.com/en-us/cpp/c-runtime-library/null-crt
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/9d81be47-232e-42cf-8f0d-7a3b29bf2eb2

Comment 29

[IT Support]

Attachment: 1682620-kill-base64.patch

OK, adding a contact is not that easy, here are the instructions we received from the MAPI list:
https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2012&L=MAPI-L&D=0&P=20993268

In the meantime as an easy New Year's Day exercise, let's kill base64 in the entry-ID-based URIs. Yes, it saved a couple of bytes in memory back in 2010 and earlier, but everyone on the planet simply prints those byte strings. If you use tools like OutlookSpy or MFCMAPI, those base64-encoded entry IDs are not helpful.

Comment 30

[IT Support]

Attachment: 1682620-kill-base64.patch

Sorry, a little simpler.

Comment 31

[IT Support]

Attachment: 1682620-fix-add-card.patch - WIP

This implements part of https://peach.ease.lsoft.com/scripts/wa-PEACH.exe?A2=2012&L=MAPI-L&D=0&P=20993268

... grab the top OAB container or the root AB container
(returned from IAddrBook::OpenEntry(0, null, ...)), open the hierarchy table
(IABContainer:: GetHierarchyTable), and request PR_ENTRYID and
PR_CONTAB_FOLDER_ENTRYID for all rows using HrQueryAllRows.
Loop through all rows and compare the entry id of the IABContainer container
(PR_ENTRYID) with the PR_ENTRYID property in each row using IMAPISession::CompareEntryIDs.
Once you find the match, grab the PR_CONTAB_FOLDER_ENTRYID property from
that row and use it to call IMAPISession::OpenEntry - you will get back IMAPIFolder.

We ran into a link issue (error: undefined symbol: HrQueryAllRows), will ask on the list.

Any progress on the other patches?

Comment 32

[Geoff Lankow (:darktrojan)]

Comment on attachment 9195005 [details] [diff] [review]
1682620-kill-base64.patch

I've just noticed the user name on this patch. That's not right.

Comment 33

[IT Support]

Attachment: Part 2: 1682620-kill-base64.patch

Thanks, fixed that.

Comment 34

[IT Support]

Comment on attachment 9195482 [details] [diff] [review]
1682620-fix-add-card.patch - WIP

HrQueryAllRows is too hard to be made to work, so we've implemented an alternative approach. Turns out that the instructions from the mailing list don't yield the correct result, or we misinterpreted them. To be continued.

Comment 35

[IT Support]

Attachment: Part 2: 1682620-fix-edit-add-notifications.patch - WIP

Looking at this some more, the first edit works fine, but subsequent ones don't. We also found a copy/paste error in the previous patch (printing the wrong return value). Finally we decided to pass the directory into the property getter/setter and to rename the class member variable from the plain mMapiData to mDirEntry since it is the entry ID of the directory we're working on. To be continued.

Comment 36

[IT Support]

Attachment: Part 4: 1682620-fix-add-card.patch - WIP

This implements most of the instructions from the mailing list, see comment #31. We located the object/folder where we need to create the new contact. To be continued.

Comment 37

[IT Support]

Attachment: Part 2: 1682620-fix-edit-add-notifications.patch

This is a variation of the original "fix edit" patch (part 2) which received an r+ from Geoff already in comment #26. The variation is described in comment #35. We got it to work for multiple edits now :-) - Geoff, you might take another look here or let it go straight to Ben as per comment #26. Sorry about the many versions, MAPI programming isn't that much fun and solutions need to be researched. So with edit now fixed, add still needs to be fixed, and there is bug 1685166 for mailing lists.

Comment 38

[IT Support]

Attachment: Part 4: 1682620-fix-add-card.patch - WIP

Some more until we ran into the next snag.

Comment 39

[IT Support]

Attachment: Part 4: 1682620-fix-add-card.patch - WIP

We got to the next snag with this. Time to ask more questions on the mailing list. To be continued.

Comment 40

[IT Support]

Comment on attachment 9195496 [details] [diff] [review]
Part 2: 1682620-kill-base64.patch

This clean-up patch can be part 2.

Comment 41

[IT Support]

Attachment: Part 3: 1682620-fix-add-edit-add-notifications.patch

We concluded the work on adding a card and merged everything into one patch since there were various dependencies.

The key to understanding the modifications is this, which we also documented in the code:

// Microsoft distinguishes between address book entries and contacts.
// Address book entries are of class IMailUser and are stored in containers
// of class IABContainer.
// Local contacts are stored in the "contacts folder" of class IMAPIFolder and
// are of class IMessage with "message class" IPM.Contact.
// For local address books the entry ID of the contact can be derived from the
// entry ID of the address book entry and vice versa.
// Most attributes can be retrieved from both classes with some exceptions:
// The primary e-mail address is only stored on the IMailUser, the contact
// has three named email properties (which are not used so far).
// The birthday is only stored on the contact.

Most of the changes are some refactoring based on the "duality" of address book entry and card. A lot of noise is caused by renaming mMapiData to mDirEntry since it is the entry ID of the directory we're working on. Many changes in nsAbWinHelper.cpp are just to fix the very defective error handling. Without correct error handling and logging, it's impossible to debug.

Comment 42

[IT Support]

Comment on attachment 9196137 [details] [diff] [review]
Part 3: 1682620-fix-add-edit-add-notifications.patch

Review of attachment 9196137 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbWinHelper.cpp
@@ +1008,5 @@
> +               CONTAB_PROVIDER_ID_LENGTH) != 0) {
> +      aFromContact = false;
> +    } else {
> +      contactIdLength = abEntryId->length;
> +      contactId = (LPENTRYID) & (abEntryId->idBytes);

Could use reinterpret_cast here, but there are many C-style casts in the code and this patch (look for (LPSRestriction) and (LPSPropTagArray)) that it doesn't really matter. Happy to change it!

Comment 43

[Ben Campbell]

Comment on attachment 9196137 [details] [diff] [review]
Part 3: 1682620-fix-add-edit-add-notifications.patch

Review of attachment 9196137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +1138,5 @@
> +  if (!aIsAddition) {
> +    NotifyItemModification(aModifiedCard);
> +    if (oldCard) NotifyCardPropertyChanges(oldCard, aModifiedCard);
> +  }
> +

This block doesn't _feel_ quite right to me.
I see that, and my first impression is: "but what if we're adding a new card? Isn't there a notification for that too? Where?"

I know it's correct as is, but I'd still suggest getting rid of the aIsAddition parameter entirely, and move these NotifyItemModification()/NotifyCardPropertyChange() calls back up into nsAbOutlookDirectory::ModifyCard().
if nothing else, it spreads out the code a bit makes ModifyCardInternal() a little simpler :-)

::: mailnews/addrbook/src/nsAbWinHelper.cpp
@@ +1008,5 @@
> +               CONTAB_PROVIDER_ID_LENGTH) != 0) {
> +      aFromContact = false;
> +    } else {
> +      contactIdLength = abEntryId->length;
> +      contactId = (LPENTRYID) & (abEntryId->idBytes);

I think the c-style casts are fine, especially on these LP* types. Don't think we've got any particular policy on them, so whatever you think reads better.

Comment 44

[IT Support]

Thanks for the review. We had the same thought. For new cards, the notification is here:
https://searchfox.org/comm-central/rev/75aaa96983fe0c594aba966ba1b2f172c927d831/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#188

Yes, it would be nice to move the modify notification outside ModifyCardInternal(). But we can't since the NotifyCardPropertyChange() needs the old card which is only available inside ModifyCardInternal(), it's not possible without passing that back which would make it even uglier.

As for the cast, it seems like the existing code for some reason uses reinterpret_cast<LPENTRYID> instead of the C-style cast used for other types. So we might just want to be consistent with that.

Comment 45

[IT Support]

Attachment: Part 3: 1682620-fix-add-edit-add-notifications.patch

Changed the cast. Also changed a malloc() to a moz_xmalloc() with subsequent result checking.

Comment 46

[IT Support]

Maybe not advisable to check in before bug 1686700.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cc34b97f6122
Fix logging conversion specifiers and add PRINT_TO_CONSOLE. r=darktrojan
https://hg.mozilla.org/comm-central/rev/1040a332c95a
Don't base64-encode Outlook AB entry IDs, just print them. r=darktrojan
https://hg.mozilla.org/comm-central/rev/671b2cef3d10
Fix Outlook address book card add/edit and call observer service for add/edit/delete. r=darktrojan,benc