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

RESOLVED FIXED in mozilla29

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: tbsaunde)

Tracking

(Blocks: 4 bugs, {assertion, testcase})

Trunk
mozilla29
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 763338 [details]
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
(Assignee)

Updated

5 years ago
Blocks: 888531
(Assignee)

Comment 1

5 years ago
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.
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?
(Assignee)

Comment 3

5 years ago
(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.
(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?
(Assignee)

Comment 5

5 years ago
(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 :/
(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?
(Assignee)

Comment 7

5 years ago
(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
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?
(Assignee)

Comment 9

5 years ago
(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.
(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?
(Assignee)

Comment 11

5 years ago
(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.
So do you want something like DestorySubtree instead InvalidateChildren()? Do you see (possible) disadvantages of reparenting?
(Assignee)

Comment 13

5 years ago
(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.
(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).
(Assignee)

Comment 15

5 years ago
Created attachment 8346736 [details] [diff] [review]
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
Attachment #8346736 - Flags: review?(surkov.alexander)
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
Attachment #8346736 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

5 years ago
(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?
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 8348350 [details] [diff] [review]
bug 883708 - don't assert when parent of reorder event is shown later

added test, and tried to fix nits
Attachment #8346736 - Attachment is obsolete: true
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?
(Assignee)

Comment 21

5 years ago
> ::: 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.
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)?
(Assignee)

Updated

5 years ago
Blocks: 951228
https://hg.mozilla.org/mozilla-central/rev/85830cef2651
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.