1268320 - crash in PLDHashTable::Remove | mozilla::a11y::DocAccessible::UnbindFromDocument

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-86d1f3b9-59b2-4688-9a55-4bee82160427.
=============================================================

Low volume crash in recently changed code

Comment 1

[alexander :surkov (:asurkov)]

David, could you help with identifying this crash? Is it likely because mtEvent->GetDocAccessible() returns null?

Comment 2

[alexander :surkov (:asurkov)]

bug 1267977 may be caused by same problem

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #0)
> report bp-86d1f3b9-59b2-4688-9a55-4bee82160427.

(In reply to alexander :surkov from comment #1)
> David, could you help with identifying this crash? Is it likely because
> mtEvent->GetDocAccessible() returns null?

We're calling PLDHashTable::Remove with this=0xe0.  The return address on the stack is to mozilla::a11y::EventTree::Process, but it's to the instruction after a call to mozilla::a11y::Accessible::ShutdownChildrenInSubtree, which ends with a tail call to mozilla::a11y::DocAccessible::UnbindFromDocument, which ends with a tail call to PLDHashTable::Remove.  So that explains how the call was made, but it's a little hard to figure out the underlying state.  It does seem consistent with calling DocAccessible::UnbindFromDocument with a null this pointer (since it looks like it's possible to get through the entire function with a null this pointer except for the very last line), although I didn't check that 0xE0 is the offset of mAccessibleCache in DocAccessible on amd64.  Likewise it seems possible for ShutdownChildrenInSubtree to execute completely with a null this pointer through to the UnbindFromDocument call at the end.

So mtEvent->GetDocAccessible() being null seems entirely possible.

Comment 4

[Nicholas Nethercote [inactive]]

This is the #4 topcrash in Nightly 20160501030217, with 16 crashes.

Comment 5

[alexander :surkov (:asurkov)]

No ideas so far how a hide event target may be shutdown before the event is processed, are there any URLs I can try to reproduce?

Comment 6

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

Last 28 days: about 60 crashes on Windows, 1 on Linux, none on Mac.

* 30 crashes are generic: https://www.facebook.com/
* Maybe are longer facebook Urls, e.g: 2 crashes on https://www.facebook.com/4UStudio2015520/
* https://de.wikipedia.org/wiki/DM51

All crashes seem to be on amd64

Comment 7

[Marcia Knous [:marcia]]

Tracking for 49.

Comment 8

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

Do we need to paper over this crash with a null check?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #8)
> Do we need to paper over this crash with a null check?

I think so, I want to try to reproduce it before that though.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patchren

rename AccEvent::GetDocAccessilbe() to Document()

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch

Nicholas, would you be interested to review the patch since you've been in touch with this code lately?

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8753576 [details] [diff] [review]
patch

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

I am not an appropriate reviewer for this patch, sorry.

Also, it makes things easier for reviewers if you explain what a patch is doing. You haven't even included a commit message in this patch. So even if I was an appropriate reviewer for this patch, I wouldn't be comfortable reviewing it without that information.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8753576 [details] [diff] [review]
patch

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

ok, the issue is rather trivial, when event is fired then listener can destroy the document, I added mochitests illustrating the problem.

Comment 15

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

Comment on attachment 8753576 [details] [diff] [review]
patch

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

Looks good with 2 nits and 1 question

::: accessible/base/EventTree.cpp
@@ +206,5 @@
>      MOZ_ASSERT(mtEvent->mEventRule != AccEvent::eDoNotEmit,
>                 "The event shouldn't be presented in the tree");
>  
>      nsEventShell::FireEvent(mtEvent);
> +    if (!mtEvent->Document()) {

Just curious , here and below when we return, what happens with other mDependentEvents? They aren't to be fired?

::: accessible/tests/mochitest/events/test_mutation.html
@@ +380,5 @@
> +    function hideNDestroyDoc()
> +    {
> +      this.txt = null;
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_HIDE, function() { return this.txt }.bind(this))

missing ; in return statement.

@@ +404,5 @@
> +    function hideHideNDestroyDoc()
> +    {
> +      this.target = null;
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_HIDE, function() { return this.target }.bind(this))

missing ;

Comment 16

[alexander :surkov (:asurkov)]

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

> Just curious , here and below when we return, what happens with other
> mDependentEvents? They aren't to be fired?

if their document was shut down then they don't make any sense anymore. There's other theoretical problematic path here: if we've got a shutdown accessible before we processed its event (which shouldn't be possible but all impossible things tends to happen one day), then in that case we'll miss those sibling events.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch v2

here's a more safe version

Comment 18

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

Comment on attachment 8753862 [details] [diff] [review]
patch v2

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

thanks

Comment 19

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca34a07172f67508bd75964c0b617e8a344d8397
Bug 1268320 - rename AccEvent::GetDocAccessible to AccEvent::Document, r=marcoz

Comment 20

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b44a0d216be66c4ac1a05d344a838a5056692e9
Bug 1268320 - stop event tree processing if the document goes shutdown, r=yzen

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/ca34a07172f6
https://hg.mozilla.org/mozilla-central/rev/9b44a0d216be

Comment 22

[Nicholas Nethercote [inactive]]

I see 10 occurrences of this in Nightly 20160520030251 and zero in Nightly 20160521030227.

Comment 23

[Marco Zehe (:MarcoZ)]

Nicholas, do we need to uplift this to 48? Have you seen any occurrences there?

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to Marco Zehe (:MarcoZ) from comment #23)
> Nicholas, do we need to uplift this to 48? Have you seen any occurrences
> there?

I don't see any in 48. Thank you for asking :)