Closed Bug 1523904 Opened 5 years ago Closed 5 years ago

Crash in nsAbCardProperty::SetUID

Categories

(MailNews Core :: Address Book, defect)

Unspecified
macOS
defect
Not set
critical

Tracking

(thunderbird_esr6065+ fixed, thunderbird66 fixed, thunderbird67 fixed)

RESOLVED FIXED
Thunderbird 67.0
Tracking Status
thunderbird_esr60 65+ fixed
thunderbird66 --- fixed
thunderbird67 --- fixed

People

(Reporter: wsmwk, Assigned: darktrojan)

References

Details

(Keywords: crash, regression, Whiteboard: [regression:tb60.5.0])

Crash Data

Attachments

(2 files, 1 obsolete file)

We have several of these already for 60.5.0, on Mac. Although some also exist for 60.4.0 bp-05a10f4f-389e-4679-af5d-879b20190127 - seems mostly on Windows.

bp-4c24b94d-ce48-46af-8437-f6fc60190130 60.5.0 Mac

Top 10 frames of crashing thread:

0 XUL nsAbCardProperty::SetUID comm/mailnews/addrbook/src/nsAbCardProperty.cpp:389
1 XUL nsAbCardProperty::GetUID comm/mailnews/addrbook/src/nsAbCardProperty.cpp:367
2 XUL NS_InvokeByIndex
3 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1801
4 XUL XPC_WN_GetterSetter js/xpconnect/src/xpcprivate.h:1563
5 XUL js::InternalCallOrConstruct js/src/vm/JSContext-inl.h:260
6 XUL js::CallGetter js/src/vm/Interpreter.cpp:484
7 XUL js::NativeGetProperty js/src/vm/NativeObject.cpp:2028
8 XUL js::GetProperty js/src/vm/NativeObject.h:1589
9 XUL Interpret js/src/vm/Interpreter.cpp:203

Flags: needinfo?(geoff)

signature is still Mac-only. Almost 100% startup.

OWL add-on installed 100% in a sampling of >10 crashes

Flags: needinfo?(ben.bucksch)
Keywords: regression
OS: Unspecified → macOS
Whiteboard: [regression:tb60.5.0]

I know that Owl triggers this, but given that Owl is 100% JS, it cannot possibly be the underlying cause of the crash. The crash is in AB UID handling, i.e. bug 1482040.

Geoff, could you maybe take a look at this?

We could look at it as well, but I we have another high priority bug to look at right now.

Assignee: nobody → geoff
Flags: needinfo?(ben.bucksch)

Hi so i removed the addon and it seems to be working fine. The reason i added it in the first place was when i added my 2nd email account (which is an Office365 account), the system provided a link to OWL to configure my email account to use it. This time, i entered all the details manually and its working fine.

Thanks
Christine

(I contacted Christine by email.)

Geoff, this might be related to the macOS address book.

Does Owl try to read the UID of the macOS address book or its contents?

Flags: needinfo?(geoff)

I don't think so.

Attached patch 1523904-setuid-crash-1.diff (obsolete) — Splinter Review

We probably shouldn't attempt to write to a read-only database, so this returns NS_ERROR_FAILURE in that case. I think this is the right thing to do, but if anyone has any better ideas…

Attachment #9041324 - Flags: review?(mkmelin+mozilla)

Is that the line that's crashing? If this fixes the crash, then it sounds like a sane fix to me.

Long-term, you would want to store the UIDs in a third storage, to keep a unified API.

Or does the macOS maybe already have UUIDs for each entry? Can GetUID just return those existing IDs? That would be another fix. (The caller here in this crash called the getter. I'd still keep your patch here, in case somebody calls the setter.)

Attachment #9041324 - Flags: review+
Comment on attachment 9041324 [details] [diff] [review]
1523904-setuid-crash-1.diff

Unfortunately this is a null pointer crash - `abManager->GetDirectoryFromId` is returning `null`. lldb wouldn't tell me what `directoryId` was though.

FYI the access to the `UID` appears to be caused by an `onItemAdded` notification generated by the OSX address book that's being handled by `ext-addressBook.js`.
Comment on attachment 9041324 [details] [diff] [review]
1523904-setuid-crash-1.diff

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

LGTM, I suppose you could use NS_ERROR_FILE_READ_ONLY which is perhaps the closest to an explanation you'd get.
Attachment #9041324 - Flags: review?(mkmelin+mozilla) → review+

We looked at it from the Owl perspective, and from our side, we do nothing with the OS X address book, and we do specific that triggers this crash. Every single addon that uses the new address book API would have triggered the crash. Owl just happens to be the first user of the API and therefore be blamed on the crash stats.

Sorry, typo, I meant: we do nothing specific that triggers this crash

Neil meant to say in comment 10 that the patch here won't actually fix the crash. Neil should attach a wallpaper fix for the crash soon. The fix adds just a null check to avoid the crash, but there's another underlying bug why this is null that should be addressed. This should be looked at as well.

The patch above logically makes sense to me, and fixes yet another aspect, but won't fix the actual crash.

Comment on attachment 9041866 [details] [diff] [review]
Wallpaper over null pointer crash

I haven't had a chance to look into why GetDirectoryFromId would return null, but this'll do the job for now.
Attachment #9041866 - Flags: review?(geoff)
Attachment #9041866 - Flags: review+
Attachment #9041866 - Flags: approval-comm-esr60?
Attachment #9041866 - Flags: approval-comm-beta?
Attachment #9041324 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #9041866 - Flags: approval-comm-esr60?
Attachment #9041866 - Flags: approval-comm-esr60+
Attachment #9041866 - Flags: approval-comm-beta?
Attachment #9041866 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fec810d47748
Fix crash in nsAbCardProperty::SetUID(). r=darktrojan

Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0

@Geoff: Your original patch still made sense to me. Want to land this as well?

Also, could you please file a bug and investigate the underlying why why GetDirectoryFromId returns null?

Comment on attachment 9041324 [details] [diff] [review]
1523904-setuid-crash-1.diff

I think this still makes sense, no?
Attachment #9041324 - Attachment is obsolete: false

We've released Owl version 0.5.3 that has a temporary fix to work around the crash.

So are we landing the original patch?

Flags: needinfo?(geoff)

FYI, this affected 26 different users. Luckily not that many, but we made their Thunderbird crash badly: 286 crashes total.

I guess we do land this. My concern was (apart from it not applying on top of the other patch) that it just shifts the failure to whatever calls GetUID (and therefore SetUID), but it looks like those callers are all in javascript, or places where the directory is writable.

Attachment #9041324 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9042353 - Flags: review+
Attachment #9042353 - Flags: approval-comm-esr60?
Attachment #9042353 - Flags: approval-comm-beta?
Attachment #9042353 - Flags: approval-comm-esr60?
Attachment #9042353 - Flags: approval-comm-esr60+
Attachment #9042353 - Flags: approval-comm-beta?
Attachment #9042353 - Flags: approval-comm-beta+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f6c2b6ce1fd4
Return NS_ERROR_FAILURE when setting UID on a read-only address book. r=mkmelin,BenB

Keywords: checkin-needed

FYI, I just found that there were 9 crashes not related to Owl. Most of those have GeneralSync installed https://generalsync.com/en/ .

Since Feb 7 when we released Owl 0.5.3 with the workaround, there was only one further crash with Owl.
https://crash-stats.mozilla.com/report/index/33c0522c-0bf7-4f8e-9a41-7b16e0190207#tab-extensions
but was right after the release and might be an one-off with an odd circumstance, e.g. the addon updated, but Thunderbird not yet restarted.

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

Attachment

General

Created:
Updated:
Size: