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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Calixte Denizet (:calixte)]

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

Comment 2

[Jesper Hansen]

Got hit by this today in the latest (54.0a1 (2017-02-21) (64-bit)) nightly while closing three tabs in succession.

Comment 3

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

Attachment: 1340579 diagnostics

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8839581 [details] [diff] [review]
1340579 diagnostics

we're not dealing with null pointers here, so that isn't going to help.

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: add another diagnostic assert

Comment 7

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

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18bc0b31d8c8
add another diagnostic assert r=davidb

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18bc0b31d8c8

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

Comment 12

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

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.

Comment 13

[Eitan Isaacson [:eeejay]]

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...

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5144d2aa92af for assertion failures and crashes, https://treeherder.mozilla.org/logviewer.html#?job_id=80124269&repo=mozilla-inbound being fairly typical of them.

Comment 17

[Nicholas Nethercote [inactive]]

This is the #5 Windows topcrash in Nightly 20170224030232.

Comment 18

[Nicholas Nethercote [inactive]]

(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 19

[Trevor Saunders (:tbsaunde)]

Attachment: look up this DocAccessibleParent in live docs instead of using this in DocAccessibleParent::Destroy()

Comment 20

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

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?

Comment 21

[Trevor Saunders (:tbsaunde)]

> ::: 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 22

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

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c3c56d71a7
fixup werror bustage landing on a CLOSED TREE

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c809b2f8930
https://hg.mozilla.org/mozilla-central/rev/05c3c56d71a7

Comment 26

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

Trevor let's err on the side of caution and get this uplifted to branches?

Comment 27

[Trevor Saunders (:tbsaunde)]

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

Comment 28

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

Is beta 53 affected? I don't see any crashes reported on 53 with these 3 signatures.

Comment 29

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

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

This landed on m-c when it was still tracking 54. AFAICT, there's nothing to uplift to Aurora here?

Comment 31

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

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.

Comment 32

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

Agreed.