1680952 - Empty connected Outlook address book causes problems

Status: RESOLVED FIXED

(Thunderbird :: 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:

As per bug 1679525, connect an empty Outlook address book to Thunderbird.

Actual results:

The address book window becomes dysfunctional, with many errors in the console:

Uncaught TypeError: can't access property "children", this._rowMap[aIndex] is undefined isContainer chrome://messenger/content/jsTreeView.js:90
Uncaught TypeError: can't access property "getProperties", this._rowMap[aRow] is undefined getRowProperties chrome://messenger/content/jsTreeView.js:67
Uncaught TypeError: can't access property "children", this._rowMap[aIndex] is undefined isContainer chrome://messenger/content/jsTreeView.js:90

Expected results:

Thunderbird should have handled an empty Outlook address book properly. Everything is fine if the address book contains at least one entry. Due to unfortunate Outlook configuration, it can happen that Outlook has multiple address books, one of them empty which triggers this bug.

Comment 1

[IT Support]

Attachment: 1680952-childcards.patch

This seems to be the cause of the issue:
JavaScript error: chrome://messenger/content/addressbook/abView.js, line 20: : Component returned failure code: 0xaaaaaaaa [nsIAbDirectory.childCards]

The patch fixes the immediate issue, however, we don't know how well empty address books care handled in general.

Comment 2

[IT Support]

Attachment: 1680952-childcards-alternative.patch

Alternative approach. Maybe better than the first attempt.

Comment 3

[IT Support]

Attachment: 1680952-init-rv.patch

Looking at this one more time, all it was was an uninitialised variable which led to the 0xAAAAAAAA return code.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9191577 [details] [diff] [review]
1680952-init-rv.patch

Got there in the end! Thanks for finding and fixing this.

As you can probably tell, this code doesn't receive a lot of love. We don't even acknowledge it exists by including the preferences by default. I hope to completely overhaul it and the OS X provider some time in the new year.

Comment 5

[IT Support]

Thanks for the review and sorry about the unsuitable prior versions. However, the approach taken in attachment 9191512 [details] [diff] [review] may still be worth considering additionally since failure of dir.childCards for whatever reason makes the entire address book tree unusable.

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

As you can probably tell, this code doesn't receive a lot of love. We don't even acknowledge it exists by including the preferences by default.

Indeed, however, we came across bug 83100[1] from 2001 with plans to expose the option in the UI.

I hope to completely overhaul it and the OS X provider some time in the new year.

So that means we shouldn't continue to look into bug 1680923 and bug 1679525? The crash described in the former was caused by moving the nsIAbCard implementation to JS. That Outlook address book search hasn't worked in a long time is another issue which may also cause the latter bug. Hard to tell since we didn't go back further than version 60, so we didn't find a working version. BTW, sorry about all the comments in bug 1679525, we just came across three issues at once and it took some time to analyse.

While we're talking about an overhaul, we noticed that the Mac AB now provides the OS-supplied UID:
https://hg.mozilla.org/comm-central/rev/d3ce9854522e#l2.13
which is nice for linking with external systems. Are there plans to do the same for the Outlook address book?
PidTagAddressBookObjectGuid (https://docs.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-oxoabk/3bdafbc9-4f16-4430-bdd9-eac321c87647). Right now, Thunderbird sets a property with the URI
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#1378
but that doesn't appear to be useful for linking with external systems. We haven't looked into where how that URI is constructed but it doesn't appear to reflect any property in the Outlook address book.

Will there be a spec to comment on for the overhaul? Any ETA? Maybe ready for the next major version?

[1] mentioned here: http://kb.mozillazine.org/Using_Outlook_and_OE_contacts_with_Thunderbird_or_Mozilla_Mail, at the very bottom.

Comment 6

[IT Support]

And another thing: CardForEmailAddress() is implemented on Mac
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOSXDirectory.mm#735
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOSXDirectory.h#77
overriding the default of "not implemented"
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbDirProperty.cpp#327
which takes effect for the Outlook address book. Any plans to implement it there? That would require some investigation into the underlying MAPI.

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to IT Support from comment #5)

Indeed, however, we came across bug 83100[1] from 2001 with plans to expose the option in the UI.

Well yes, there is a bug, but it was filed 19 years ago by people who have long since left the project. That was probably about the last time did any serious work on this provider. Not that we don't want it fixed up and enabled but … priorities.

So that means we shouldn't continue to look into bug 1680923 and bug 1679525?

That depends. I haven't had a chance to look close enough to understand what's going on there but, if nothing else, fixes can be uplifted to Thunderbird 78 so you and other users can benefit from them without waiting until the next release.

What I have in mind for the future is for the C++ parts that interact with system libraries to remain, and have JS collect data from them and it into the appropriate structures. Something like the CardForEmailAddress implementation you mentioned, has no reason to be in C++.

While we're talking about an overhaul, we noticed that the Mac AB now provides the OS-supplied UID:
https://hg.mozilla.org/comm-central/rev/d3ce9854522e#l2.13
which is nice for linking with external systems. Are there plans to do the same for the Outlook address book?
PidTagAddressBookObjectGuid (https://docs.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-oxoabk/3bdafbc9-4f16-4430-bdd9-eac321c87647). Right now, Thunderbird sets a property with the URI
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#1378
but that doesn't appear to be useful for linking with external systems. We haven't looked into where how that URI is constructed but it doesn't appear to reflect any property in the Outlook address book.

No plans, but no objection to it happening either. That change was made by a volunteer to fix a specific problem he was having.

Will there be a spec to comment on for the overhaul? Any ETA? Maybe ready for the next major version?

At the moment I'm trying to work out what is needed. It's likely to be a fairly thin but generic wrapper around the system libraries, with functions that do things like list all directories or return all of the properties for a given contact. If existing code is suitable and saves me having to figure things out, I'll probably adapt it instead of starting from scratch. At the moment ETA is the next version.

That all said, thank you for putting some effort in to the Outlook provider. It sorely needs some attention that we've been unable to give it. As you can probably tell when it still has problems like returning uninitialised values. :-)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/58bf53f51a4c
Initialise rv in nsAbOutlookDirectory::GetChildCards() to make 'no cards' work. r=darktrojan

Comment 9

[IT Support]

Thank you for your detailed reply. As we said before, bug 1680923 is caused an XPConnect issue, details in bug 1680923 comment #3. Sadly our knowledge in this area is limited, but we found someone who contributed a vaguely related change in LDAP, so hopefully this person or someone else in your team can give some direction. We're happy to look into the Windows/MAPI side of things, but how all this C++/JS interface hangs together, especially with respect to threading (search happens on a special search thread), is sadly not part of our expertise. Maybe you or someone else can give a few hints to start us off.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9191577 [details] [diff] [review]
1680952-init-rv.patch

[Approval Request Comment]
This might as well go straight to ESR. It's only just landed but it's zero risk, this code isn't just pref'ed off, the prefs don't exist. For the very tiny fraction of users using the Outlook provider, this will fix a problem should they ever have no Outlook contacts.

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9191577 [details] [diff] [review]
1680952-init-rv.patch

[Triage Comment]
Approved for esr78

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/64abce3e8616

Comment 13

[IT Support]

Right now, Thunderbird sets a property with the URI
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#1378
but that doesn't appear to be useful for linking with external systems. We haven't looked into where how that URI is constructed but it doesn't appear to reflect any property in the Outlook address book.

For the record: The statement above is incorrect: That URI is basically a base64 encoding of the Outlook address book EntryId, so it can be used for linking with external systems:
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#965
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/src/nsAbWinHelper.cpp#174