1263188 - crash in PLDHashTable::Search | mozilla::a11y::DocAccessible::UnbindFromDocument

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This bug was filed from the Socorro interface and is 
report bp-571c42ab-8b06-421d-ae90-aec012160408.
=============================================================
I encountered this while alt-tabbing back into today's Nightly. I had an IRCCloud chat tab open with Davidb at the time. 

1. I had alt-tabbed to the desktop, 
2. launched another app, 
3. and when I alt-tabbed back into Nightly, it immediately crashed.

In the meantime, David had sent in a few private messages to me. So the page I was about to come back to wasn't the same I had left earlier.

I suspect this is fall-out from bug 1261425, which landed between the 2016-04-07 and 2016-04-08 builds, and I'd never seen this crash before.

Comment 1

[Benjamin Smedberg]

[Tracking Requested - why for this release]:

I'm experiencing this every time I search in the gmail interface.

Comment 2

[Nicholas Nethercote [inactive]]

We have 998 crash reports in Nightly in the past three days. That's a huge number. They started in Nightly 20160408030212.

There are also several other signatures that started at the same time and look like they are related:

> PLDHashTable::Search | nsBaseHashtable<T>::Get
> PLDHashTable::Remove | mozilla::a11y::DocAccessible::ShutdownChildrenInSubtree 
> PLDHashTable::Search | nsInterfaceHashtable<T>::Get
> PLDHashTable::Search | nsClassHashtable<T>::Get

They range from a handful to 100+.

The crash looks like it might be a null deref of mNodeToAccessibleMap?

Surkov, bug 1261425 landed on April 7. Could it be the cause? The crash rate is extreme, and if it's at all possible that bug is the cause then its patch should be backed out. Thank you.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> We have 998 crash reports in Nightly in the past three days. That's a huge
> number. They started in Nightly 20160408030212.
> 
> There are also several other signatures that started at the same time and
> look like they are related:
> 
> > PLDHashTable::Search | nsBaseHashtable<T>::Get
> > PLDHashTable::Remove | mozilla::a11y::DocAccessible::ShutdownChildrenInSubtree 
> > PLDHashTable::Search | nsInterfaceHashtable<T>::Get
> > PLDHashTable::Search | nsClassHashtable<T>::Get
> 
> They range from a handful to 100+.
> 
> The crash looks like it might be a null deref of mNodeToAccessibleMap?
> 
> Surkov, bug 1261425 landed on April 7. Could it be the cause? The crash rate
> is extreme, and if it's at all possible that bug is the cause then its patch
> should be backed out. Thank you.

It is indeed bug 1261425, I was able to reproduce in gmail like Benjamin, I hope to have a fix tomorrow or so.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: stop bleeding

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8739828 [details] [diff] [review]
stop bleeding

>diff --git a/accessible/base/EventTree.cpp b/accessible/base/EventTree.cpp
>--- a/accessible/base/EventTree.cpp
>+++ b/accessible/base/EventTree.cpp
>@@ -214,17 +214,17 @@ EventTree::Process()
>         // handling.
>         if (mtEvent->mAccessible->ARIARole() == roles::MENUPOPUP) {
>           nsEventShell::FireEvent(nsIAccessibleEvent::EVENT_MENUPOPUP_END,
>                                   mtEvent->mAccessible);
>         }
> 
>         AccHideEvent* hideEvent = downcast_accEvent(mtEvent);
>         if (hideEvent->NeedsShutdown()) {
>-          mContainer->Document()->ShutdownChildrenInSubtree(hideEvent->mAccessible);
>+          mtEvent->GetDocAccessible()->ShutdownChildrenInSubtree(mtEvent->mAccessible);
>         }
>       }
>     }
> 
>     // Fire reorder event at last.
>     if (mFireReorder) {
>       nsEventShell::FireEvent(nsIAccessibleEvent::EVENT_REORDER, mContainer);
>     }

Comment 6

[Ioana Chiorean]

I started getting this on Nightly(04/10 build) when playing youtube: dismissing advertisements, back, fw
Ubuntu 14.04

Comment 7

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ba2370b0726f39d103a197349a2f6be8f6bc97
Bug 1263188 - stop bleeding, r=yzen

Comment 8

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

Note:

* 2016-01-13 was when this sig started (a11y active[1] for these crashes):
** PLDHashTable::Search | nsClassHashtable<T>::Get

* 2016-04-08 was when this sig started showing up:
** PLDHashTable::Search | mozilla::a11y::DocAccessible::UnbindFromDocument

Alex do you expect the current patch to fix both?

1. https://crash-stats.mozilla.com/signature/?product=Firefox&version=48.0a1&date=%3C2016-04-11T19%3A02%3A31&date=%3E%3D2016-04-04T19%3A02%3A31&signature=PLDHashTable%3A%3ASearch+%7C+nsClassHashtable%3CT%3E%3A%3AGet&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=accessibility&page=1

Comment 9

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3edc9f9a1a68

Comment 10

[alexander :surkov (:asurkov)]

Attachment: part1: fix event tree cutting

Comment 11

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

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

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8740135 [details] [diff] [review]
part1: fix event tree cutting

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

thanks, just one question.

::: accessible/base/EventTree.cpp
@@ +479,5 @@
>    // discard those subtree mutations as we are no longer interested in them.
> +  nsAutoPtr<EventTree>* node = &mFirst;
> +  while (*node) {
> +    if ((*node)->mContainer == aEv->mAccessible) {
> +      *node = Move((*node)->mNext);

Are we not clearing subtree mutations?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #12)

> >    // discard those subtree mutations as we are no longer interested in them.
> > +  nsAutoPtr<EventTree>* node = &mFirst;
> > +  while (*node) {
> > +    if ((*node)->mContainer == aEv->mAccessible) {
> > +      *node = Move((*node)->mNext);
> 
> Are we not clearing subtree mutations?

we do, when we change *node, then it's previous object is destroyed, since it is nsAutoPtr, desctructor is called, which calls Clear().

Comment 14

[alexander :surkov (:asurkov)]

Attachment: part2: fix event tree building

Comment 15

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/547dbf605e71dcffcca5f735dbefdf55d0e0423a
Bug 1263188 - fix event tree cutting, part1, r=yzen

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to alexander :surkov from comment #13)
> (In reply to Yura Zenevich [:yzen] from comment #12)
> 
> > >    // discard those subtree mutations as we are no longer interested in them.
> > > +  nsAutoPtr<EventTree>* node = &mFirst;
> > > +  while (*node) {
> > > +    if ((*node)->mContainer == aEv->mAccessible) {
> > > +      *node = Move((*node)->mNext);
> > 
> > Are we not clearing subtree mutations?
> 
> we do, when we change *node, then it's previous object is destroyed, since
> it is nsAutoPtr, desctructor is called, which calls Clear().

gotcha

Comment 17

[alexander :surkov (:asurkov)]

Attachment: part3: more assertions

Comment 18

[alexander :surkov (:asurkov)]

Attachment: part4: fix event coalescense

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/547dbf605e71

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8740537 [details] [diff] [review]
part2: fix event tree building

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

couple of comments

::: accessible/base/EventTree.cpp
@@ +327,5 @@
>      prevNode = node;
>    } while ((node = node->mNext));
>  
>    MOZ_ASSERT(prevNode, "Nowhere to insert");
> +  MOZ_ASSERT(!prevNode->mNext, "A bed is taken");

Not sure I get this message.

@@ +332,5 @@
> +
> +  // If 'this' node contains the given container accessible, then
> +  //   do not emit a reorder event for the container
> +  //   if a dependent show event target contains the given container then do not
> +  //      emit show / hide events (to be done)

nit: indentation.

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8740671 [details] [diff] [review]
part3: more assertions

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

::: accessible/base/EventTree.cpp
@@ +230,4 @@
>  
> +  // Fire reorder event at last.
> +  if (mFireReorder) {
> +    MOZ_ASSERT(mContainer);

nit: message?

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8740673 [details] [diff] [review]
part4: fix event coalescense

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

looks good

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #20)

> >    MOZ_ASSERT(prevNode, "Nowhere to insert");
> > +  MOZ_ASSERT(!prevNode->mNext, "A bed is taken");
> 
> Not sure I get this message.

It was an allusion to Masha and the bear folk tale. I meant that mNext must be a null. Should I change it to that message or remove it or replace on something else?

Comment 24

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

(In reply to alexander :surkov from comment #23)
> (In reply to Yura Zenevich [:yzen] from comment #20)
> 
> > >    MOZ_ASSERT(prevNode, "Nowhere to insert");
> > > +  MOZ_ASSERT(!prevNode->mNext, "A bed is taken");
> > 
> > Not sure I get this message.
> 
> It was an allusion to Masha and the bear folk tale. I meant that mNext must
> be a null. Should I change it to that message or remove it or replace on
> something else?

We like contributors, even ones that don't know the Masha story.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #21)
> Comment on attachment 8740671 [details] [diff] [review]
> part3: more assertions
> 
> Review of attachment 8740671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/EventTree.cpp
> @@ +230,4 @@
> >  
> > +  // Fire reorder event at last.
> > +  if (mFireReorder) {
> > +    MOZ_ASSERT(mContainer);
> 
> nit: message?

I'm not sure what a good message might be here. Having "container shouldn't be null" repeats the assert's expression itself, also the assertion is right before a line where we attempt to use it.

Comment 26

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8bded39b76cb9f32ded2f988209d46695d45ae
Bug 1263188 - fix event tree building, part2, r=yzen

Comment 27

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

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

Comment 28

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97c95a625a6fe8a160409ef3f603b783565c262c
Bug 1263188 - more assertions, part3, r=yzen

Comment 29

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6500ef91810315bd2a4103b0b9ada0207c2b3686
Bug 1263188 - fix event tree coalescence, part4, r=yzen

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97c95a625a6f
https://hg.mozilla.org/mozilla-central/rev/6500ef918103

Comment 31

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

Recent regression, tracking in case it reopens