Trevor Saunders (:tbsaunde) Reporter
	Description • 10 years ago

to reproduce just load google.com (with e10s on this will crash, otherwise there will just be missing events)

The problem is that CacheChildrenInSubtree() can create accessibles but it doesn't know if they're new or just being recached because of InvalidateChildren().  CacheChildrenInSubtree() can create child accessibles outside of those created in UpdateTree() which should be responsible for firing events.

related IRC discussion:
< tbsaunde> surkov: have a few minutes to see if you can reproduce something?
<@surkov> sure, what do I need for that?
< tbsaunde> surkov: I think just opening google with a debugger open
< tbsaunde> I'm seeing ProcessContentInserted() called with an array of one element which is a br in a div in a html input and when its called for that it creates an accessible from CacheChildrenInsubtree
<@surkov> ok, next?
-!- icaaq|afk is now known as icaaq
< tbsaunde> surkov: you see that too?
<@surkov> tbsaunde: I didn’t try but that sounds reasoable, what’s wrong do you see in that?
<@surkov> ProcessContentInserted() is supposed to call CacheChildrenInsubtree to create a subtree
< tbsaunde> surkov: well, it means accessibles are created and added to subtree, but there's no show event
<@surkov> UpdateTree should take care of it
<@surkov> is that CacheChildrenInsubtree called from UpdateTree()?
< tbsaunde> surkov: should all accessible creation happen through UpdateTree() ?
< tbsaunde> surkov: no, directly
<@surkov> oh, that one
<@surkov> and that UpdateTree() fails to handle that?
< tbsaunde> surkov: yeah, seems so
<@surkov> quite strange
< tbsaunde> the inserted content UpdateTree is called for is just the br, but some random other div gets a new accessible
< tbsaunde> surkov: can you into it please?
<~davidb> is there an editable involved?
<@surkov> I guess that div is a parent
<@surkov> I guess I can take a look, can you file a bug?
< tbsaunde> davidb: yeah, there's some inputs around

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

can you think of test case? In general it sounds like layout issue since we should have insertion notification for that div before 'br' or even instead it.

	Trevor Saunders (:tbsaunde) Reporter
	Comment 2 • 10 years ago

(In reply to alexander :surkov from comment #1)
> can you think of test case? In general it sounds like layout issue since we
> should have insertion notification for that div before 'br' or even instead
> it.

google.com is the only thing I have, its probably easiest to get something smaller by seeing what happens in gdb.

	Trevor Saunders (:tbsaunde) Reporter
	Comment 3 • 10 years ago

Or maybe you could go back and find some of the other bugs where stuff like this has happened and try loading those test cases with e10s and see what happens.

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

can you check this build out if it helps https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-349a29a77c03/

	Trevor Saunders (:tbsaunde) Reporter
	Comment 5 • 10 years ago

(In reply to alexander :surkov from comment #4)
> can you check this build out if it helps

not easily, just a patch would be much  easier.

	Trevor Saunders (:tbsaunde) Reporter
	Comment 6 • 10 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > can you check this build out if it helps
> 
> not easily, just a patch would be much  easier.

actually I managed to dig up the patch from tbpl, and it seems to fix the issue.

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

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> actually I managed to dig up the patch from tbpl, and it seems to fix the
> issue.

cool, thanks for checking it out. I'll finish it up then.

	Trevor Saunders (:tbsaunde) Reporter
	Comment 9 • 10 years ago

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

>+   * Cache children if necessary. Return false is no caching is required.
>    */
>   bool EnsureChildren();

nothing checks the return value right? so just make it return void.

>+  inline void SetSurvivingInUpdate(bool aIsSurviving)

nit, inline is redundant

> 
>   static const uint8_t kChildrenFlagsBits = 2;
>-  static const uint8_t kStateFlagsBits = 8;
>+  static const uint8_t kStateFlagsBits = 9;
>   static const uint8_t kContextFlagsBits = 1;
>   static const uint8_t kTypeBits = 6;
>   static const uint8_t kGenericTypesBits = 13;

looks like we're running low on space again :/

>+    // We have a DOM/layout change under the container accessible, and its tree
>+    // might need an update. Since DOM/layout change of the element may affect
>+    // on the accessibleness of adjacent elements (for example, insertion of
>+    // extra HTML:body make the old body accessible) then we have to recache
>+    // children of the container, and then fire show/hide events for a change.
>+    UpdateTreeOnInsertion(container);
>+    break;

that's rather strange loop.

>     var gQueue = null;
>-    //gA11yEventDumpToConsole = true; // debug stuff
>+    gA11yEventDumpToConsole = true; // debug stuff

remember to disable it

>-      testAccessibleTree("listbox", accTree);
>+  //    testAccessibleTree("listbox", accTree);

why do you disable it?

>                 ]
>-},
>+              },

extranious whitespace changes are really annoying.

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

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> >   static const uint8_t kChildrenFlagsBits = 2;
> >-  static const uint8_t kStateFlagsBits = 8;
> >+  static const uint8_t kStateFlagsBits = 9;
> >   static const uint8_t kContextFlagsBits = 1;
> >   static const uint8_t kTypeBits = 6;
> >   static const uint8_t kGenericTypesBits = 13;
> 
> looks like we're running low on space again :/

yeah, unfortunately

> >+    // We have a DOM/layout change under the container accessible, and its tree
> >+    // might need an update. Since DOM/layout change of the element may affect
> >+    // on the accessibleness of adjacent elements (for example, insertion of
> >+    // extra HTML:body make the old body accessible) then we have to recache
> >+    // children of the container, and then fire show/hide events for a change.
> >+    UpdateTreeOnInsertion(container);
> >+    break;
> 
> that's rather strange loop.

agree, otherwise extra check after loop which is also not nice

> >     var gQueue = null;
> >-    //gA11yEventDumpToConsole = true; // debug stuff
> >+    gA11yEventDumpToConsole = true; // debug stuff
> 
> remember to disable it
> 
> >-      testAccessibleTree("listbox", accTree);
> >+  //    testAccessibleTree("listbox", accTree);
> 
> why do you disable it?

that was for testing, not part of landing

> >                 ]
> >-},
> >+              },
> 
> extranious whitespace changes are really annoying.

just some fixes as we go, dealing with them separately also not nice

	Trevor Saunders (:tbsaunde) Reporter
	Comment 11 • 10 years ago

> > >-      testAccessibleTree("listbox", accTree);
> > >+  //    testAccessibleTree("listbox", accTree);
> > 
> > why do you disable it?
> 
> that was for testing, not part of landing

if you revert it before committing fine.

> > >                 ]
> > >-},
> > >+              },
> > 
> > extranious whitespace changes are really annoying.
> 
> just some fixes as we go, dealing with them separately also not nice

what's wrong with doing them by themselves?

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

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

> > > extranious whitespace changes are really annoying.
> > 
> > just some fixes as we go, dealing with them separately also not nice
> 
> what's wrong with doing them by themselves?

that's ok but it's extra work and it may be avoided

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

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