Closed Bug 1268320 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

Unspecified
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox49 + fixed

People

(Reporter: jchen, Assigned: surkov)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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

Low volume crash in recently changed code
David, could you help with identifying this crash? Is it likely because mtEvent->GetDocAccessible() returns null?
bug 1267977 may be caused by same problem
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
This is the #4 topcrash in Nightly 20160501030217, with 16 crashes.
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?
Flags: needinfo?(dbolter)
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
Flags: needinfo?(dbolter)
Crash Signature: [@ PLDHashTable::Remove | mozilla::a11y::EventTree::Process] → [@ PLDHashTable::Remove | mozilla::a11y::EventTree::Process] [@ PLDHashTable::Remove | mozilla::a11y::DocAccessible::UnbindFromDocument]
Summary: crash in PLDHashTable::Remove | mozilla::a11y::EventTree::Process → crash in PLDHashTable::Remove | mozilla::a11y::DocAccessible::UnbindFromDocument
Tracking for 49.
Do we need to paper over this crash with a null check?
Assignee: nobody → surkov.alexander
(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.
Attached patch patchrenSplinter Review
rename AccEvent::GetDocAccessilbe() to Document()
Attachment #8753574 - Flags: review?(mzehe)
Attached patch patchSplinter Review
Nicholas, would you be interested to review the patch since you've been in touch with this code lately?
Attachment #8753576 - Flags: review?(n.nethercote)
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.
Attachment #8753576 - Flags: review?(n.nethercote)
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.
Attachment #8753576 - Flags: review?(yzenevich)
Attachment #8753574 - Flags: review?(mzehe) → review+
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 ;
Attachment #8753576 - Flags: review?(yzenevich) → review+
(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.
Attached patch patch v2Splinter Review
here's a more safe version
Attachment #8753862 - Flags: review?(yzenevich)
Comment on attachment 8753862 [details] [diff] [review]
patch v2

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

thanks
Attachment #8753862 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/ca34a07172f6
https://hg.mozilla.org/mozilla-central/rev/9b44a0d216be
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I see 10 occurrences of this in Nightly 20160520030251 and zero in Nightly 20160521030227.
Status: RESOLVED → VERIFIED
Nicholas, do we need to uplift this to 48? Have you seen any occurrences there?
Flags: needinfo?(n.nethercote)
(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 :)
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.