Closed Bug 1472629 Opened Last year Closed Last year

Crash in CrashReporter::TerminateHandler | libc++abi.dylib@0x260a0

Categories

(Core :: Widget: Cocoa, defect, P1, critical)

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: gsvelto, Assigned: spohl)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-e38e89fd-a601-4513-8375-2b5e80180701.
=============================================================

Top 10 frames of crashing thread:

0 XUL CrashReporter::TerminateHandler toolkit/crashreporter/nsExceptionHandler.cpp:1435
1 libc++abi.dylib libc++abi.dylib@0x260a0 
2 libc++abi.dylib libc++abi.dylib@0x25b2f 
3 libobjc.A.dylib libobjc.A.dylib@0xe897 
4 CoreFoundation CoreFoundation@0x163bd8 
5 AppKit AppKit@0xb1391 
6 XUL -[BaseWindow accessibilityAttributeValue:] widget/cocoa/nsCocoaWindow.mm:3457
7 AppKit AppKit@0xb1296 
8 AppKit AppKit@0xb0677 
9 AppKit AppKit@0xb000d 

=============================================================

This is caused by an uncaught exception.
A few observations:

1. accessibilityAttributeValue is deprecated[1].
2. The crash is a null dereference.
3. Could attribute be null, and would a simple null check here[2] avoid the crash?

[1] https://developer.apple.com/documentation/objectivec/nsobject/1532465-accessibilityattributevalue?language=objc
[2] https://hg.mozilla.org/releases/mozilla-beta/annotate/3178ed26d318f5b65b8e15a2392794bd70c13296/widget/cocoa/nsCocoaWindow.mm#l3457
Attached patch PatchSplinter Review
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8993412 - Flags: review?(mstange)
Priority: -- → P1
Comment on attachment 8993412 [details] [diff] [review]
Patch

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

Are we crashing because the caller of this method doesn't correctly handle exceptions?
Attachment #8993412 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #3)
> Comment on attachment 8993412 [details] [diff] [review]
> Patch
> 
> Review of attachment 8993412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we crashing because the caller of this method doesn't correctly handle
> exceptions?

I believe this to be the case due to the given crash reason of "Unhandled exception" in the crash reports. If this crash signature doesn't disappear, we may have to revisit.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ecff8f25c842188469ec713f3942f0abea6d6c
Bug 1472629: Handle native exceptions when reading accessibility attribute values to avoid crashing. r=mstange
Oh, I see, that makes sense.
https://hg.mozilla.org/mozilla-central/rev/a2ecff8f25c8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
This grafts cleanly to Beta. Should we backport?
Flags: needinfo?(spohl.mozilla.bugs)
Comment on attachment 8993412 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: Possibility of crashes when using accessibility tools.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: There don't appear to be any crash reports since this has landed.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This patch adds native exception handling code that is used throughout our code base for code that calls out to native APIs. No functional changes are included in this patch.
[String changes made/needed]: none
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8993412 - Flags: approval-mozilla-beta?
Comment on attachment 8993412 [details] [diff] [review]
Patch

Even though I wasn't able to see any crashes with this signature in the last week, the fix seems low risk and ok to uplift as we are still mid-beta, Beta62+
Attachment #8993412 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I don't think we need to worry about this for ESR60. Feel free to nominate if you feel strongly otherwise.
You need to log in before you can comment on or make changes to this bug.