Guilherme Lima Reporter
	Description • 9 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 1 • 9 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 9 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 9 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 9 years ago

(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)?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 9 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 6 • 9 years ago

ok, no magic macros then :) thank you!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 9 years ago

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

I'm pretty sure if (owner-IsDefunct()) { doesn't compile

	alexander :surkov (:asurkov) Assignee
	Comment 9 • 9 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 10 • 9 years ago

lost > is found

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6db43f49b67

	Robert Kaiser
	Comment 12 • 9 years ago

Is bug 1206107 related to this? Seems like both signatures were rising at the same time.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b6db43f49b67

	Guilherme Lima Reporter
	Comment 14 • 9 years ago

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

	Robert Kaiser
	Comment 15 • 9 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 16 • 9 years ago

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?

	alexander :surkov (:asurkov) Assignee
	Comment 18 • 9 years ago

(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

	Robert Kaiser
	Comment 19 • 9 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 20 • 9 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 21 • 9 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 22 • 9 years ago

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.

	Pulsebot
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/187cd7421f08

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 9 years ago

Backout: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/701a59dcfa76

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 9 years ago

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.

	Carsten Book [:Tomcat]
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/187cd7421f08

	Guilherme Lima Reporter
	Comment 27 • 9 years ago

Still not fixed =/

https://crash-stats.mozilla.com/report/index/1947ad95-ff46-4e2e-a89a-ebc562150924

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 29 • 9 years ago

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)

	alexander :surkov (:asurkov) Assignee
	Comment 30 • 9 years ago

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

	Pulsebot
	Comment 31 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/23ec08e5d23f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 32 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/23ec08e5d23f