Closed Bug 1676503 Opened 11 months ago Closed 11 months ago

Crash in [@ nsAbQueryLDAPMessageListener::OnLDAPError]


(Thunderbird :: General, defect)



(thunderbird_esr78+ fixed, thunderbird83 wontfix)

84 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird83 --- wontfix


(Reporter: wsmwk, Assigned: benc)




(Keywords: crash, regression)

Crash Data


(1 file)

This crash exists on 83 beta, despite the recent fixing of Bug 1673205 in 83.0b2.

Crash report:


Top 10 frames of crashing thread:

0 XUL nsAbQueryLDAPMessageListener::OnLDAPError comm/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp:104
1 XUL mozilla::detail::RunnableFunction<nsLDAPConnection::InvokeErrorCallback xpcom/threads/nsThreadUtils.h:577
2 XUL mozilla::RunnableTask::Run xpcom/threads/TaskController.cpp:245
3 XUL mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:515
4 XUL mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:374
5 XUL mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal xpcom/threads/nsThreadUtils.h:577
6 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1197
7 CoreFoundation -[__NSArrayM removeObjectsInRange:] 
8 XUL NS_ProcessPendingEvents xpcom/threads/nsThreadUtils.cpp:461
9 XUL nsBaseAppShell::NativeEventCallback widget/nsBaseAppShell.cpp:87
Flags: needinfo?(benc)

Hmm. I'm a bit baffled by this one...
Happens on Linux, Windows and OSX in a pretty consistent-looking form.
I'd say it came in with Bug 1659947 (which is where I added the OnLDAPError method to the listener).

101 NS_IMETHODIMP nsAbQueryLDAPMessageListener::OnLDAPError(nsresult status,
102                                                         nsISupports* secInfo) {
103   if (mResultListener) {
104     mResultListener->OnQueryResult(
105         nsIAbDirectoryQueryResultListener::queryResultError,
106         nsILDAPErrors::OTHER);
107   }
108   return NS_OK;
109 }

It's crashing on line 104, trying to read location 0x0, but mResultListener is checked in line 103.
The only thing I can think of is that mResultListener (or even the "this" ptr) is being cleared on another thread between lines 103 and 104...
We're already on the main thread at this point, and I thought all the core directory query stuff was single-threaded?
And in any case, there doesn't seem to be any code anywhere that clears mResultListener (it's set in the ctor and lasts for the whole lifespan of its owner).

The calling code is here (running on the socket transport thread):

      "InvokeErrorCallback", [listener, status, tsi, location]() {
        listener->OnLDAPError(status, tsi, location);

All the variables are copied into the closure so there shouldn't be any scoping issues there. listener and tsi are nsCOMPtr<>, so the closure will be getting a reference to hold them in scope... listener provides the "this" pointer in OnLDAPError(), so the nsCOMPtr<> reference should hold it in existance just fine.

Without a way to replicate this, I'm kind of out of ideas...

Flags: needinfo?(benc)

I doubt this will fix it, but my understanding of C++ closures could be all wrong, so who knows :-)
In my understanding this should be exactly the same, but the patch version is probably a slightly nicer syntax anyway...

Attachment #9187044 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9187044 [details] [diff] [review]

Review of attachment 9187044 [details] [diff] [review]:

Worth a try. r=mkmelin
Attachment #9187044 - Flags: review?(mkmelin+mozilla) → review+
Assignee: nobody → benc
OS: macOS → All
Target Milestone: --- → 84 Branch

Pushed by
Trivial simplification to nsLDAPConnection::InvokeErrorCallback() implementation. r=mkmelin

Closed: 11 months ago
Resolution: --- → FIXED

I feel RESOLVED/FIXED might be a little optimistic here, but hey, we live in hope ;-)

So far NO crashes since 2020110820332, for example bp-b17fe55f-47db-4f8a-b70c-8ec7f0201117
But I think we need to see what happens on beta this week end next.

I'm half hoping that this crash does show up again.
If it doesn't, it's off to dull-C++-in-depth-pedantic-detail-land for me!

Comment on attachment 9187044 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): bug 1659947 which went to 78.5.1
Crash fix.

Attachment #9187044 - Flags: approval-comm-esr78?

Comment on attachment 9187044 [details] [diff] [review]

[Triage Comment]
Approved for esr78

Attachment #9187044 - Flags: approval-comm-esr78? → approval-comm-esr78+

Sighs, and goes to read up on the intricacies of fancy new C++ features. Again.

Not possible to verify fixed on 78.6.0 - no crashes ever seen for this signature on release channel.
But clearly gone in beta.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.