1324863 - Crash in mozilla::a11y::DocAccessibleParent::RemoveChildDoc

Status: RESOLVED DUPLICATE of bug 1326084

(Core :: Disability Access APIs, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-251dc21e-f125-4301-b257-b37392161219.
=============================================================

Seen while looking at trunk data. This one started using 20161217030205: http://bit.ly/2gZ0frO

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=63b447888a6469b9f6ae8f76ac5f0d7c6ea239da&tochange=34a1ab064cb5b868fa75cb74d052e978eb34d6c1

Bug 1321936 is in the regression range - ni on aklotz

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Not related to 1321936. Could be fallout from bug 1314707 or bug 1321935. I don't think we should refer to this as a "regression," however, as cleaning up those bugs probably just means that whatever weird conditions were triggering those bugs are probably triggering this one too.

In particular it looks like the child doc being destroyed does not have a parent.

Comment 2

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

Is a null check the right fix?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

That would help with wallpapering over the crash, for sure, however I am concerned about the fact that we even reach that state in the first place.

Trev, what are your thoughts about this crash?

Comment 4

[Nicholas Nethercote [inactive]]

127 occurrences across Nightly and Aurora in the past 7 days.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to Nicholas Nethercote [:njn] (PTO until January 9th) from comment #4)
> 127 occurrences across Nightly and Aurora in the past 7 days.

#2 crash for nightly

Comment 6

[Nicholas Nethercote [inactive]]

#7 topcrash in Nightly 20161230030205. Still waiting for info from tbsaunde.

Comment 7

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

Trevor is back agreed to take a look at this.

Comment 8

[Trevor Saunders (:tbsaunde)]

> In particular it looks like the child doc being destroyed does not have a
> parent.

I see we have DocAccessibleParent::Destroy() calling DocAccessibleparent::Destroy() that suggests there is a document that has the child as its child, but the child doesn't know about the parent.  I'm tempted to go back to the checking patch in bug 1184217 to try and crash as soon as we corrupt the data structure.

Comment 9

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

<davidb> tbsaunde: are there any more findings we can add to bug 1324863?
<@tbsaunde> davidb: at the moment trying to fix other bugs first awith the hope its a dup of one of the others

Comment 10

[Nicholas Nethercote [inactive]]

#7 topcrash again in Nightly 20170113030227.

Comment 11

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

I think 100% of these crashes are amd64. Is that interesting?

Comment 12

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

Still crashing unfortunately. E.g. https://crash-stats.mozilla.com/report/index/bfe23e9c-8984-4526-a37a-d39d82170120

Comment 13

[Jim Mathies [:jimm]]

(In reply to David Bolter [:davidb] from comment #11)
> I think 100% of these crashes are amd64. Is that interesting?

These are all happening in 64-bit builds where we still have a11y getting activated by touchscreens. Fixing that would lessen the severity but wouldn't fix the core problem.

Comment 14

[Jim Mathies [:jimm]]

Only about 30% are shutdown crashes, so this is something users see while surfing. user comments back this up.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: paper over null parent proxy accessible

I'm not really a fan of this, but what's the worst that can happen?

Comment 16

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

Comment on attachment 8829627 [details] [diff] [review]
paper over null parent proxy accessible

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

Hhmmm okay... note this won't paper all the crashes.

Comment 17

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

(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to David Bolter [:davidb] from comment #11)
> > I think 100% of these crashes are amd64. Is that interesting?
> 
> These are all happening in 64-bit builds where we still have a11y getting
> activated by touchscreens. Fixing that would lessen the severity but
> wouldn't fix the core problem.

Yeah.

And it seems there is a signature "nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc" crash-stats treats separately which shows os arch as x86. So we're not just talking about amd64. E.g. https://crash-stats.mozilla.com/report/index/2bcfd386-c4ca-463f-863b-713ca2170123

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0498497a3282
paper over null parent proxy accessible r=davidb

Comment 19

[Marco Zehe (:MarcoZ)]

Don't know if this will help, but I just got a crash with this signature as I closed frozen Nightly from the task bar. Report is here:
https://crash-stats.mozilla.com/report/index/9cebbafb-ca75-4587-bcc7-1aa9b2170124

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Marking as sec bug.  Note that the crash address is mostly non-0 and non-nullptr-plus-offset.  Also, there are clear UAF signatures, and this likely would be used as object pointer for a method call (i.e. the vptr table will be used for execution addresses).

The wallpaper that landed has no chance of fixing this.  The problem appears to be that nothing is holding a reference to the object returned by Parent() (I presume).

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0498497a3282

Comment 22

[Trevor Saunders (:tbsaunde)]

(In reply to Randell Jesup [:jesup] from comment #20)
> Marking as sec bug.  Note that the crash address is mostly non-0 and
> non-nullptr-plus-offset.  Also, there are clear UAF signatures, and this
> likely would be used as object pointer for a method call (i.e. the vptr
> table will be used for execution addresses).
> 
> The wallpaper that landed has no chance of fixing this.  The problem appears

that is what was sort of obscurely said in earlier comments, but I was told it is 0 some of the time, I'm not sure if that is a different issue from the UAF or just random chance.  So obviously it doesn't fix things, but David seems to think avoiding the null crashes may be good, and I don't see it being any worse so *shrug*.

> to be that nothing is holding a reference to the object returned by Parent()
> (I presume).

well, its that something should be clearing the mParent pointer, but isn't.  Unfortunately tracking that down hasn't proved to be simple yet.

Comment 23

[Trevor Saunders (:tbsaunde)]

I think at this point we can say this is actually a dup

Comment 24

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

Comment on attachment 8829627 [details] [diff] [review]
paper over null parent proxy accessible

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

Parent is bad (and usually non-null) so this doesn't help us really.

::: accessible/ipc/DocAccessibleParent.h
@@ +113,5 @@
>    {
> +    ProxyAccessible* parent = aChildDoc->Parent();
> +    MOZ_ASSERT(parent);
> +    if (parent) {
> +      aChildDoc->Parent()->ClearChildDoc(aChildDoc);

Why not reuse the parent variable here? (Not that it makes a difference)

Comment 25

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

(In reply to David Bolter [:davidb] from comment #24)

> Parent is bad (and usually non-null) so this doesn't help us really.

Oh and comment 20. Thanks Randell.

Comment 26

[Calixte Denizet (:calixte)]

The signature "nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc" reappeared in nightly 54 with buildid 20170216030210 just after the fix for bug 1339472.
From 2017-02-16 to 2017-02-25, there are 343 crashes and most of them (94%) are a read on a null pointer [1].

[1] https://crash-stats.mozilla.com/search/?signature=~RemoveChildDoc&build_id=%3E%3D20170216000000&product=Firefox&version=54.0a1&platform=Windows&date=%3E%3D2017-02-12T17%3A03%3A00.000Z&date=%3C2017-02-25T17%3A03%3A00.000Z&_sort=-date&_facets=signature&_facets=build_id&_facets=address&_facets=reason&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-address