1680923 - Crash in [@ nsXPCWrappedJS::Release] related to Outlook address book search

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[IT Support]

Crash report: https://crash-stats.mozilla.org/report/index/2627809c-00d4-4642-accd-7d7980201205

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(NS_IsMainThread()) (nsXPCWrappedJS::Release called off main thread)

Top 8 frames of crashing thread:

0 xul.dll nsXPCWrappedJS::Release js/xpconnect/src/XPCWrappedJS.cpp:258
1 xul.dll QueryThreadFunc comm/mailnews/addrbook/src/nsAbOutlookDirectory.cpp:804
2 nss3.dll PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:399
3 nss3.dll pr_root nsprpub/pr/src/md/windows/w95thred.c:139
4 ucrtbase.dll thread_start<unsigned int , 1> 
5 kernelbase.dll VirtualQuery 
6 mozglue.dll patched_BaseThreadInitThunk mozglue/dllservices/WindowsDllBlocklist.cpp:588
7 ntdll.dll RtlUserThreadStart 

This crashes in Daily when you add and Outlook address book as per bug 1679525 and then search for an address, either in the Outlook address book or "All Address Books". We noticed while looking into the bug we filed.

100% reproducible and apparently not related to bug 1517464.

Comment 1

[IT Support]

Crash from version 78.5.1: https://crash-stats.mozilla.org/report/index/41667794-7ce6-4b09-9ef3-ebeaa0201205

Comment 2

[IT Support]

Searching Outlook address books hasn't worked in version 60 and 68, and in version 78 it crashes.

Comment 3

[IT Support]

There are two problems here: The crash is caused by the fact that nsIAbCard is now implemented in JS (AddrBookCard.jsm), so the do_QueryElementAt() here
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#929
crashes since it's not run on the main thread. It's run inside nsAbOutlookDirectory::ExecuteQuery() which is run in a special search thread:
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#794
Proxying the QI to the main thread with something like

  for (i = 0; i < nbResults; ++i) {
    NS_DispatchToMainThread(NS_NewRunnableFunction(
      "QueryOnMainThread", [resultsArray, i, &aListener]() {
        nsCOMPtr<nsIAbCard> card;
        card = do_QueryElementAt(resultsArray, i);
        aListener->OnSearchFoundCard(card);
    }));

is not successful since resultsArray is still owned by the search thread. This code snippet may not be optimal, but it compiles ;-)

Once the threading is fixed, the second issue needs to be addressed, that is that the search doesn't work at all. So far we're seeing "Cannot set restriction 80040117" from
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbWinHelper.cpp#790
BTW, 80040117 is MAPI_E_TOO_COMPLEX, so something to do with the restrictions built in BuildRestriction(). If we ignore that error, we actually get all cards returned, which is promising, but then we run into the threading/QI crash.

Ben, we found this snippet authored by you:
https://hg.mozilla.org/comm-central/rev/44e81d98aeb6#l2.82

Maybe you have some insight how to solve the threading/QI issue.

Background: We'd like to evaluate Thunderbird as an Outlook complement, but with this bug here and bug 1679525 it is not really working. We have some experience in Windows and MAPI programming.

Comment 4

[IT Support]

"Crash" means a MOZ_RELEASE_ASSERT(NS_IsMainThread(), ... from XPCWrappedJS.cpp as you can also see in the original report.

Comment 5

[Geoff Lankow (:darktrojan)]

(In reply to IT Support from comment #3)

There are two problems here: The crash is caused by the fact that nsIAbCard is now implemented in JS (AddrBookCard.jsm), so the do_QueryElementAt() here
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#929
crashes since it's not run on the main thread.

All of the cards here should come from this line in OutlookCardForURI, which still produces an instance of nsAbCardProperty.

Comment 6

[Ben Campbell]

OK, analyzing the crash in the opening comment (sorry, a bit longwinded, but helps me work it all out!):

The crash occurs when the query thread is exiting:
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#804
The "I'm on the wrong thread!" assert triggers when the data passed into the thread is deleted. This is a struct of the form:

struct QueryThreadArgs {
  nsAbOutlookDirectory* mThis;
  SRestriction mRestriction;
  nsCOMPtr<nsIAbDirSearchListener> mListener;
  int32_t mResultLimit;
  int32_t mTimeout;
  int32_t mThreadId;
};

There is no destructor defined for QueryThreadArgs, so it's just going to be using the default behaviour: destroying each member in turn.
mThis is a plain pointer, so nothing will happen. Presumably the nsAbOutlookDirectory outlives the query thread (fingers crossed ;- ).
mRestriction (I'm guessing this is a MAPI search filter thing) has already been destroyed, and I'm assuming SRestriction's dtor is OK with that (I'd guess it's a plain struct anyway with default dtor).
mListener is the issue, I think. The listener is passed in to DoQuery(), which passes it on to the QueryThreadArgs, incrementing it's refcount. The thread is kicked off and DoQuery() exits, releasing the listener reference originally passed in. This leaves QueryThreadArgs as the only place holding the listener open, so when the thread finishes, the QueryThreadArgs dtor releases the listener, the listener is deleted on the query thread and... kaboom. Turns out the listener was a javascript object and doesn't like being deleted off the main thread :-)

Comment 7

[IT Support]

Thanks for the comment. What you describe is part of the problem if there are no cards returned since the search is generally broken (2nd part of comment #3). That could be fixed by proxying the release to the main thread. But the issue goes further. We tweaked the process in a way that cards were actually returned, and then this line crashes:
https://searchfox.org/comm-central/rev/ea46a911666aedc9b7832f27bf0363a32a4a1ae2/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#929

The QI involving a JS object with all the related ref-counting can apparently also only be done on the main thread and here it happens on the query thread. So what we wrote in comment #3 is the harder nut to crack here. Any suggestions for that?

Comment 8

[Ben Campbell]

So. Possible solutions:

we need to make sure the listener is released on the main thread. Quick and cheesy method would be to dispatch the cleanup back to the main thread i.e. at the end of the query thread, do something like:

NS_DispatchToMainThread(NS_NewRunnableFunction(
  "FreeQueryThreadArgs", [arguments]() {
    DestroyRestriction(arguments->mRestriction);
    delete arguments;
});

I think this would sort it out. Might also be worth making the QueryThreadArgs::mThis
a nsCOMPtr<nsAbOutlookDirectory>, to lock in the nsAbOutlookDirectory so it doesn't disappear while the thread is running...

Longer term, I think the concern about javascript-implemented nsIAbCard might be a valid one (Geoff? any plans?).
In this case, the MAPI thread should probably avoid using XPCOM objects. Maybe it directly passes MAPI structs for found addresses to the main thread and performs the Card creation there?
Alternatively, is threading even needed? Are MAPI searches likely to be slow? I guess they could well be - you'd be hitting the network, right? Scratch than then :-) Basically, I'd aim to strip down the thread as much as possible. Just MAPI stuff. Anything XPCOM-related gets dispatched to the main thread instead.
If the cheesy fix I mentioned above doesn't help, then I can have a crack at sketching out how to move the XPCOM stuff out of the query thread, if you're happy to test and iterate (I haven't currently got a convenient windows build set up).

(On a side note, we're phasing out use of both nsIMutableArray and nsISimpleEnumerator, in favour of using nsTArray<> (like std::vector), which simplifies code a bit).

Comment 9

[IT Support]

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

All of the cards here should come from this line in OutlookCardForURI, which still produces an instance of nsAbCardProperty.

nsAbCard? That's a JS object now:
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/modules/AddrBookCard.jsm#18
Are we misunderstanding this?

Comment 10

[IT Support]

Longer term, I think the concern about javascript-implemented nsIAbCard might be a valid one.

It was C++ before, see: https://hg.mozilla.org/comm-central/rev/b0bac431a4e7e4adc6ae732285b3d892c74bdc9b. So JS seems to be the future direction.

Comment 11

[IT Support]

Attachment: 1680923-remove-threading.patch

This removes threading in nsAbOutlookDirectory::DoQuery(). Some remarks:
As per comment #3, we don't think that proxying to the main thread in ExecuteQuery() when run on the query thread will help. The query thread still owns the results in resultsArray which was allocated on the query thread and hence cannot be QI'ed to an object on the main thread. That is why the code shown in comment #3 fails:

  for (i = 0; i < nbResults; ++i) {
    NS_DispatchToMainThread(NS_NewRunnableFunction(
      "QueryOnMainThread", [resultsArray, i, &aListener]() {
        nsCOMPtr<nsIAbCard> card;
        card = do_QueryElementAt(resultsArray, i);
        aListener->OnSearchFoundCard(card);
    }));

We get:
Hit MOZ_CRASH(nsAbCardProperty not thread-safe) at c:/mozilla-source/comm-central/xpcom/base/nsISupportsImpl.cpp:41
from nsAutoOwningThread::AssertCurrentThreadOwnsMe()
Our interpretation is what we wrote above: resultsArray is owned by the query thread and can't be QI'ed to an object on the main thread. The cards in the results array are created here in GetChildCards() on the query thread
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#961
and appended to the results array. Even if you change that from a mutable array to nsTArray<RefPtr<nsIAbCard>>, the issue of the non-thread-safeness of the object remains. Please let us know if you don't agree so we can pursue a different approach.

With the patch, there is no more crash when no results are found, and when ignoring that there is something wrong with the restrictions, see next patch, results are returned correctly. We'll address the restriction problem separately.
In our experience this code only accesses a local Outlook address book, so performance should not be an issue here. Certainly a small delay is preferable to a crash. We don't believe that querying a subset of cards is any slower than displaying all cards in the address book which is what this code here does:
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/content/abView.js#20
Threading is a little questionable here anyway, since results are not added to the panel via aListener->OnSearchFoundCard() during the search operation, but instead in a block once all results are found. So the user needs to wait anyway.
Furthermore Geoff is planning an overhaul of this area, see bug 1680952 comment #7, so at that time the JS framework he's planning to implement might use a JS worker thread.

Comment 12

[IT Support]

Attachment: ignore-restriction-issue.patch

This patch glosses over the problem with the restrictions so that all cards are returned. This will need to be replaced by a proper fix.

Comment 13

[IT Support]

The cards in the results array are created here in GetChildCards() on the query thread
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#961

More precisely:
https://searchfox.org/comm-central/rev/1c7609a0edd9752c0ae7f5000e9ffe6a53168824/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#969
https://searchfox.org/comm-central/rev/a18b84093ce824cd90f76054da6885378bc390c8/mailnews/addrbook/src/nsAbOutlookDirectory.cpp#1374
Created on the query thread and not thread-safe.

Comment 14

[IT Support]

Additional info: Based on the error Hit MOZ_CRASH(nsAbCardProperty not thread-safe) we changed nsAbCardProperty to be derived from NS_DECL_THREADSAFE_ISUPPORTS in nsAbCardProperty.h which shifts the error to Hit MOZ_CRASH(nsVariant not thread-safe) using the code snippet from comment #11. So right now, removing the threading appears to be the best approach.

Comment 15

[IT Support]

We filed bug 1681824 for the broken search since it doesn't appear to be a simple issue which could be fixed at the same time as the crash.

Comment 16

[Ben Campbell]

Comment on attachment 9192366 [details] [diff] [review]
1680923-remove-threading.patch

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

Thanks, that patch looks good to me!
Geoff: I'm fine with this patch from a C++ point of view, but just running it by you too as I'm not too familiar with the address book. No hidden issues in moving this on to the main thread?

Comment 17

[IT Support]

Attachment: 1680923-remove-threading.patch

We looked at this again and noticed that DoQuery() should return a context ID according to the IDL. That got over-zealously removed in the original patch, so we restored it here. We assume that the r+ covers this since we merely restored a few lines. (Too bad you can't compare patches any more.)

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9192936 [details] [diff] [review]
1680923-remove-threading.patch

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

I'm happy with this as long as it works, which I assume it does.

Comment 19

[IT Support]

Working, sure, see attachment 9192893 [details].

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/617e9e0bb281
Remove threading in nsAbOutlookDirectory::DoQuery() to avoid XPConnect issues. r=benc

Comment 21

[IT Support]

Would be good to get this into Thunderbird 78.x as swiftly as bug 1680952. Can't get worse than crashing ;-)

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9192936 [details] [diff] [review]
1680923-remove-threading.patch

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

Crash fix.

Comment 23

[IT Support]

Surely it's OK to fix the crash, but the search still doesn't work without bug 1681824, and auto-complete is useful, too, bug 1679525.

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 9192936 [details] [diff] [review]
1680923-remove-threading.patch

[Triage Comment]
Approved for esr78

Comment 25

[Rob Lemley [:rjl]]

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/ea02185c6882