883708 - "ASSERTION: Accessible tree was created after it was modified" with table, changing visibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Enable accessibility.
2. Load the testcase.

###!!! ASSERTION: Accessible tree was created after it was modified! Huh?: 'Error', file accessible/src/base/EventQueue.cpp, line 217

Comment 1

[Trevor Saunders (:tbsaunde)]

Ok, so I don't completely understand this yet, but I'm not sure I have another gdb session left in me today, so dumping what I think I know.

All of this seems to happen while in a nested event loop showing some modal window, and in particular I think everything interesting happens in one refresh driver tick.

When onload fires the accessible looks like this

DocAccessible has one kid which is for the middle div without a a id attribute.  That accessible has one kid the inner most div with id=c. the outermost div and the <td> appear to be inaccessible.
First DocAccessibleContentRemoved(doc, innermost div) is called, and so we fire hide of innermost div and reorder on middle div.
Then ProcessConentInserted() is called for doc as container with inserted content the outermost div.  For that we make accessible for outermost div, and then fire show event on it and reorder on doc.  Then we assert because earlier hide / reorder is still in queue so earlier hide means sub tree of that newly accessible div  was already accessible which we assert about.

Comment 2

[alexander :surkov (:asurkov)]

the assert is about the case when shown event is fired for element that was a target of some reorder event, i.e. if show event is for outermost div then previous reorder should be for outermost div too but it contradicts with your first paragraph (reorder on middle div). What do I miss?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> the assert is about the case when shown event is fired for element that was
> a target of some reorder event, i.e. if show event is for outermost div then
> previous reorder should be for outermost div too but it contradicts with
> your first paragraph (reorder on middle div). What do I miss?

no, the assert is about case when previous reorder event's target is target of new show event or child of accessible being shown.  So what I said is fine because middle div  is child of outer one so reorder event's target is child of target of later show event.  See EventQueue.cpp:235 or so for why the assert includes children.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > the assert is about the case when shown event is fired for element that was
> > a target of some reorder event, i.e. if show event is for outermost div then
> > previous reorder should be for outermost div too but it contradicts with
> > your first paragraph (reorder on middle div). What do I miss?
> 
> no, the assert is about case when previous reorder event's target is target
> of new show event or child of accessible being shown.

same what I said except "or" part, where does it come from?

>  So what I said is
> fine because middle div  is child of outer one so reorder event's target is
> child of target of later show event.

anyway, does it happen that we get some tree mutations of something that will be shown (created) later?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > (In reply to alexander :surkov from comment #2)
> > > the assert is about the case when shown event is fired for element that was
> > > a target of some reorder event, i.e. if show event is for outermost div then
> > > previous reorder should be for outermost div too but it contradicts with
> > > your first paragraph (reorder on middle div). What do I miss?
> > 
> > no, the assert is about case when previous reorder event's target is target
> > of new show event or child of accessible being shown.
> 
> same what I said except "or" part, where does it come from?

I don't get question

> >  So what I said is
> > fine because middle div  is child of outer one so reorder event's target is
> > child of target of later show event.
> 
> anyway, does it happen that we get some tree mutations of something that
> will be shown (created) later?

yes, subtree is modified then tree is reparented from doc to new accessible being shown iirc its been a couple days now :/

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> yes, subtree is modified then tree is reparented from doc to new accessible
> being shown

do we allow reparenting?

>  iirc its been a couple days now :/

what do you mean?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > yes, subtree is modified then tree is reparented from doc to new accessible
> > being shown
> 
> do we allow reparenting?

I'm not sure what you mean by allow.  reparenting happens because invalidateChildren and then new accessible is cached and old children cached under that.

> >  iirc its been a couple days now :/
> 
> what do you mean?

just that my memory was a little fuzzy

Comment 8

[alexander :surkov (:asurkov)]

so seems like we should not not emit thisEvent in that case (coalesce it), at least that's tempting

but I'm curious what happens if outer div appearance is rolled back, we will destroy its whole subtree and then recreate it, right?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> so seems like we should not not emit thisEvent in that case (coalesce it),
> at least that's tempting

it seems to me that getting into this state is a bug and we shouldn't reparent because of InvalidateChildren()

> but I'm curious what happens if outer div appearance is rolled back, we will
> destroy its whole subtree and then recreate it, right?

I'd think / hope so, but I'm not really sure.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > so seems like we should not not emit thisEvent in that case (coalesce it),
> > at least that's tempting
> 
> it seems to me that getting into this state is a bug and we shouldn't
> reparent because of InvalidateChildren()

I thought of that but then I thought reparenting is not necessary bad idea if it has proper implementation. Do you know the path how reparenting happens?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > (In reply to alexander :surkov from comment #8)
> > > so seems like we should not not emit thisEvent in that case (coalesce it),
> > > at least that's tempting
> > 
> > it seems to me that getting into this state is a bug and we shouldn't
> > reparent because of InvalidateChildren()
> 
> I thought of that but then I thought reparenting is not necessary bad idea
> if it has proper implementation. Do you know the path how reparenting
> happens?

accessible for middle div becomes hanging by InvalidateChildren() then we create new accessible for outer div and CacheChildrenInSubtree() caches accessible for middle div as child of new accessible.

Comment 12

[alexander :surkov (:asurkov)]

So do you want something like DestorySubtree instead InvalidateChildren()? Do you see (possible) disadvantages of reparenting?

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> So do you want something like DestorySubtree instead InvalidateChildren()?

what would the difference be?

> Do you see (possible) disadvantages of reparenting?

Well, the way we do it right now is kind of a waste of effort in dealing with events.  But mostly I see fixing this as a necessary part of getting rid of InvalidateChildren()  I'm just not really sure how I want it to look yet, so I hope we can think about that.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to alexander :surkov from comment #12)
> > So do you want something like DestorySubtree instead InvalidateChildren()?
> 
> what would the difference be?

InvalidateChildren doesn't destroy the subtree (it unattaches the subtree)

> > Do you see (possible) disadvantages of reparenting?
> 
> Well, the way we do it right now is kind of a waste of effort in dealing
> with events.  But mostly I see fixing this as a necessary part of getting
> rid of InvalidateChildren()  I'm just not really sure how I want it to look
> yet, so I hope we can think about that.

I think I can live with either way. Reparenting sounds less predictable at this point but in the end it can be smarter approach (since it skip destroy-recreate phase).

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: bug 883708 - don't assert when parent of reorder event is shown later

I want to take care of reparenting in a nicer way in the future, but I think for now we're kind of fine just removing the assert though it would be nice to add it back later so we assert we don't do extra work

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 8346736 [details] [diff] [review]
bug 883708 - don't assert when parent of reorder event is shown later

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

can't we have a mochitest for that?

::: accessible/src/base/EventQueue.cpp
@@ +225,5 @@
>      }
>  
>      // If tailEvent contains thisEvent
>      // then
> +    //   if show or hide of tailEvent contains a grand parent of thisEvent

it'd be great if you add a comment below clarifying that show event case because it's not really evident

@@ +235,5 @@
>          AccReorderEvent* tailReorder = downcast_accEvent(aTailEvent);
>          uint32_t eventType = tailReorder->IsShowHideEventTarget(thisParent);
>  
> +        if (eventType == nsIAccessibleEvent::EVENT_SHOW
> +            || eventType == nsIAccessibleEvent::EVENT_HIDE) {

nit: || on previous line

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> Comment on attachment 8346736 [details] [diff] [review]
> bug 883708 - don't assert when parent of reorder event is shown later
> 
> Review of attachment 8346736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> can't we have a mochitest for that?

sure, I'll modify the test case.

> ::: accessible/src/base/EventQueue.cpp
> @@ +225,5 @@
> >      }
> >  
> >      // If tailEvent contains thisEvent
> >      // then
> > +    //   if show or hide of tailEvent contains a grand parent of thisEvent
> 
> it'd be great if you add a comment below clarifying that show event case
> because it's not really evident

saying what?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> saying what?

at least a link to this bug or (what's better) some extraction from your comment 1 and comment 3.

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: bug 883708 - don't assert when parent of reorder event is shown later

added test, and tried to fix nits

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 8348350 [details] [diff] [review]
bug 883708 - don't assert when parent of reorder event is shown later

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

::: accessible/src/base/EventQueue.cpp
@@ +239,5 @@
> +        // DocAccessible::CacheChildrenInSubtree() can conspire to reparent an
> +        // accessible in this case no need for mutation events.  Se bug 883708
> +        // for details.
> +        if (eventType == nsIAccessibleEvent::EVENT_SHOW ||
> +             eventType == nsIAccessibleEvent::EVENT_HIDE) {

nit: one extra space on second line (indentation issue)

::: accessible/tests/mochitest/treeupdate/test_bug883708.xhtml
@@ +11,5 @@
> +  var newSpan = document.createElementNS("http://www.w3.org/1999/xhtml", "span");
> +  c.insertBefore(newSpan, d);
> +  a.style.visibility = "visible";
> +  ok(true, "test didn't crash or assert");
> +  SimpleTest.finish();

what is the propose of the text? check we don't hit removed assertion? Should you check if we don't receive unexpected show event?

Comment 21

[Trevor Saunders (:tbsaunde)]

> ::: accessible/tests/mochitest/treeupdate/test_bug883708.xhtml
> @@ +11,5 @@
> > +  var newSpan = document.createElementNS("http://www.w3.org/1999/xhtml", "span");
> > +  c.insertBefore(newSpan, d);
> > +  a.style.visibility = "visible";
> > +  ok(true, "test didn't crash or assert");
> > +  SimpleTest.finish();
> 
> what is the propose of the text? check we don't hit removed assertion?
> Should you check if we don't receive unexpected show event?

just check no assertions, I'd rather leave more complicated tests to follow up mentored bug.

Comment 22

[alexander :surkov (:asurkov)]

ok, would you please file a follow up bug (transforming this test to events/test_coalescence.html to check no extra show event is fired)?

Comment 23

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85830cef2651

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85830cef2651