Closed Bug 1255009 Opened 8 years ago Closed 8 years ago

insert children into the tree on content insertion instead the recaching

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(6 files)

Attached patch patchSplinter Review
      No description provided.
Attachment #8728430 - Flags: review?(yzenevich)
Comment on attachment 8728430 [details] [diff] [review]
patch

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

Nice

::: accessible/tests/mochitest/tree/test_txtctrl.xul
@@ +162,5 @@
>        } else {
>          SimpleTest.ok(true, "Testing (New) Toolkit autocomplete widget.");
>  
>          // Dumb access to trigger popup lazy creation.
> +        dump("Trigget popup lazy creation");

should this be commented out?
Attachment #8728430 - Flags: review?(yzenevich) → review+
this is actually a good landmark for the test's execution phase
https://hg.mozilla.org/integration/mozilla-inbound/rev/4916251eb38968263c69c633fee264d129570768
Bug 1255009 - insert children into the tree on content insertion instead the recaching, r=yzen
https://hg.mozilla.org/mozilla-central/rev/4916251eb389
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Bisection points to this causing a 100% crash in AWSY [1] during testing. I think for now we should back it out and I'll try to provide a reduced test case.

[1] https://areweslimyet.com
no crash for me on os x, any specific steps to reproduce?

also curious, do you know what makes accessibility running on your machine?
(In reply to alexander :surkov from comment #6)
> no crash for me on os x, any specific steps to reproduce?

I guess you didn't meant the web site itself, but anyway, str or crash stack will be useful
Current STR is to run AWSY, note that it crashes (this is clearly not a great STR). AWSY runs against linux64 opt. Unfortunately currently I don't have a stack due to limitations in our test harness, I should be able to reduce tomorrow.

If it wasn't clear, this has caused every AWSY run to crash since landing on 3-11.
With e10s enabled AWSY crashes pretty quickly, always on the same page (tp5/163.com/www.163.com/index.html) out of the tp5 pageset [1]:

> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ff613e72419 in MaiAtkObject::Shutdown() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> (gdb) bt
> #0  0x00007ff613e72419 in MaiAtkObject::Shutdown() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #1  0x00007ff613e73ed7 in mozilla::a11y::ProxyDestroyed(mozilla::a11y::ProxyAccessible*) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #2  0x00007ff613eadf03 in mozilla::a11y::DocAccessibleParent::Destroy() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #3  0x00007ff613eadf6e in mozilla::a11y::DocAccessibleParent::RecvShutdown() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #4  0x00007ff612b7554e in mozilla::a11y::PDocAccessibleParent::OnMessageReceived(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #5  0x00007ff612b56711 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #6  0x00007ff61299f10b in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #7  0x00007ff6129a639e in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #8  0x00007ff6129a80d6 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #9  0x00007ff612986163 in MessageLoop::RunTask(Task*) ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #10 0x00007ff61298aabf in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #11 0x00007ff61298abe6 in MessageLoop::DoWork() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #12 0x00007ff61299b5c8 in mozilla::ipc::DoWorkRunnable::Run() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #13 0x00007ff612781523 in nsThread::ProcessNextEvent(bool, bool*) ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #14 0x00007ff61279bed9 in NS_ProcessNextEvent(nsIThread*, bool) ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #15 0x00007ff61299e7ce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #16 0x00007ff6129861ce in MessageLoop::Run() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #17 0x00007ff6139ed715 in nsBaseAppShell::Run() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #18 0x00007ff613fd5dba in nsAppStartup::Run() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #19 0x00007ff614009ee9 in XREMain::XRE_mainRun() ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #20 0x00007ff61400c2ed in XREMain::XRE_main(int, char**, nsXREAppData const*)
>     () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #21 0x00007ff61400c539 in XRE_main ()
>    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> #22 0x0000000000405e29 in do_main(int, char**, char**, nsIFile*) ()
> #23 0x000000000040548e in main ()

Presumably the issue is |DocAccessibleParent::Destroy| calling |ProxyDestroyed(nullptr)| [2].

[1] http://people.mozilla.org/~jmaher/taloszips/zips/tp5n.zip
[2] https://dxr.mozilla.org/mozilla-central/rev/d6ee82b9a74155b6bfd544166f036fc572ae8c56/accessible/ipc/DocAccessibleParent.cpp#328
cc'ing tbsaunde

(In reply to Eric Rahm [:erahm] from comment #9)
> Presumably the issue is |DocAccessibleParent::Destroy| calling
> |ProxyDestroyed(nullptr)| [2].

would you be able to check this idea please? as far as I see, mAccessibles hash mutates only here [1], and proxy shoudn't be a null there

[1] https://dxr.mozilla.org/mozilla-central/rev/d6ee82b9a74155b6bfd544166f036fc572ae8c56/accessible/ipc/DocAccessibleParent.cpp#87
Backed out to un-break AWSY.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c54c291870e
Assignee: nobody → surkov.alexander
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla48 → ---
Trev can you take a look?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Eric Rahm [:erahm] from comment #9)
> With e10s enabled AWSY crashes pretty quickly, always on the same page
> (tp5/163.com/www.163.com/index.html) out of the tp5 pageset [1]:

am I supposed to just load the page or some extra steps are required?

> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00007ff613e72419 in MaiAtkObject::Shutdown() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so

is there a complete log available?
(In reply to Eric Rahm [:erahm] from comment #9)
> With e10s enabled AWSY crashes pretty quickly, always on the same page
> (tp5/163.com/www.163.com/index.html) out of the tp5 pageset [1]:
> 
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00007ff613e72419 in MaiAtkObject::Shutdown() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > (gdb) bt
> > #0  0x00007ff613e72419 in MaiAtkObject::Shutdown() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #1  0x00007ff613e73ed7 in mozilla::a11y::ProxyDestroyed(mozilla::a11y::ProxyAccessible*) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #2  0x00007ff613eadf03 in mozilla::a11y::DocAccessibleParent::Destroy() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #3  0x00007ff613eadf6e in mozilla::a11y::DocAccessibleParent::RecvShutdown() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #4  0x00007ff612b7554e in mozilla::a11y::PDocAccessibleParent::OnMessageReceived(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #5  0x00007ff612b56711 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #6  0x00007ff61299f10b in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #7  0x00007ff6129a639e in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #8  0x00007ff6129a80d6 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #9  0x00007ff612986163 in MessageLoop::RunTask(Task*) ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #10 0x00007ff61298aabf in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #11 0x00007ff61298abe6 in MessageLoop::DoWork() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #12 0x00007ff61299b5c8 in mozilla::ipc::DoWorkRunnable::Run() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #13 0x00007ff612781523 in nsThread::ProcessNextEvent(bool, bool*) ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #14 0x00007ff61279bed9 in NS_ProcessNextEvent(nsIThread*, bool) ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #15 0x00007ff61299e7ce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #16 0x00007ff6129861ce in MessageLoop::Run() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #17 0x00007ff6139ed715 in nsBaseAppShell::Run() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #18 0x00007ff613fd5dba in nsAppStartup::Run() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #19 0x00007ff614009ee9 in XREMain::XRE_mainRun() ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #20 0x00007ff61400c2ed in XREMain::XRE_main(int, char**, nsXREAppData const*)
> >     () from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #21 0x00007ff61400c539 in XRE_main ()
> >    from /home/erahm/dev/areweslimyet/firefox/libxul.so
> > #22 0x0000000000405e29 in do_main(int, char**, char**, nsIFile*) ()
> > #23 0x000000000040548e in main ()
> 
> Presumably the issue is |DocAccessibleParent::Destroy| calling
> |ProxyDestroyed(nullptr)| [2].

presumalby something like that, and yes ProxyDestroyed() really shouldn't be called with null, and the hash table shouldn't contain null.  So it seems odd, I guess somebody needs to just debug it.

(In reply to David Bolter [:davidb] from comment #12)
> Trev can you take a look?

I don't know what
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #14)

> presumalby something like that, and yes ProxyDestroyed() really shouldn't be
> called with null, and the hash table shouldn't contain null.  So it seems
> odd, I guess somebody needs to just debug it.
> 
> (In reply to David Bolter [:davidb] from comment #12)
> > Trev can you take a look?
> 
> I don't know what

you'd need to install AWSY on your machine for that, I think Eric can share instructions how to do that
Attached file gecko.log.bz2
This is the log from a debug run. Just filtering for 'accessible' we see the following:

> [Parent 4161] WARNING: NS_ENSURE_TRUE(parentObject) failed: file /home/erahm/dev/mozilla-central/accessible/atk/AccessibleWrap.cpp, line 1586
> [Parent 4161] WARNING: NS_ENSURE_TRUE(parentObject) failed: file /home/erahm/dev/mozilla-central/accessible/atk/AccessibleWrap.cpp, line 1586
> [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44

Well, this suggests there is at least two problems.  First the child probably isn't firing Accessible show / hide events correctly.  Second whatever is wrong with the first problem it shouldn't lead to a parent process crash.
I was able to reproduce in atsy [1] (a simpler form of AWSY). I can provide help in getting that setup (there's a bootstrap script for Ubuntu provided at [1]) if you'd like to use it to repro.

I was also able to reproduce similar behavior on OSX although it seemed that the content process was getting SIGTERM'd rather than the parent process crashing.

At this point I don't think there's much else I can do on my end.

[1] https://github.com/EricRahm/atsy
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> 
> Well, this suggests there is at least two problems.  First the child
> probably isn't firing Accessible show / hide events correctly.

It seems there's nothing wrong with show/hide events, because of the nature of insertion, show events might be fired unordered, for example, show event is fired for 2nd child, and then for 1st child.
I have confirmed that AWSY has fully processed each build since the backout landed.
This, or bug 1255614, causes a regression when an li within an ul is being changed/updated. This can be observed in IRCCloud:

1. Log into https://irccloud.mozilla.com.
2. Ask someone to send you a private message.

Result: Near the top of the page, a list item is being inserted or updated that points to the person who sent you that message.

Before these two bugs landed (they landed together), the updated item would contain a number (the badge), plus the link to the person. Now, after these two bugs landed, the list item gets updated, but NVDA sees it as empty. Not even a page refresh (NVDA+F5) can make the link and the number return to the list item.

So whatever either of these bugs, or the two of them in concert, do, breaks the tree update when the badge gets inserted.
Attached patch ipc fixSplinter Review
Attachment #8732225 - Flags: review?(dbolter)
From a high level, I'm not sure I understand why mChildren should have gaps? What is happening?

Is it because event ordering can be jumbled?
Flags: needinfo?(surkov.alexander)
(In reply to David Bolter [:davidb] from comment #24)
> From a high level, I'm not sure I understand why mChildren should have gaps?
> What is happening?
> 
> Is it because event ordering can be jumbled?

here's a scenario, a child is inserted, a show event is queued, a child before the first child is inserted, its show event is queued, then event queue is processed and all events are fired to ipc layer in the order they were queued, so the first show event's target has child index 1, 2nd one has index 0.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #25)
> (In reply to David Bolter [:davidb] from comment #24)
> > From a high level, I'm not sure I understand why mChildren should have gaps?
> > What is happening?
> > 
> > Is it because event ordering can be jumbled?
> 
> here's a scenario, a child is inserted, a show event is queued, a child
> before the first child is inserted, its show event is queued, then event
> queue is processed and all events are fired to ipc layer in the order they
> were queued, so the first show event's target has child index 1, 2nd one has
> index 0.

OK thanks.
(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > > > [Parent 4161] ###!!! ASSERTION: invalid index to add child at: 'Error', file /home/erahm/dev/mozilla-central/accessible/ipc/DocAccessibleParent.cpp, line 44
> > 
> > Well, this suggests there is at least two problems.  First the child
> > probably isn't firing Accessible show / hide events correctly.
> 
> It seems there's nothing wrong with show/hide events, because of the nature
> of insertion, show events might be fired unordered, for example, show event
> is fired for 2nd child, and then for 1st child.

I would define that as broken.  inserting a child into an array at index N doesn't make any sense when the current length of the array is < n-1.

(In reply to David Bolter [:davidb] from comment #24)
> From a high level, I'm not sure I understand why mChildren should have gaps?
> What is happening?
> 
> Is it because event ordering can be jumbled?

I think more or less yes.  That said I'm not convinced these are ok changes to make, and if they are they certainly need to come with more justification, and fewer random unrelated changes.
(In reply to Trevor Saunders (:tbsaunde) from comment #27)

> > It seems there's nothing wrong with show/hide events, because of the nature
> > of insertion, show events might be fired unordered, for example, show event
> > is fired for 2nd child, and then for 1st child.
> 
> I would define that as broken.  inserting a child into an array at index N
> doesn't make any sense when the current length of the array is < n-1.

If you end up with gaps then you have a broken tree, if you don't then you are fine. You can replace this logic on something more complicated, but not sure I see a reason for that. If you've got out of sync, then you have a problem.

> (In reply to David Bolter [:davidb] from comment #24)
> > From a high level, I'm not sure I understand why mChildren should have gaps?
> > What is happening?
> > 
> > Is it because event ordering can be jumbled?
> 
> I think more or less yes.  That said I'm not convinced these are ok changes
> to make, and if they are they certainly need to come with more
> justification,

you have a scenario how it happens, what else do you need?

> and fewer random unrelated changes.

I can certainly revert those MOZ_ASSERT stuff, if they look bad.
> > It seems there's nothing wrong with show/hide events, because of the nature
> > of insertion, show events might be fired unordered, for example, show event
> > is fired for 2nd child, and then for 1st child.
> 
> I would define that as broken.

Problematic yes, unbroken I'm less sure. If you take the recent example Alex gave me... if I understand correctly, technically the show events are in the right order, they just happen to not be in child/sibling order because the children were added out of order (why?)
> (why?)

I guess because web developer.
(In reply to David Bolter [:davidb] from comment #30)
> > (why?)
> 
> I guess because web developer.

exactly, if you do something like
parent.appendChild(newNode1);
parent.insertBefore(newNode2, parent.firstChild)

then you run into that scenario (if layout is not smart enough to change their ordering).
(In reply to David Bolter [:davidb] from comment #29)
> > > It seems there's nothing wrong with show/hide events, because of the nature
> > > of insertion, show events might be fired unordered, for example, show event
> > > is fired for 2nd child, and then for 1st child.
> > 
> > I would define that as broken.
> 
> Problematic yes, unbroken I'm less sure. If you take the recent example Alex
> gave me... if I understand correctly, technically the show events are in the
> right order, they just happen to not be in child/sibling order because the
> children were added out of order (why?)

I think I'd say when you add to the end of the array sibling order is the only correct one.  In addition doing something other than that changes behavior other people may depend on.

> > (In reply to David Bolter [:davidb] from comment #24)
> > > From a high level, I'm not sure I understand why mChildren should have gaps?
> > > What is happening?
> > > 
> > > Is it because event ordering can be jumbled?
> > 
> > I think more or less yes.  That said I'm not convinced these are ok changes
> > to make, and if they are they certainly need to come with more
> > justification,
> 
> you have a scenario how it happens, what else do you need?

a convincing argument that its the best solution to the problem.  YOu also need to justify changing the behavior of event order.  Also show that you aren't introducing any bugs, and ideally aren't making any harder to deal with.

> > and fewer random unrelated changes.
> 
> I can certainly revert those MOZ_ASSERT stuff, if they look bad.

I don't know that I care one way or another about that and the renaming, but it certainly makes the patch unnecessarily hard to read.
(In reply to Trevor Saunders (:tbsaunde) from comment #32)

> I think I'd say when you add to the end of the array sibling order is the
> only correct one.

I can add them into the end, and insert, that'd be slower, but also an option of course

>  In addition doing something other than that changes
> behavior other people may depend on.

after event queue is flushed then we shouldn't have any gaps, not sure who may depend on gaps between show events handling.
(In reply to alexander :surkov from comment #33)
> (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> 
> > I think I'd say when you add to the end of the array sibling order is the
> > only correct one.
> 
> I can add them into the end, and insert, that'd be slower, but also an
> option of course

Its not clear to me what you are suggesting.

> >  In addition doing something other than that changes
> > behavior other people may depend on.
> 
> after event queue is flushed then we shouldn't have any gaps, not sure who
> may depend on gaps between show events handling.

well, that's not the only thing you are changing.  You are changing the order of events.
(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> (In reply to alexander :surkov from comment #33)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > 
> > > I think I'd say when you add to the end of the array sibling order is the
> > > only correct one.
> > 
> > I can add them into the end, and insert, that'd be slower, but also an
> > option of course
> 
> Its not clear to me what you are suggesting.

I can do either way
1) resize array and set a value (my approach)
2) append into the end if index is bigger (your suggestion)

I prefer 1) since 2nd is slower as we need to make insertion more often

> > >  In addition doing something other than that changes
> > > behavior other people may depend on.
> > 
> > after event queue is flushed then we shouldn't have any gaps, not sure who
> > may depend on gaps between show events handling.
> 
> well, that's not the only thing you are changing.  You are changing the
> order of events.

difference in event ordering is a consequence of the insertion approach (instead of the recaching we used to have). In general no one should assume that show events are ordered.
(In reply to alexander :surkov from comment #35)
> (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > (In reply to alexander :surkov from comment #33)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
 
> > > >  In addition doing something other than that changes
> > > > behavior other people may depend on.
> > > 
> > > after event queue is flushed then we shouldn't have any gaps, not sure who
> > > may depend on gaps between show events handling.
> > 
> > well, that's not the only thing you are changing.  You are changing the
> > order of events.
> 
> difference in event ordering is a consequence of the insertion approach
> (instead of the recaching we used to have). In general no one should assume
> that show events are ordered.

As I understand the event order change correctly we would be gaining more accuracy in terms of having show events in the same time-ordering the children are added, is that correct? If so, I'd be okay with seeing if that broke anyone.

Separate to this, I don't really know what to think about the gaps issue. It feels possibly fragile but maybe not.
(In reply to David Bolter [:davidb] from comment #36)

> As I understand the event order change correctly we would be gaining more
> accuracy in terms of having show events in the same time-ordering the
> children are added, is that correct?

yes, event ordering is time based, whichever came first, was fired first.

> If so, I'd be okay with seeing if that
> broke anyone.

The patch was several days on trunk, Marco found one regressions, which wasn't event related, so far. But again, I hardly can imagine that somebody might expect show events ordered by index in parent (the ipc layer is an exception, but that's rather a design issue).

> Separate to this, I don't really know what to think about the gaps issue. It
> feels possibly fragile but maybe not.

I agree it may feel fragile, because you may end with null children, which is totally weird, but if you do, you have a tree problem that you have to fix. So if you end up with null children, then it's more likely you will catch the problem faster, because you don't masking it. So the approach may be seen as a benefit actually. And yes it's faster, which is good too. Having said that I'm good to go with any approach that you think should be taken.
(In reply to alexander :surkov from comment #37)
> (In reply to David Bolter [:davidb] from comment #36)

> > If so, I'd be okay with seeing if that
> > broke anyone.
> 
> The patch was several days on trunk, Marco found one regressions, which
> wasn't event related, so far. But again, I hardly can imagine that somebody
> might expect show events ordered by index in parent (the ipc layer is an
> exception, but that's rather a design issue).

Do we reply on the original behaviour in IPC code? Trevor?
huh, thought I replied to this first part before.

(In reply to alexander :surkov from comment #35)
> (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > (In reply to alexander :surkov from comment #33)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > > 
> > > > I think I'd say when you add to the end of the array sibling order is the
> > > > only correct one.
> > > 
> > > I can add them into the end, and insert, that'd be slower, but also an
> > > option of course
> > 
> > Its not clear to me what you are suggesting.
> 
> I can do either way
> 1) resize array and set a value (my approach)
> 2) append into the end if index is bigger (your suggestion)

I was actually suggesting, the event firing should be changed to fire events in children order, or fire them with insertion indexs that are correct when they are inserted.

That said I guess 2 is ok with a
// XXX we sometimes get show events for indexs that are past the end of our current children, which we work  around here by adjusting indexs greater than ChildCount() to be ChildCount().

comment.

> I prefer 1) since 2nd is slower as we need to make insertion more often

I seriously doubt that's at all noticeable, and 2 means more predictable behavior and no reuse of ids.

> > > >  In addition doing something other than that changes
> > > > behavior other people may depend on.
> > > 
> > > after event queue is flushed then we shouldn't have any gaps, not sure who
> > > may depend on gaps between show events handling.
> > 
> > well, that's not the only thing you are changing.  You are changing the
> > order of events.
> 
> difference in event ordering is a consequence of the insertion approach
> (instead of the recaching we used to have). In general no one should assume
> that show events are ordered.

uh, why not? that seems like something fairly reasonable to rely on and anyway people might be doing it.

(In reply to David Bolter [:davidb] from comment #36)
> (In reply to alexander :surkov from comment #35)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > > (In reply to alexander :surkov from comment #33)
> > > > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
>  
> > > > >  In addition doing something other than that changes
> > > > > behavior other people may depend on.
> > > > 
> > > > after event queue is flushed then we shouldn't have any gaps, not sure who
> > > > may depend on gaps between show events handling.
> > > 
> > > well, that's not the only thing you are changing.  You are changing the
> > > order of events.
> > 
> > difference in event ordering is a consequence of the insertion approach
> > (instead of the recaching we used to have). In general no one should assume
> > that show events are ordered.
> 
> As I understand the event order change correctly we would be gaining more
> accuracy in terms of having show events in the same time-ordering the
> children are added, is that correct? If so, I'd be okay with seeing if that
> broke anyone.

Well, arguably its more correct in the sense of time, but it means that insertion indexs are wrong.  That is you can get an event saying child N was just added when you know about M children and m + 1 < N, which doesn't really make sense.

> Separate to this, I don't really know what to think about the gaps issue. It
> feels possibly fragile but maybe not.

That doesn't bother me nearly as much as trying to reuse IDs that already have proxy accessibles.

(In reply to alexander :surkov from comment #37)
> (In reply to David Bolter [:davidb] from comment #36)
> 
> > As I understand the event order change correctly we would be gaining more
> > accuracy in terms of having show events in the same time-ordering the
> > children are added, is that correct?
> 
> yes, event ordering is time based, whichever came first, was fired first.
> 
> > If so, I'd be okay with seeing if that
> > broke anyone.
> 
> The patch was several days on trunk, Marco found one regressions, which
> wasn't event related, so far. But again, I hardly can imagine that somebody
> might expect show events ordered by index in parent (the ipc layer is an
> exception, but that's rather a design issue).

I think what the ipc code is doing here is a pretty reasonable way to maintain a copy of the tree, and I wouldn't at all suprised if anyone else trying to do the same thing does it in the same way.

> > Separate to this, I don't really know what to think about the gaps issue. It
> > feels possibly fragile but maybe not.
> 
> I agree it may feel fragile, because you may end with null children, which
> is totally weird, but if you do, you have a tree problem that you have to
> fix. So if you end up with null children, then it's more likely you will
> catch the problem faster, because you don't masking it. So the approach may
> be seen as a benefit actually. And yes it's faster, which is good too.

Well, we should be trying to abort when we get bad tree updates that don't make sense, not masking them in empty children for a while, and hoping somebody tries to access those missing children to find out its broken.

(In reply to David Bolter [:davidb] from comment #38)
> (In reply to alexander :surkov from comment #37)
> > (In reply to David Bolter [:davidb] from comment #36)
> 
> > > If so, I'd be okay with seeing if that
> > > broke anyone.
> > 
> > The patch was several days on trunk, Marco found one regressions, which
> > wasn't event related, so far. But again, I hardly can imagine that somebody
> > might expect show events ordered by index in parent (the ipc layer is an
> > exception, but that's rather a design issue).
> 
> Do we reply on the original behaviour in IPC code? Trevor?

you mean properties of the previous order events were fired in? yes.
(In reply to Trevor Saunders (:tbsaunde) from comment #39)

> > I can do either way
> > 1) resize array and set a value (my approach)
> > 2) append into the end if index is bigger (your suggestion)
> 
> I was actually suggesting, the event firing should be changed to fire events
> in children order, or fire them with insertion indexs that are correct when
> they are inserted.

it's extra work in a place known being a perf bottleneck, and it seems have no practical reason

> > I prefer 1) since 2nd is slower as we need to make insertion more often
> 
> I seriously doubt that's at all noticeable, and 2 means more predictable
> behavior and no reuse of ids.

I wouldn't be so confident, because insertion is o(n), appending and assignment is o(1). If you insert n children in reverse order then you end up with o(n^2), so I think you will find a test case sooner or later, where this code is a bottleneck.

> > difference in event ordering is a consequence of the insertion approach
> > (instead of the recaching we used to have). In general no one should assume
> > that show events are ordered.
> 
> uh, why not? that seems like something fairly reasonable to rely on and
> anyway people might be doing it.

simply because the developers can't rely on it, because of async nature of event handling. In other words, they can't be sure, that they get a first event before our WillRefresh ticks a second time, and we fire one more portion of events.

> (In reply to David Bolter [:davidb] from comment #36)
> > > difference in event ordering is a consequence of the insertion approach
> > > (instead of the recaching we used to have). In general no one should assume
> > > that show events are ordered.
> > 
> > As I understand the event order change correctly we would be gaining more
> > accuracy in terms of having show events in the same time-ordering the
> > children are added, is that correct? If so, I'd be okay with seeing if that
> > broke anyone.
> 
> Well, arguably its more correct in the sense of time, but it means that
> insertion indexs are wrong. 

not sure what insertion index definition is, but if you meant child indexes, then the statement cannot be true, because mChildren.ElementAt(index).IndexInParent() == index.

> > The patch was several days on trunk, Marco found one regressions, which
> > wasn't event related, so far. But again, I hardly can imagine that somebody
> > might expect show events ordered by index in parent (the ipc layer is an
> > exception, but that's rather a design issue).
> 
> I think what the ipc code is doing here is a pretty reasonable way to
> maintain a copy of the tree, and I wouldn't at all suprised if anyone else
> trying to do the same thing does it in the same way.

you may rely on our internals, others probably won't.
(In reply to alexander :surkov from comment #40)

> > uh, why not? that seems like something fairly reasonable to rely on and
> > anyway people might be doing it.
> 
> simply because the developers can't rely on it, because of async nature of
> event handling. In other words, they can't be sure, that they get a first
> event before our WillRefresh ticks a second time, and we fire one more
> portion of events.

actually I'm not even sure if you are allowed to rely on this assumption in the ipc layer.
In talking with Trevor this morning (Trevor please correct me if I don't get this right), I think Trevor's main concern is how we deal with the case where mAccessibles already has an object with the new child's ID. I would have naively thought this was probably okay (probably same object) but I think there is a worry that GetID returns an object erroneously, that we should have removed from mAccessibles (maybe we missed a hide event, or one wasn't fired when it should have been), and rather than bailing we now deal with that object, with unknown results.
(In reply to David Bolter [:davidb] from comment #42)
> think there is a worry that GetID returns an object erroneously, that we

Note: GetID here is short for "mAccessibles.GetEntry(childData.ID())"
(In reply to David Bolter [:davidb] from comment #42)
> In talking with Trevor this morning (Trevor please correct me if I don't get
> this right), I think Trevor's main concern is how we deal with the case
> where mAccessibles already has an object with the new child's ID. I would
> have naively thought this was probably okay (probably same object)

yes, this is a plausible scenario, you may get a show event, and then hide event. Say, you insert a tree, that has aria-owns, then you'll get a show event for the subtree root, and thide event for accessible that was moved

> but I
> think there is a worry that GetID returns an object erroneously, that we
> should have removed from mAccessibles (maybe we missed a hide event, or one
> wasn't fired when it should have been), and rather than bailing we now deal
> with that object, with unknown results.
occasionally pressed the enter

> yes, this is a plausible scenario, you may get a show event, and then hide
> event. Say, you insert a tree, that has aria-owns, then you'll get a show
> event for the subtree root, and thide event for accessible that was moved
> 
> > but I
> > think there is a worry that GetID returns an object erroneously, 

if that's a point then it's rather a serious issue, I'm not sure how you can build ipc trees at all, if you cannot trust a result of GetID().
(In reply to alexander :surkov from comment #44)

> yes, this is a plausible scenario, you may get a show event, and then hide
> event. Say, you insert a tree, that has aria-owns, then you'll get a show
> event for the subtree root, and thide event for accessible that was moved

something like this should make a trick:

<div id="d1"></div>
<div id="d2" hidden>
  <div aria-owns="d1"></div>
</div>

then you do document.getElementById("d2").hidden = false;
ugh, bugzilla is the worst and ate my comment the first time.

(In reply to alexander :surkov from comment #40)
> (In reply to Trevor Saunders (:tbsaunde) from comment #39)
> 
> > > I can do either way
> > > 1) resize array and set a value (my approach)
> > > 2) append into the end if index is bigger (your suggestion)
> > 
> > I was actually suggesting, the event firing should be changed to fire events
> > in children order, or fire them with insertion indexs that are correct when
> > they are inserted.
> 
> it's extra work in a place known being a perf bottleneck, and it seems have
> no practical reason

correctness is pretty important.

> > > I prefer 1) since 2nd is slower as we need to make insertion more often
> > 
> > I seriously doubt that's at all noticeable, and 2 means more predictable
> > behavior and no reuse of ids.
> 
> I wouldn't be so confident, because insertion is o(n), appending and
> assignment is o(1). If you insert n children in reverse order then you end
> up with o(n^2), so I think you will find a test case sooner or later, where
> this code is a bottleneck.

maaayyybee, on the other hand memcpy is fast, and lots of children is pretty rare and hurts layout too, especially if you decide to insert all of them backwards.

> > > difference in event ordering is a consequence of the insertion approach
> > > (instead of the recaching we used to have). In general no one should assume
> > > that show events are ordered.
> > 
> > uh, why not? that seems like something fairly reasonable to rely on and
> > anyway people might be doing it.
> 
> simply because the developers can't rely on it, because of async nature of
> event handling. In other words, they can't be sure, that they get a first
> event before our WillRefresh ticks a second time, and we fire one more
> portion of events.

uh they can and it should work so long as we don't run js in Willrefresh() and I'm pretty sure the only way for that to happen is addons, which really shouldn't be flushing layout when notified of a11y events.

> > (In reply to David Bolter [:davidb] from comment #36)
> > > > difference in event ordering is a consequence of the insertion approach
> > > > (instead of the recaching we used to have). In general no one should assume
> > > > that show events are ordered.
> > > 
> > > As I understand the event order change correctly we would be gaining more
> > > accuracy in terms of having show events in the same time-ordering the
> > > children are added, is that correct? If so, I'd be okay with seeing if that
> > > broke anyone.
> > 
> > Well, arguably its more correct in the sense of time, but it means that
> > insertion indexs are wrong. 
> 
> not sure what insertion index definition is, but if you meant child indexes,
> then the statement cannot be true, because
> mChildren.ElementAt(index).IndexInParent() == index.

no, the index they are at when inserted, which clearly is in the range [0, current number of children].

> > > The patch was several days on trunk, Marco found one regressions, which
> > > wasn't event related, so far. But again, I hardly can imagine that somebody
> > > might expect show events ordered by index in parent (the ipc layer is an
> > > exception, but that's rather a design issue).
> > 
> > I think what the ipc code is doing here is a pretty reasonable way to
> > maintain a copy of the tree, and I wouldn't at all suprised if anyone else
> > trying to do the same thing does it in the same way.
> 
> you may rely on our internals, others probably won't.

I'd say its only relying on show / hide events behaving the way they should.

(In reply to alexander :surkov from comment #44)
> (In reply to David Bolter [:davidb] from comment #42)
> > In talking with Trevor this morning (Trevor please correct me if I don't get
> > this right), I think Trevor's main concern is how we deal with the case
> > where mAccessibles already has an object with the new child's ID. I would
> > have naively thought this was probably okay (probably same object)
> 
> yes, this is a plausible scenario, you may get a show event, and then hide
> event. Say, you insert a tree, that has aria-owns, then you'll get a show
> event for the subtree root, and thide event for accessible that was moved

then there is another way the tree update event firing code is broken.  How does it make any sense to be saying one tree is in two places at once.

(In reply to alexander :surkov from comment #45)
> occasionally pressed the enter
> 
> > yes, this is a plausible scenario, you may get a show event, and then hide
> > event. Say, you insert a tree, that has aria-owns, then you'll get a show
> > event for the subtree root, and thide event for accessible that was moved
> > 
> > > but I
> > > think there is a worry that GetID returns an object erroneously, 
> 
> if that's a point then it's rather a serious issue, I'm not sure how you can
> build ipc trees at all, if you cannot trust a result of GetID().

I wouldn't say you can't trust it.  I'm not convinced mutating the tree in any other way than applying the simple mutation implied by the event is safe.  I remember deciding reusing IDs like this was a bad idea a year ago, but I don't remember the details, and I'm not really interested in figuring it out again.
(In reply to Trevor Saunders (:tbsaunde) from comment #47)

> > > > difference in event ordering is a consequence of the insertion approach
> > > > (instead of the recaching we used to have). In general no one should assume
> > > > that show events are ordered.
> > > 
> > > uh, why not? that seems like something fairly reasonable to rely on and
> > > anyway people might be doing it.
> > 
> > simply because the developers can't rely on it, because of async nature of
> > event handling. In other words, they can't be sure, that they get a first
> > event before our WillRefresh ticks a second time, and we fire one more
> > portion of events.
> 
> uh they can and it should work

this is interesting, what does keep WillRefresh on hold, while, for example, MS COM makes its async event delivering?

> so long as we don't run js in Willrefresh()
> and I'm pretty sure the only way for that to happen is addons, which really
> shouldn't be flushing layout when notified of a11y events.

nothing prevents them from doing this

> > yes, this is a plausible scenario, you may get a show event, and then hide
> > event. Say, you insert a tree, that has aria-owns, then you'll get a show
> > event for the subtree root, and thide event for accessible that was moved
> 
> then there is another way the tree update event firing code is broken.

it's false statement, events in this case reflect exactly what we did

>  How
> does it make any sense to be saying one tree is in two places at once.

there's no trees two places
(In reply to alexander :surkov from comment #48)
> (In reply to Trevor Saunders (:tbsaunde) from comment #47)

> > > yes, this is a plausible scenario, you may get a show event, and then hide
> > > event. Say, you insert a tree, that has aria-owns, then you'll get a show
> > > event for the subtree root, and thide event for accessible that was moved
> > 
> > then there is another way the tree update event firing code is broken.
> 
> it's false statement, events in this case reflect exactly what we did

note, if you do a similar thing in DOM like

var d1 = document.getElementById("d1");
var d2 = document.createElement("div");
d1.parentNode.insertBefore(d2, d1.nextElementSibling);
d2.appendChild(d1);

and will be listening the changes via MutationObserver, then you will get exactly same results: append for 'd2', removal for 'd1', append for 'd1'. accessibility events are same, except last show for 'd1' which is coalesced by parent's 'd2' show.

anyway the accessible may be moved under a showing tree and ipc layer should handle this case properly. My patch contains a fix for that, I'm not sure if it can be done a way better. If somebody wants to investigate it then sure get cards in your hands. I just need a quick fix for AWSY problem.
(In reply to alexander :surkov from comment #48)
> (In reply to Trevor Saunders (:tbsaunde) from comment #47)
> 
> > > > > difference in event ordering is a consequence of the insertion approach
> > > > > (instead of the recaching we used to have). In general no one should assume
> > > > > that show events are ordered.
> > > > 
> > > > uh, why not? that seems like something fairly reasonable to rely on and
> > > > anyway people might be doing it.
> > > 
> > > simply because the developers can't rely on it, because of async nature of
> > > event handling. In other words, they can't be sure, that they get a first
> > > event before our WillRefresh ticks a second time, and we fire one more
> > > portion of events.
> > 
> > uh they can and it should work
> 
> this is interesting, what does keep WillRefresh on hold, while, for example,
> MS COM makes its async event delivering?

delivering the events async doesn't change the order, and you shouldn't forget  inprocess clients that see events sync, as well as linux.

> > so long as we don't run js in Willrefresh()
> > and I'm pretty sure the only way for that to happen is addons, which really
> > shouldn't be flushing layout when notified of a11y events.
> 
> nothing prevents them from doing this

nothing prevents them from crashing the browser either.  I think I'd say if they do it that's not supported.

> > > yes, this is a plausible scenario, you may get a show event, and then hide
> > > event. Say, you insert a tree, that has aria-owns, then you'll get a show
> > > event for the subtree root, and thide event for accessible that was moved
> > 
> > then there is another way the tree update event firing code is broken.
> 
> it's false statement, events in this case reflect exactly what we did

no, firing the hide after the show *is* bogus.  Unless of course you want to say  that between the events the tree is in two places.

> >  How
> > does it make any sense to be saying one tree is in two places at once.
> 
> there's no trees two places

according to the events you pretend are valid there is.

(In reply to alexander :surkov from comment #49)
> (In reply to alexander :surkov from comment #48)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #47)
> 
> > > > yes, this is a plausible scenario, you may get a show event, and then hide
> > > > event. Say, you insert a tree, that has aria-owns, then you'll get a show
> > > > event for the subtree root, and thide event for accessible that was moved
> > > 
> > > then there is another way the tree update event firing code is broken.
> > 
> > it's false statement, events in this case reflect exactly what we did
> 
> note, if you do a similar thing in DOM like
> 
> var d1 = document.getElementById("d1");
> var d2 = document.createElement("div");
> d1.parentNode.insertBefore(d2, d1.nextElementSibling);
> d2.appendChild(d1);
> 
> and will be listening the changes via MutationObserver, then you will get
> exactly same results: append for 'd2', removal for 'd1', append for 'd1'.

frankly I don't realy care what happens with mutation observers.

> accessibility events are same, except last show for 'd1' which is coalesced
> by parent's 'd2' show.

that's bogus.

> anyway the accessible may be moved under a showing tree and ipc layer should
> handle this case properly. My patch contains a fix for that, I'm not sure if

no, hide events should be fired before things show up other places.

> it can be done a way better. If somebody wants to investigate it then sure
> get cards in your hands. I just need a quick fix for AWSY problem.

I have no idea what you mean with cards.
This patch is unacceptable because of it, and I don't really careif you can fix what's broken about it quickly or not.
Attached patch ipc fix lightSplinter Review
if you are not sure about existing ID Shutdown() part, then I have a lighter version for you, which seems to be not crashing too, what works with me.
Attachment #8733552 - Flags: review?(dbolter)
Comment on attachment 8733552 [details] [diff] [review]
ipc fix light

Removing review flag while Trevor is working on an alternative as per our meeting.
Attachment #8733552 - Flags: review?(dbolter)
It looks like awsy runs successfully with this patch and the rebased copy of the original patch.
Attachment #8734437 - Flags: review?(dbolter)
Comment on attachment 8734437 [details] [diff] [review]
make AccShowEvent store the index of the insertion index of the new child

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

Thanks
Attachment #8734437 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a257e0e77f2b17af8147e108eadcbc17964ccf82
Bug 1255009 - insert children into the tree on content insertion instead the recaching, r=yzen
https://hg.mozilla.org/mozilla-central/rev/201cab594d2a
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: CVE-2016-5273
You need to log in before you can comment on or make changes to this bug.