1471504 - Crash in [@ CrashReporter::TerminateHandler | std::__terminate][@ mozAccessible parent]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

This bug was filed from the Socorro interface and is
report bp-acfd9ef3-fc93-4f3d-9565-86e740180626.
=============================================================

Top 10 frames of crashing thread:

0 XUL CrashReporter::TerminateHandler toolkit/crashreporter/nsExceptionHandler.cpp:1435
1 libc++abi.dylib std::__terminate 
2 libc++abi.dylib __cxa_throw 
3 libobjc.A.dylib objc_exception_throw 
4 CoreFoundation +[NSException raise:format:arguments:] 
5 Foundation -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] 
6 XUL -[mozAccessible parent] accessible/mac/mozAccessible.mm:595
7 XUL -[mozAccessible accessibilityAttributeValue:] accessible/mac/mozAccessible.mm:286
8 AppKit NSAccessibilityGetObjectForAttributeUsingLegacyAPI 
9 AppKit ___NSAccessibilityEntryPointValueForAttribute_block_invoke.804 

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

Comment 1

[[:philipp]]

this bucket of crashes here makes up the bulk of the remaining [@ CrashReporter::TerminateHandler | std::__terminate ] signature on 62.0b6 after bug 1467582 got fixed - overall that's accounting for 20% of mac browser crashes in beta 6.

Comment 2

[[:philipp]]

it seems to be mostly ru/uk builds that are affected by this type of crash.
one comment says it's repeatedly happening after hitting the ESC key to leave from fullscreen mode while playing a video.

Comment 3

[[:philipp]]

this is a socorro query covering just the particular a11y issue:
https://crash-stats.mozilla.com/signature/?product=Firefox&proto_signature=~mozAccessible%20parent&signature=CrashReporter%3A%3ATerminateHandler%20%7C%20std%3A%3A__terminate&date=%3E%3D2018-03-01#reports

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, that shows about 100 crashes in the last week, on beta 62. 
I think jwatt is just back from PTO so he will likely investigate soon.

Comment 5

[Jonathan Watt [:jwatt]]

By way of background, the patch for bug 1270217 was created because Emilio's patch for bug 1465585 to switch us from using mozilla::Move to std::move broke local builds (not CI builds) on Mac since the headers for 10.7 don't have std::move. I wrote the patch to update us to 10.9, but that was just because I was one of the first people to bump into the local bustage rather than anything else.

I don't actually have much experience of using/debugging macOS platform libs so I'll probably be quite slow to debug this. I'll can certainly work on it, but maybe dmajor would be willing to take a look since I'm sure he'd manage to figure things out a lot faster.

Comment 6

[Jonathan Watt [:jwatt]]

Oops, it was spohl that fixed bug 1467582, not dmajor. I also didn't mean to remove the needinfo for myself.

Comment 7

[Marcia Knous [:marcia]]

Adding another crash signature I spotted which has mozAccessible parent in the stack, as well as CrashReporter::TerminateHandler().

Comment 8

[Stephen A Pohl [:spohl]]

This is an intentional crash in our accessibility code, seemingly due to the inability of finding a parent when we expect one:

https://hg.mozilla.org/releases/mozilla-beta/annotate/1716b76aa7b8df189e74bf87394d3a11e0c5ac12/accessible/mac/mozAccessible.mm#l595

Joanmarie, I see that you have been active in this file recently. Do you have any thoughts here?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

NI Jamie for awareness. This is currently tracking 62, but might be fixed? (Not many recent crashes?)

Comment 10

[James Teh [:Jamie]]

(In reply to Stephen A Pohl [:spohl] from comment #8)
> This is an intentional crash in our accessibility code, seemingly due to the
> inability of finding a parent when we expect one:

True enough, but why would bug 1270217 cause this to manifest? Also (I'm not a Mac developer), is NSAssert1 really intended to run on release builds? Was this assertion perhaps previously captured by our native exception handling? And if so, would that be also fixed by bug 1290972? or are you suggesting that the correlation with bug 1270217 is bogus?

Unfortunately, we don't have anyone on the a11y team who really knows anything about Mac native code. However, I did a bit of poking around. It seems that ProxyAccessible::OuterDocOfRemoteBrowser (as called at mozAccessible.mm line 588) can return null; see bug 1171728 comment 6. If that happens, we'd certainly hit that assertion. (It *shouldn't* happen, but we've seen it in the wild, as above.)

So, the fix here (for now) is to convert that NSAssert1 into some kind of assert that only fires on debug builds (we still want to be able to debug this if a developer sees it) and return null in that case:

  SomeKindOfDebugOnlyNSAssert1(nativeParent, @"!!! we can't find a parent for %@", self);
  if (!nativeParent) {
    return nil;
  }

  return GetObjectOrRepresentedView(nativeParent);

Comment 11

[James Teh [:Jamie]]

Might also be good to add a debug only assert for outerDoc earlier.

Comment 12

[Stephen A Pohl [:spohl]]

Bug 1290972 landed, which appears to fix this.

Comment 13

[Stephen A Pohl [:spohl]]

Bug 1290972 has been uplifted to 62.