1375130 - Crash in mozilla::a11y::LazyInstantiator::GatherTelemetry

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-0980383b-8b19-4183-9a04-d37420170621.
=============================================================

There are 12 crashes from the same installation in nightly 56 with buildid 20170621030208. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1323069.

[1] https://hg.mozilla.org/mozilla-central/rev?node=94ba0b72c28ac6fbb6a680c52ab073f10503eb16

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add missing return value check

Oops.

I'm not sure why we're failing in there (could be due to OS restrictions), but regardless we shouldn't crash.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 8880080 [details] [diff] [review]
Add missing return value check

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

r+ if we can still count failed a11y inits with failed queries (see below).

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +239,5 @@
>    nsCOMPtr<nsIFile> clientExe;
> +  if (!GetClientExecutableName(aClientTid, getter_AddRefs(clientExe))) {
> +    // We cannot retrieve the client executable name!
> +    // Just return true as a failsafe.
> +    return true;

Your hunch is QueryFullProcessImageName is failing?

Will we have a clear picture of what the proportion of failed queries are? If not, is it worth recording a fallback reserved name in telemetry (eg. "N/A").

But, I assume we can use our current a11y init numbers as the denominator:
((number of successful exe probes)/(number of a11y instantiations))

Right?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Comment on attachment 8880080 [details] [diff] [review]
> Add missing return value check
> 
> Review of attachment 8880080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if we can still count failed a11y inits with failed queries (see below).
> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +239,5 @@
> >    nsCOMPtr<nsIFile> clientExe;
> > +  if (!GetClientExecutableName(aClientTid, getter_AddRefs(clientExe))) {
> > +    // We cannot retrieve the client executable name!
> > +    // Just return true as a failsafe.
> > +    return true;
> 
> Your hunch is QueryFullProcessImageName is failing?
> 

Yeah, we've got clientExe is null because of some failure. It might not necessarily be QueryFullProcessImageName, it might be OpenThread or OpenProcess failing.

> Will we have a clear picture of what the proportion of failed queries are?
> If not, is it worth recording a fallback reserved name in telemetry (eg.
> "N/A").
> 

You know what, I think that is a good idea; I'm going to modify this patch. Failing to resolve is an important data point in its own right.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add missing return value check (r2)

This patch is basically the same thing but before returning true we record a failure message to the telemetry probe.

Carrying forward r+.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/866cf6ccf7669a0a9532d91f235b670de20a6072
Bug 1375130: Add missing return value check in a11y::LazyInstantiator call to GetClientExecutableName; r=eeejay

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/894c7a554519c201fb0736df108ff2a7d09b49c2
Bug 1375130: Add missing guard around call to LazyInstantiator::AccumulateTelemetry; r=bustage

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/866cf6ccf766
https://hg.mozilla.org/mozilla-central/rev/894c7a554519

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Users on today's nightly are still crashing :(
https://crash-stats.mozilla.com/report/index/076e9b6d-8158-4b97-8dd4-c64800170623#tab-details

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Different bug. Moving those signatures over to bug 1375912.

Comment 10

[Julian Seward [:jseward]]

This is still visible as topcrash #6 in the Windows nightlies of 20170622030208.