Crash in PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible

RESOLVED FIXED in Firefox 54

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jseward, Assigned: tbsaunde)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla54
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 unaffected, firefox54 fixed)

Details

(Whiteboard: aes+, [clouseau], crash signature)

Attachments

(5 attachments)

This bug was filed from the Socorro interface and is 
report bp-5e81f704-9632-4e5e-baab-b6a402170217.
=============================================================

This is ranked #12 in the Windows crashes for Nightly 20170216030210.
There are 124 crashes for both signatures.
In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1339472.

[1] https://hg.mozilla.org/mozilla-central/rev?node=e9f0915303b008fd84af2775f5c8d637d894974e
Crash Signature: [@ PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible] → [@ PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible] [@ nsTHashtable<T>::GetEntry | mozilla::a11y::DocAccessibleParent::GetAccessible]
Flags: needinfo?(tbsaunde+mozbugs)
Got hit by this today in the latest (54.0a1 (2017-02-21) (64-bit)) nightly while closing three tabs in succession.
Attachment #8839581 - Flags: review?(tbsaunde+mozbugs)
Keywords: leave-open
Comment on attachment 8839581 [details] [diff] [review]
1340579 diagnostics

we're not dealing with null pointers here, so that isn't going to help.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8839581 - Flags: review?(tbsaunde+mozbugs)
Attachment #8839625 - Flags: review?(dbolter)
Duplicate of this bug: 1340490
Whiteboard: aes+
Assignee: nobody → tbsaunde+mozbugs
Comment on attachment 8839625 [details] [diff] [review]
add another diagnostic assert

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

Sorry for delay. Hope this tells us something.
Attachment #8839625 - Flags: review?(dbolter) → review+
Duplicate of this bug: 1341619
Blocks: 1326084
Crash Signature: [@ PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible] [@ nsTHashtable<T>::GetEntry | mozilla::a11y::DocAccessibleParent::GetAccessible] → [@ PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible] [@ nsTHashtable<T>::GetEntry | mozilla::a11y::DocAccessibleParent::GetAccessible] [@ mozilla::a11y::HyperTextAccessibleWrap::QueryInterface]
Comment on attachment 8840520 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

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

Redirecting review.
Attachment #8840520 - Flags: review?(dbolter) → review?(eitan)
Attachment #8840520 - Flags: review?(eitan)
Instead of dereferencing null, I think we should check for null and assert. that way, we don't crash! And hopefully resolve the mystery. Also, maybe have an assert that checks if we add the same key twice to the LiveDocs table. You never know...
Attachment #8841000 - Flags: review?(eitan) → review+
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb97347c9690
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy() r=eeejay
This is the #5 Windows topcrash in Nightly 20170224030232.
(In reply to Nicholas Nethercote [:njn] from comment #17)
> This is the #5 Windows topcrash in Nightly 20170224030232.

Actually... the "PLDHashTable::Search | mozilla::a11y::DocAccessibleParent::GetAccessible" signature is #5. The " 	mozilla::a11y::HyperTextAccessibleWrap::QueryInterface" is #6. If you combine them they would get to #4.
Comment on attachment 8842420 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

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

First pass - some questions.

::: accessible/ipc/DocAccessibleParent.cpp
@@ +482,5 @@
> +  for (uint32_t i = childDocCount - 1; i < childDocCount; i--) {
> +    DocAccessibleParent* thisDoc = LiveDocs().Get(actorID);
> +    MOZ_ASSERT(thisDoc);
> +    if (!thisDoc) {
> +      return;

Should this be break instead of return, or are we hosed enough at this point to know we should just completely bail?

@@ +495,5 @@
>      iter.Remove();
>    }
>  
>    // The code above should have already completely cleared these, but to be
>    // extra safe make sure they are cleared here.

nit:This comment should be moved down to where you do the clearing.

@@ +517,5 @@
> +  thisDoc = LiveDocs().Get(actorID);
> +  MOZ_ASSERT(thisDoc);
> +  if (!thisDoc) {
> +    return;
> +  }

Can you comment why you get and check thisDoc three times?

::: accessible/ipc/DocAccessibleParent.h
@@ +34,5 @@
>                                        , mEmulatedWindowHandle(nullptr)
>  #endif // defined(XP_WIN)
> +  {
> +    MOZ_COUNT_CTOR_INHERITED(DocAccessibleParent, ProxyAccessible);
> +    sMaxDocID++;

Do we ever run out?
> ::: accessible/ipc/DocAccessibleParent.cpp
> @@ +482,5 @@
> > +  for (uint32_t i = childDocCount - 1; i < childDocCount; i--) {
> > +    DocAccessibleParent* thisDoc = LiveDocs().Get(actorID);
> > +    MOZ_ASSERT(thisDoc);
> > +    if (!thisDoc) {
> > +      return;
> 
> Should this be break instead of return, or are we hosed enough at this point
> to know we should just completely bail?

if it happens it means the document we are operating on has gone away, so we'd skip most of this function anyway.

> @@ +517,5 @@
> > +  thisDoc = LiveDocs().Get(actorID);
> > +  MOZ_ASSERT(thisDoc);
> > +  if (!thisDoc) {
> > +    return;
> > +  }
> 
> Can you comment why you get and check thisDoc three times?

it seems likely its going away at some point, and I'm trying to handle that "safely"

> ::: accessible/ipc/DocAccessibleParent.h
> @@ +34,5 @@
> >                                        , mEmulatedWindowHandle(nullptr)
> >  #endif // defined(XP_WIN)
> > +  {
> > +    MOZ_COUNT_CTOR_INHERITED(DocAccessibleParent, ProxyAccessible);
> > +    sMaxDocID++;
> 
> Do we ever run out?

basically no, you'd need to run a browser for years creating millions of documents every second to get close.
Comment on attachment 8842420 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

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

r=me thanks.
Attachment #8842420 - Flags: review?(dbolter) → review+
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c809b2f8930
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy() r=davidb
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c3c56d71a7
fixup werror bustage landing on a CLOSED TREE
Whiteboard: aes+ → aes+, [clouseau]
Trevor let's err on the side of caution and get this uplifted to branches?
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8842420 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

Approval Request Comment
[Feature/Bug causing the regression]:unclear
[User impact if declined]:crashes that may be exploitable.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:the other patches in this bug
[Is the change risky?]:no, its pretty straight forward and shouldn't change behavior.
[Why is the change risky/not risky?]:
[String changes made/needed]:none
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8842420 - Flags: approval-mozilla-beta?
Attachment #8842420 - Flags: approval-mozilla-aurora?
Is beta 53 affected? I don't see any crashes reported on 53 with these 3 signatures.
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8842420 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

Crash fix, let's bring this to aurora.
Attachment #8842420 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: leave-open
Target Milestone: --- → mozilla55
This landed on m-c when it was still tracking 54. AFAICT, there's nothing to uplift to Aurora here?
Target Milestone: mozilla55 → mozilla54
Comment on attachment 8842420 [details] [diff] [review]
look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

As far as I can tell we don't have this issue on 53. Please let me know if that's not the case.
Flags: needinfo?(dbolter)
Attachment #8842420 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Agreed.
Flags: needinfo?(dbolter)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Flags: needinfo?(tbsaunde+mozbugs)
You need to log in before you can comment on or make changes to this bug.