Closed Bug 1205476 Opened 4 years ago Closed 4 years ago

crash in mozilla::a11y::DocAccessible::ProcessInvalidationList()

Categories

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

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 + fixed
firefox44 + fixed

People

(Reporter: guijoselito, Assigned: surkov)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-5bbbc1f8-9c21-42d8-9942-e7d632150916.
=============================================================

I just crashed twice on Facebook after updating to today's Nightly. Same signature both times.
The regression window would be http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c69e31de9aec2b9e8c0b3128ac6dc3897aa7381f&tochange=3e8dde8f8c174cce2c0b65c951808f88e35d1875

I have e10s off.
it looks like accessible is destroyed after it was added to the list. It makes sense to keep strong pointers to members of ARIAOwnsPair, but I'm not sure how to add them into cycle collection of DocAccessible, because ARIAOwnsPair struct is not cycle collectible itself but its members are.

Olli, may I ask you to take a look?
Flags: needinfo?(bugs)
Depends on: 1133213
What is the ownership here? Who is supposed to keep ARIAOwnsPair alive? Is that something cycle collectable? Is so, could it just traverse/unlink the members of ARIAOwnsPair?
Flags: needinfo?(bugs)
Flags: needinfo?(surkov.alexander)
DocAccessible seems to own mARIAOwnsInvalidationList. So one could just make member variables of  
ARIAOwnsPair strong and explicitly traverse/unlink them in DocAccessible's traverse/unlink.
(In reply to Olli Pettay [:smaug] from comment #3)
> DocAccessible seems to own mARIAOwnsInvalidationList. So one could just make
> member variables of  
> ARIAOwnsPair strong and explicitly traverse/unlink them in DocAccessible's
> traverse/unlink.

right, what macros is suitable for that, since I cannot do simple NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mARIAOwnsInvalidationList)?
Flags: needinfo?(surkov.alexander)
Manually iterate the array there

for (uint32_t i = 0; i < tmp->mARIAOwnsInvalidationList.Length(); ++i) {
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(tmp->mARIAOwnsInvalidationList[i].mMemberVariableFooBar)
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(tmp->mARIAOwnsInvalidationList[i].mMemberVariableBarFoo)
}
and similar for unlink
ok, no magic macros then :) thank you!
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #8662446 - Flags: review?(bugs)
Comment on attachment 8662446 [details] [diff] [review]
patch

I'm pretty sure if (owner-IsDefunct()) { doesn't compile
Attachment #8662446 - Flags: review?(bugs) → review-
not sure how but it compiles https://treeherder.mozilla.org/#/jobs?repo=try&revision=491792366bdb :), tests fail

this one https://treeherder.mozilla.org/#/jobs?repo=try&revision=7de088670734 looks ok. Do you need a new patch for that missed bit or just flip over that r-?
Attached patch patchSplinter Review
lost > is found
Attachment #8662446 - Attachment is obsolete: true
Attachment #8662907 - Flags: review?(bugs)
Attachment #8662907 - Flags: review?(bugs) → review+
Is bug 1206107 related to this? Seems like both signatures were rising at the same time.
https://hg.mozilla.org/mozilla-central/rev/b6db43f49b67
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Sorry to say, but the patch didn't fix the crash. I still crash while scrolling the list of friends on the right side of Facebook.

Crash report from last crash, after updating to Sep 19th build: https://crash-stats.mozilla.com/report/index/0b6f1787-3e69-4bb2-a626-d0c082150919
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[Tracking Requested - why for this release]:
The crashes in this bug and bug 1206107 together are more than all other crashes for Nightly 43 together in recent days. We cannot ship Dev Edition updates with this kind of crash issue.
The timing is unfortunate here. I wonder if we could revert the aria-owns bug 1133213 before uplift. Kairo is that the preferred option here? Or,

Alexander do you have a likely fix in mind?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(kairo)
Attached patch patchSplinter Review
Flags: needinfo?(surkov.alexander)
Attachment #8663693 - Flags: review?(dbolter)
(In reply to David Bolter [:davidb] from comment #16)
> 
> Alexander do you have a likely fix in mind?

it's hard what caused it but let's try this one
We can also revert after uplift, but then we'd need to do it twice if we want both dev edition and nightly to have it backed out.

(In reply to alexander :surkov from comment #18)
> it's hard what caused it but let's try this one

At least the data suggests that something that landed on 2015-09-15 has cased it as on the 16th those signatures started spiking. That said, the mozilla::a11y::Accessible::HasGenericType signature of bug 1206107 did exist previously at a lower level, but hugely spiked with the 0916 build (and later ones).
Flags: needinfo?(kairo)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #19)
> We can also revert after uplift, but then we'd need to do it twice if we
> want both dev edition and nightly to have it backed out.
> 
> (In reply to alexander :surkov from comment #18)
> > it's hard what caused it but let's try this one
> 
> At least the data suggests that something that landed on 2015-09-15 has
> cased it as on the 16th those signatures started spiking. That said, the
> mozilla::a11y::Accessible::HasGenericType signature of bug 1206107 did exist
> previously at a lower level, but hugely spiked with the 0916 build (and
> later ones).

I think we have a bug to blame. I meant I'm not sure what code paths may lead to the crash.
Comment on attachment 8663693 [details] [diff] [review]
patch

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

OK.

Let's be ready to revert on Aurora though...

::: accessible/generic/DocAccessible.cpp
@@ +1347,5 @@
>  
>    // Alter the tree according to aria-owns (seize the trees).
>    for (uint32_t idx = 0; idx < mARIAOwnsInvalidationList.Length(); idx++) {
>      Accessible* owner = mARIAOwnsInvalidationList[idx].mOwner;
> +    if (!owner->IsInDocument()) { // eventually died until we've got here

Well yeah.
Attachment #8663693 - Flags: review?(dbolter) → review+
Comment on attachment 8663693 [details] [diff] [review]
patch

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

::: accessible/generic/DocAccessible.cpp
@@ +1347,5 @@
>  
>    // Alter the tree according to aria-owns (seize the trees).
>    for (uint32_t idx = 0; idx < mARIAOwnsInvalidationList.Length(); idx++) {
>      Accessible* owner = mARIAOwnsInvalidationList[idx].mOwner;
> +    if (!owner->IsInDocument()) { // eventually died until we've got here

Please update the comment.
I'm not sure how this bug should end up since I backed out this, bug 1206165, and bug 1133213 on Aurora but not on trunk.
Flags: needinfo?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/187cd7421f08
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla43 → mozilla44
Still not fixed =/

https://crash-stats.mozilla.com/report/index/1947ad95-ff46-4e2e-a89a-ebc562150924
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v3Splinter Review
Flags: needinfo?(surkov.alexander)
Attachment #8665404 - Flags: review?(dbolter)
Comment on attachment 8665404 [details] [diff] [review]
patch v3

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

Was thinking similarly. (Wonder if we need the other checks anymore -- I guess doesn't hurt)
Attachment #8665404 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #29)

> Was thinking similarly. (Wonder if we need the other checks anymore -- I
> guess doesn't hurt)

the accessible may go away before we process the invalidation queue, so those checks are reasonable. But no good guess about missing parent.
https://hg.mozilla.org/mozilla-central/rev/23ec08e5d23f
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.