1273020 - Crash in mozilla::a11y::ApplicationAccessibleWrap::Name

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-48636586-4760-4b42-8f36-2e49a2160515.
=============================================================

25 occurrences of this crash in the past week, mostly in 45 and 46 but with one in 49, and all on Linux.

The crash address is always 0x0, so it looks like a straightforward null-deref crash. I think ApplicationAccessible::mAppInfo is null -- it's initialized in the constructor, but |do_GetService("@mozilla.org/xre/app-info;1");| is fallible and so can return nullptr -- every other call like that in the codebase does a null check.

The obvious fix is to just add null checks in the few places where mAppInfo is used.

This lack of null check looks like it predates bug 779520, which happened in 2012.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add missing null checks in ApplicationAccessible

I'm assuming that not adding text to the aVersion string is a reasonable thing
to do if mAppInfo is null.

Comment 2

[Trevor Saunders (:tbsaunde)]

> The crash address is always 0x0, so it looks like a straightforward
> null-deref crash. I think ApplicationAccessible::mAppInfo is null -- it's
> initialized in the constructor, but
> |do_GetService("@mozilla.org/xre/app-info;1");| is fallible and so can
> return nullptr -- every other call like that in the codebase does a null
> check.

That wouldn't suprise me, but I am rather curious how do_GetService() fails, its not obvious to me from looking at nsAppRunner.cpp

> The obvious fix is to just add null checks in the few places where mAppInfo
> is used.

I'm tempted to try and replace most of the do_GetService() usage with some C++ only function that is infallible, but I'm not sure its really worth it.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8752639 [details] [diff] [review]
Add missing null checks in ApplicationAccessible

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

I might misread the code, but could it because MAI_AKT_OBJECT has been created for application accessible, but wasn't destroyed at application accessible Shutdown() (because ApplicationAccessilbe::Shutdown() doesn't call AccessibleWrap::Shutdown), and then the caller triggers application accessible Name()?

Anyway, r=me for the patch as it has to address the crash, if no better solution can be found. I would put some assertions though to not mask the problem.

Comment 4

[Nicholas Nethercote [inactive]]

Getting the appinfo is clearly a fallible operation, and it's clearly happening for users. Making it assert in debug builds in this case doesn't seem useful.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bfdf5dfcf6bcf706fea4cda201f72ffc0c69c4a
Bug 1273020 - Add missing null checks in ApplicationAccessible. r=surkov.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Getting the appinfo is clearly a fallible operation, and it's clearly
> happening for users.

that may mean that Firefox is not usable for screen reader users

> Making it assert in debug builds in this case doesn't
> seem useful.

why? what are other scenarios to catch the problem?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to alexander :surkov from comment #6)
> (In reply to Nicholas Nethercote [:njn] from comment #4)
> > Getting the appinfo is clearly a fallible operation, and it's clearly
> > happening for users.
> 
> that may mean that Firefox is not usable for screen reader users

If this isn't the right way to fix the problem, then let's fix it in a better way. But triggering an abort in debug builds is not going to help anyone.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> (In reply to alexander :surkov from comment #6)
> > (In reply to Nicholas Nethercote [:njn] from comment #4)
> > > Getting the appinfo is clearly a fallible operation, and it's clearly
> > > happening for users.
> > 
> > that may mean that Firefox is not usable for screen reader users
> 
> If this isn't the right way to fix the problem, then let's fix it in a
> better way.

agreed, but nobody seems aware of a proper way

> But triggering an abort in debug builds is not going to help
> anyone.

debug builds are usually running by developers, who can report a problem, or the assertion can be triggered by automated testing.

Comment 9

[Nicholas Nethercote [inactive]]

> debug builds are usually running by developers, who can report a problem, or
> the assertion can be triggered by automated testing.

This defect was present in the code for multiple years without apparently being triggered by developers running debug builds or automated testing.

If you want to add assertions, please go ahead, and you can r=njn them.

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/798f02387d36477a9f2720cc4b632089adb74925
Bug 1273020 - add more assertions, r=njn

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8bfdf5dfcf6b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/798f02387d36

Comment 13

[Joanmarie Diggs]

Any chance this can be put back into Thunderbird stable? I'm getting a reliable crash when starting Thunderbird 45.7 (Fedora 25) and the ESR 45.2.0 if Orca is running and the Profile Manager used.
https://crash-stats.mozilla.com/report/index/075572b7-9cc9-4c91-a8d4-ec9c82170208

Comment 14

[Marco Zehe (:MarcoZ)]

As said on IRC: Don't know how much longer Thunderbird will be on 45, with 52, which will also go ESR, just around the corner, weeks, not months, away. Setting the affected Flag nonetheless.

Comment 15

[Alex ARNAUD]

(In reply to Marco Zehe (:MarcoZ) from comment #14)
> As said on IRC: Don't know how much longer Thunderbird will be on 45, with
> 52, which will also go ESR, just around the corner, weeks, not months, away.
> Setting the affected Flag nonetheless.

Dear Marco and all,

As I can see on the schedule page on wiki the next Firefox 52 ESR will be released in June so in 2 very long months. Could it be possible to backports the fix in the Firefox 45.X branch?

Best regards.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 8752639 [details] [diff] [review]
Add missing null checks in ApplicationAccessible

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crashes in Thunderbird and Firefox, see https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3AApplicationAccessibleWrap%3A%3AName&date=%3E%3D2017-02-23T15%3A46%3A00.000Z&date=%3C2017-03-02T15%3A46%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports
Fix Landed on Version:49
Risk to taking this patch (and alternatives if risky): low (few null checks)
String or UUID changes made by this patch:  no

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8752639 [details] [diff] [review]
Add missing null checks in ApplicationAccessible

AIUI 52-based thunderbird is released now so this shouldn't be necessary?