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

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

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.

Comment 1

[Stephen A Pohl [:spohl] (OOO until 1/26)]

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

Comment 2

[Stephen A Pohl [:spohl] (OOO until 1/26)]

Attachment: Patch

Comment 3

[Markus Stange [:mstange]]

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?

Comment 4

[Stephen A Pohl [:spohl] (OOO until 1/26)]

(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.

Comment 5

[Stephen A Pohl [:spohl] (OOO until 1/26)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ecff8f25c842188469ec713f3942f0abea6d6c
Bug 1472629: Handle native exceptions when reading accessibility attribute values to avoid crashing. r=mstange

Comment 6

[Markus Stange [:mstange]]

Oh, I see, that makes sense.

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a2ecff8f25c8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta. Should we backport?

Comment 9

[Stephen A Pohl [:spohl] (OOO until 1/26)]

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

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/22bf7b7633ba

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I don't think we need to worry about this for ESR60. Feel free to nominate if you feel strongly otherwise.