Crash in nsAbCardProperty::SetUID
Categories
(MailNews Core :: Address Book, defect)
Tracking
(thunderbird_esr6065+ 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)
847 bytes,
patch
|
darktrojan
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
darktrojan
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
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
Updated•5 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
signature is still Mac-only. Almost 100% startup.
OWL add-on installed 100% in a sampling of >10 crashes
Comment 3•5 years ago
|
||
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.
Comment 4•5 years ago
|
||
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
Comment 5•5 years ago
|
||
(I contacted Christine by email.)
Geoff, this might be related to the macOS address book.
Assignee | ||
Comment 6•5 years ago
|
||
Does Owl try to read the UID of the macOS address book or its contents?
Comment 7•5 years ago
|
||
I don't think so.
Assignee | ||
Comment 8•5 years ago
|
||
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…
Comment 9•5 years ago
|
||
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.)
Updated•5 years ago
|
Comment 10•5 years ago
|
||
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 11•5 years ago
|
||
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.
Comment 12•5 years ago
|
||
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.
Comment 13•5 years ago
|
||
Sorry, typo, I meant: we do nothing specific that triggers this crash
Comment 14•5 years ago
|
||
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 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 17•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fec810d47748
Fix crash in nsAbCardProperty::SetUID(). r=darktrojan
Updated•5 years ago
|
Comment 18•5 years ago
|
||
@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 19•5 years ago
|
||
Comment on attachment 9041324 [details] [diff] [review] 1523904-setuid-crash-1.diff I think this still makes sense, no?
Comment 20•5 years ago
|
||
We've released Owl version 0.5.3 that has a temporary fix to work around the crash.
Comment 22•5 years ago
|
||
FYI, this affected 26 different users. Luckily not that many, but we made their Thunderbird crash badly: 286 crashes total.
Assignee | ||
Comment 23•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 24•5 years ago
|
||
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
Comment 25•5 years ago
|
||
TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/9378f2d586b0
https://hg.mozilla.org/releases/comm-beta/rev/7759b93fc8dd
Comment 26•5 years ago
|
||
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.
Comment 27•5 years ago
|
||
TB 60.5.1:
https://hg.mozilla.org/releases/comm-esr60/rev/bff49a3085230a4161ceb820c26288adb403562d
https://hg.mozilla.org/releases/comm-esr60/rev/cacb98220bdbec6799f18d828fb0a0c14bad935b
Description
•