Created attachment 255313 [details] [review]
My first attempt at a patch.

Patch to run a GMainLoop when making AT-SPI calls. Unfortunately, this just causes another deadlock. See following attachment.

Created attachment 255315 [details]
Backtrace from gnome-shell when the above patch is used.

Created attachment 255339 [details] [review]
Patch to lower AT-SPI's timeout values used by gnome-shell

This patch seems to mitigate the issue for me, although it doesn't fix the underlying problem and might cause the magnifier to behave incorrectly when switching to the display panel.

Doing the following would be another, more invasive option that should prevent the deadlock, rather than just shorten it:
- in at-spi2-atk, have signals (StateChanged at least) send a list of properties with the event. The list of supported interfaces could be sent this way, along with the extents of the focused item. At-spi2-core could store this information so that, when the focusCaretTracker calls get_component and get_extents, it does not need to make a D-Bus call to get the information. This won't comprehensively eliminate the potential for deadlocks, so I think that lowering the timeout is a good idea regardless of whether the AT-SPI changes are done for 3.10, but it should suffice for solving this particular deadlock (and other deadlocks that happen when a change of focus leads to gnome-shell being queried over D-Bus).

I don't see any other way of fixing this for 3.10. I think that a proper fix would involve adding the ability for a client to asynchronously fetch the information that it will need in one go, but this is beyond the scope for 3.10.

Moved to gnome-shell for the time being, at least, since I think it's better to commit the patch than not to.

Gpod work Mike 

Just realised I had not fully updated gnome-shell so I am doing that now and then hopefully i will recreate the bug because I have not been able to do that yet.

I think I can find the culprit since speaking to Mike and reading the information he got for us with his stacktrace.

The trace indicates the component interface I am starting there and posting the exception message back here as soon as I can reproduce the freeze.

I am posting the stack for Mike Gorse's trace when the freeze is upon him because I still have not managed to recreate this bug in order to make my own tests useful, the other trace might not be caused by the bug at all. Both implicate the component interface so that is suspect number one until something says otherwise. So,

Can those who CAN recreate the bug try turning off focus tracking then see if the bug goes away please. That will hopefully narrow it down. To do that do this in the command prompt:  

$ gsettings set org.gnome.desktop.a11y.magnifier focus-tracking none

Mike's trace during freeze up 

(gdb) bt

+ Trace 232507

A debugging session is active.

	Inferior 1 [process 7864] will be detached.

Quit anyway? (y or n) y
Detaching from program: /opt/gnome/bin/gnome-shell, process 7864
sh-4.2$ exit

Script done on Thu 19 Sep 2013 08:53:52 PM CD

(In reply to comment #1)
> Created an attachment (id=255313) [details] [review]
> My first attempt at a patch.
> 
> Patch to run a GMainLoop when making AT-SPI calls. Unfortunately, this just
> causes another deadlock. See following attachment.

Please don't. I fought already enough reentrancy issues with recursive mainloops in other projects, I will strongly oppose introducing it in the shell.

(In reply to comment #3)
> Created an attachment (id=255339) [details] [review]
> Patch to lower AT-SPI's timeout values used by gnome-shell
> 
> This patch seems to mitigate the issue for me, although it doesn't fix the
> underlying problem and might cause the magnifier to behave incorrectly when
> switching to the display panel.
> 
> Doing the following would be another, more invasive option that should prevent
> the deadlock, rather than just shorten it:
> - in at-spi2-atk, have signals (StateChanged at least) send a list of
> properties with the event. The list of supported interfaces could be sent this
> way, along with the extents of the focused item. At-spi2-core could store this
> information so that, when the focusCaretTracker calls get_component and
> get_extents, it does not need to make a D-Bus call to get the information. This
> won't comprehensively eliminate the potential for deadlocks, so I think that
> lowering the timeout is a good idea regardless of whether the AT-SPI changes
> are done for 3.10, but it should suffice for solving this particular deadlock
> (and other deadlocks that happen when a change of focus leads to gnome-shell
> being queried over D-Bus).
> 
> I don't see any other way of fixing this for 3.10. I think that a proper fix
> would involve adding the ability for a client to asynchronously fetch the
> information that it will need in one go, but this is beyond the scope for 3.10.

If the DBus change is doable for 3.10 (which I'm not sure, but depends on you), then this is the best option for me. Otherwise, lowering the timeout might do.

As a very hacky workaround, we could also introduce a small timeout between the signal and the dbus call, so that we can process other stuff in between. Possibly, we can tie this hack to the app being gnome-control-center.

Moving back to AT-SPI, since the bug is really there, although ultimately it'll be up to the release team to decide how to handle it for 3.10, seeing as we're in code freeze...

Created attachment 255429 [details] [review]
Patch for at-spi2-core

This is needed along with a patch to at-spi2-atk.

Created attachment 255430 [details] [review]
Patch for at-spi2-atk.

Ideally, the data sent with events would be configurable, but, if this is going into 2.10, then it needs to go in now...

I should cc API on this the more I look at your trace the more I think it has to do with the javascript interface situation I have built this on both my laptop and desktop now and, although I have spotted some ATSPI logs and a gjs synchronous exception I still can't see freezing.

How about you look into the C and I get the javascript and hopefully together we can at the worst minimise the call or I can maybe even use a clutter focus method (though wee really need to test this on the caret as well if we can. 

I filed all my bugs in class today so I can wipe over and start again in case my laptop has more information due to lmuch less impressive spec. 

Next I will add a few more logs to the magnifier work. I made another script out of the git details one. This one updates all modules at once then rebuilds the whole gnome-shell and its deps. I will add it to the bug for convenience of anyone who wants something quick and easy.

There is a chance something is behind on one of you or a random module is involved somewhere :/) because I am up to date and that means the bug is got to be somewhere in a previous commit, doesn't it?

If you are not certain then please check that one again for atspi and its dependencies as well as g-s so nothing is missed. Does that make sense? Any more ideas since I went out earlier?  

I think Mike was right to bring the bug back here to look at the c codes that way I can try and tackle the javascript and hopefully between us it will get fixed well enough and we can look at the root causes when we have a less time sensitive situation on our hands.

My current idea is that I can move Mike's thread wait patch to happen after the offending interface query and if I calculate the exact time needed to run the timer I can reduce the problem. Hopefully in the meantime he will do something to limit or even solve the problem from this module.

pointerWatcher has a timer also and for the record I noted Clutter is also in this bug so it's not only atspi implicated I would like to hear thoughts and advice about this. I if a catch does not give a meaningful enough message on it's own to get this fixed completely for release which is obviously what would be the best possible situation but lets us see what the evening brings.

Suspect number 1 for me right now is the dodgy interface methods that broke my laptop until API skipped them so I will have a quick look there before I get going on the tests

Mike I just saw you added another patch!. Great work! how have you found it? Can someone who can recreate this bug review Mike's patch?

Created attachment 255443 [details]
handy jhbuild 'git pull' moduleset updater script

I am not sure if the git stashing is perfect because I added that afterwards since I had nothing to stash on the desktop. Please check it makes sense to you or you backed up before running that in case you lose some work by some accident.

Created attachment 255447 [details]
not frozen with this moduleset installed.

Adding since it seems that the bug is not affecting my desktop. However it could just be I have a better spec computer that can handle it so here are my specs that i have tp hand:

8GB ddr3 RAM
4 core Intel i5-3570 @ 3.4 GHz
AMD 2GB ddr3 Graphic


The rest I cannot remember case anyone gets any eureka moments from seeing it then ask as more info can be found on request. For now I want to use my time on helping with this bug as much as I can.

Magdalen, I wonder if you're starting gnome-control-center and then immediately switching to the display panel when you try to reproduce it. If, if you start gnome-control-center, wait 15 seconds or more, and then switch, then afaik you won't see the freeze.

Anyway, I've sent an email to the release team with the various options I see for fixing/mitigating it.

Comment on attachment 255339 [details] [review]
Patch to lower AT-SPI's timeout values used by gnome-shell

Committed this, since it has had two approvals from the release team.

This is news to me. Can someone please copy me the release term discussion. As I understand thing the at-spi2-core patch was a possible solution but as far as know the pushed commit in gnome-shell was a possible temporary measure until a complete fix was ready. 

The problem as I understand it:

1. Is not due to atspi init deadlocking with any old application it is deadlocking with one particular application in particular: The gnome control center display options.

2. We already know this deadlock is between a query atspi's component interface and some query the display options makes. 
3. We know that this only happens when the atspi interface is initialising at the time of the query.

Please correct me if the above is not correct.

4. We do not know what interface the display options query is coming from.
5. We do not know for 'definite' the time needed to query the component interface and not see a deadlock.
6. We do not know why the deadlock occurs with the display options query in particular.
7. We do not know why nothing else in gnome shell brings out an atspi deadlock.

If anyone does actually know the answer to any of the points above, please enlighten me so I can understand because although I think it is great that Mike thought of a good workaround, I fully disagree with the decision to close this bug and call then call it fixed. 

On that basis, I propose we use another interface for listening to focus events until we can determine a full solution and I will be looking into this today. The reason I make such a proposal is because the problem does not seem to be involved with atspi's text interface I do not see a reason to change anything to do with atspi initialisation, at all. Accordingly, I will open this in gnome-shell if I can convince myself that it can be solved there. 

I also want to see the following two commits reverted if that works out well:

1. https://git.gnome.org/browse/gnome-shell/commit/?id=7ced1f5b543dc2ea6bd52230bdec1c0e12959740

2. https://git.gnome.org/browse/gnome-shell/commit/?id=193f872ebedfb1fb1107a428b595855812f42153

Advice/comments/criticism welcome. It is possible there is some information available to the experts that I am not aware of and that I could be looking at the situation in the wrong way. For now, what is written here is all have to go on.

For anyone else reading this, I forwarded the release-team thread to Magdalen, and the release team members who commented (Matthias in particular) preferred the gnome-shell patch that I proposed over the AT-SPI patch since the latter is complex for something being added at the very last minute, and the former also prevented the long freeze. So a more proper fix will go into AT-SPI for 3.11.

Yeah, something like the following (completely untested) would probably be another way to fix it, since it would hopefully give gnome-shell enough time to see the gnome-control-center query (and thus respond to it) before querying gnome-control-center:
731,736c731,740
<         let component = event.source.get_component_iface();
<         if (!component || event.detail1 != 1)
<             return;
<         let extents = component.get_extents(Atspi.CoordType.SCREEN);
<         [this._xFocus, this._yFocus] = [extents.x, extents.y]
<         this._centerFromFocusPosition();
---
>         if (this._focusTimeoutId == 0)
>             this._focusTimeoutId = Mainloop.timeout_add(50,
>                 Lang.bind(this, function() {
>                 let component = event.source.get_component_iface();
>                 if (!component || event.detail1 != 1)
>                     return;
>                 let extents = component.get_extents(Atspi.CoordType.SCREEN);
>                 [this._xFocus, this._yFocus] = [extents.x, extents.y]
>                 this._centerFromFocusPosition();
>         }));

The up-side to that, compared to the Atspi.set_timeout() change, is that, if there are cases where an app takes longer than 250 ms to respond to an AT-SPI query but nevertheless responds, then the magnifier will still behave correctly. There are also a couple of downsides:
- The magnifier will always wait 50 ms (or whatever we set the timeout to) before processing any focus events. It might be possible to make the change specific to events coming from gnome-control-center, but it would need to be done in a way that doesn't query gnome-control-center when receiving the event. This may not be noticable, so it is probably a minor consideration, although I think that that could be said for any of the considerations in choosing between various work-arounds.
- If the GLib timeout goes off too soon, in this case or if there are other, similar cases where a focus change in another app results in a synchronous D-Bus call to gnome-shell, then we may miss the query ahd still end up deadlocking.

So both of these are work-arounds, and it would be better to fix AT-SPI, but fixing AT-SPI is really a 3.11-type change.

(In reply to comment #18)
> For anyone else reading this, I forwarded the release-team thread to Magdalen,

I did not lie about receiving no such thread. Mike must have typed my address incorrectly if he says he sent something. How about ccing (if there is a) next time?

Here it is https://mail.gnome.org/archives/release-team/2013-September/msg00166.html

> and the release team members who commented (Matthias in particular) preferred
> the gnome-shell patch that I proposed over the AT-SPI patch since the latter is
> complex for something being added at the very last minute, and the former also
> prevented the long freeze. So a more proper fix will go into AT-SPI for 3.11.
> 
> Yeah, something like the following (completely untested) would probably be
> another way to fix it, since it would hopefully give gnome-shell enough time to
> see the gnome-control-center query (and thus respond to it) before querying
> gnome-control-center:
> 731,736c731,740
> <         let component = event.source.get_component_iface();
> <         if (!component || event.detail1 != 1)
> <             return;
> <         let extents = component.get_extents(Atspi.CoordType.SCREEN);
> <         [this._xFocus, this._yFocus] = [extents.x, extents.y]
> <         this._centerFromFocusPosition();
> ---
> >         if (this._focusTimeoutId == 0)
> >             this._focusTimeoutId = Mainloop.timeout_add(50,
> >                 Lang.bind(this, function() {
> >                 let component = event.source.get_component_iface();
> >                 if (!component || event.detail1 != 1)
> >                     return;
> >                 let extents = component.get_extents(Atspi.CoordType.SCREEN);
> >                 [this._xFocus, this._yFocus] = [extents.x, extents.y]
> >                 this._centerFromFocusPosition();
> >         }));
> 
> The up-side to that, compared to the Atspi.set_timeout() change, is that, if
> there are cases where an app takes longer than 250 ms to respond to an AT-SPI
> query but nevertheless responds, then the magnifier will still behave
> correctly. There are also a couple of downsides:
> - The magnifier will always wait 50 ms (or whatever we set the timeout to)
> before processing any focus events. It might be possible to make the change
> specific to events coming from gnome-control-center, but it would need to be
> done in a way that doesn't query gnome-control-center when receiving the event.
> This may not be noticable, so it is probably a minor consideration, although I
> think that that could be said for any of the considerations in choosing between
> various work-arounds.
> - If the GLib timeout goes off too soon, in this case or if there are other,
> similar cases where a focus change in another app results in a synchronous
> D-Bus call to gnome-shell, then we may miss the query ahd still end up
> deadlocking.

I understood very well that you prefer using your patch rather than letting me try the idea above, when I saw your commit. I thought we were working collaboratively to solve a problem directly impacting my gnome-shell contribution. Giovanni and I were making suggestions to help not to compete for the 'best' workaround in fact Giovanni was suggesting something to include to it. I am only an intern, but if anyone is going to hack my project work, (let alone one of my GSoC mentors during my last week:/) they should be prepared to communicate.

> So both of these are work-arounds, and it would be better to fix AT-SPI, but
> fixing AT-SPI is really a 3.11-type change.

The worst case scenario has no place in the tracking features first steps. With respect I am going to keep looking for a complete answer in the present regardless of what you want to do about atspi. The offer is still there if you want help anywhere in atspi. 
From what I know of the gnome-shell this workaround will not be popular for that estimate of time and I want to be respectful of that especially since they have helped a lot with getting the feature in. I also want it to work well for the sake of its users.

(In reply to comment #19)
> (In reply to comment #18)

> I understood very well that you prefer using your patch rather than letting me
> try the idea above, when I saw your commit. I thought we were working
> collaboratively to solve a problem directly impacting my gnome-shell
> contribution. Giovanni and I were making suggestions to help not to compete for
> the 'best' workaround in fact Giovanni was suggesting something to include to
> it. I am only an intern, but if anyone is going to hack my project work, (let
> alone one of my GSoC mentors during my last week:/) they should be prepared to
> communicate.

A lot of people was working collaboratively. Joanie sent me the logs of the #a11y channel these days and a lot of people where working hard to solve this bug before today deadline. Jasper, Giovanni, Matthias, Joseph, Mike and you. Even working during this weekend. Thanks everybody. With so many people involved, I don't see strange some missed messages. 

In any case, as far as I see, it seems that you are implying that Mike made that commit under the hood, by his own decision. But he investigated the problem, provided two different options, on gnome-shell and at-spi, based on all the discussion he had with all the people involved, and before commiting it, it asked for permission from the release team on a public mailing list. In my humble opinion, Mike Gorse followed the protocol, and taking into account all the comments on this bug and IRC chats he tried to keep you on the loop all the time.

> > So both of these are work-arounds, and it would be better to fix AT-SPI, but
> > fixing AT-SPI is really a 3.11-type change.
> 
> The worst case scenario has no place in the tracking features first steps. With
> respect I am going to keep looking for a complete answer in the present
> regardless of what you want to do about atspi. The offer is still there if you
> want help anywhere in atspi. 

At this moment is not about what Mike want to do with atspi now. Mike also proposed to do the changes on at-spi, and release-team vote was with going with the safer option:
https://mail.gnome.org/archives/release-team/2013-September/msg00169.html

In any case, thanks for the offer to help on atspi. I think that a good first step would test Mike Gorse patches on at-spi2-core/atk to confirm that they solve the problem and that they don't cause any regression for future consideration.

> From what I know of the gnome-shell this workaround will not be popular for
> that estimate of time and I want to be respectful of that especially since they
> have helped a lot with getting the feature in.

Yes, Jasper, Giovanni and Florian were really hepful to get this feature in (again, thank you very much). Mike also solved some bugs on at-spi in order to get the feature in.

> I also want it to work well for the sake of its users.

With my release team hat on: we are right now on code freeze. That means that we are not in a position to commit a change if it is risky. It is true that the current situation is not ideal, but we don't want to risk to add a worse regression. Sometimes we need to compromise. This is not the first time and it will not be the last one.

Mike sent me the email and I did not receive it. The mystery got solved and neither of us are imagining things. 

Mike sent it from an address that I did not have on record and although my junk folder was empty but a search on his address pulled up the email from 22nd. So that is one mystery solved.

(In reply to comment #20)

> I don't see strange some missed messages. 

I do not make up stories. https://www.dropbox.com/s/o3xb97nl4x6uf51/Screenshot%20from%202013-09-23%2020%3A57%3A25.png

> In any case, as far as I see, it seems that you are implying that Mike made
> that commit under the hood, by his own decision. 

I very stated just the facts and I made no accusations about anyone. When you announce that I am 'implying' anything about Mike then your words become weak, please try not to attack my character. This thread is about fixing a bug: I think the bug should remain open because the bug has not been fixed. If the protocol is to close bugs that are not fixed then I think the protocol needs to be reviewed in my view.

> 
> > > So both of these are work-arounds, and it would be better to fix AT-SPI, but
> > > fixing AT-SPI is really a 3.11-type change.
> > 
> > The worst case scenario has no place in the tracking features first steps. With
> > respect I am going to keep looking for a complete answer in the present
> > regardless of what you want to do about atspi. The offer is still there if you
> > want help anywhere in atspi. 
> 
> At this moment is not about what Mike want to do with atspi now. Mike also
> proposed to do the changes on at-spi, and release-team vote was with going with
> the safer option:
> https://mail.gnome.org/archives/release-team/2013-September/msg00169.html
> 

I should have been involved in that discussion from the beginning but I am glad to find that Mike realised this (albeit a bit later than I would have liked) but that is life. It is unfortunate that his email was marked a spam but neither of us are to blame for that.

> In any case, thanks for the offer to help on atspi. I think that a good first
> step would test Mike Gorse patches on at-spi2-core/atk to confirm that they
> solve the problem and that they don't cause any regression for future
> consideration.

If Mike wants me to help him, then I am sure he will ask me and I hope he does. 

> 
> > From what I know of the gnome-shell this workaround will not be popular for
> > that estimate of time and I want to be respectful of that especially since they
> > have helped a lot with getting the feature in.

> > I also want it to work well for the sake of its users.
> 
> With my release team hat on: we are right now on code freeze.

The freeze is more or less finished. It is the 3.11 estimate that concerns me. This is too long to leave the problem when there are other alternatives but we shall see what happens next.

(In reply to comment #21)
> (In reply to comment #20)
> 
> > I don't see strange some missed messages. 
> 
> I do not make up stories.

I was not implying that you were making up stories. What I tried to say is that when several people is involved, communication breakdowns can happen, for whatever and unintentional reasons.

> > In any case, as far as I see, it seems that you are implying that Mike made
> > that commit under the hood, by his own decision. 
> 
> I very stated just the facts and I made no accusations about anyone. When you
> announce that I am 'implying' anything about Mike then your words become weak,
> please try not to attack my character. 

Was not trying to attack your character, just explaining what I was understanding from your words. If that is not the case, then the better.

> This thread is about fixing a bug: I
> think the bug should remain open because the bug has not been fixed. If the
> protocol is to close bugs that are not fixed then I think the protocol needs to
> be reviewed in my view.

Yes, it is true that it would be good to track the fact that the current solution is not the best one. In any case, as a solution was already committed, and the bug was closed, I think that what it is worth is open a new bug. Something like "Need to improve the solution to the freeze on gnome-control-center with the magnifier", but with proper wording, or probably going to the underlying reasons of the bug.

> > In any case, thanks for the offer to help on atspi. I think that a good first
> > step would test Mike Gorse patches on at-spi2-core/atk to confirm that they
> > solve the problem and that they don't cause any regression for future
> > consideration.
> 
> If Mike wants me to help him, then I am sure he will ask me and I hope he does. 

Ok. Thanks.

> > > From what I know of the gnome-shell this workaround will not be popular for
> > > that estimate of time and I want to be respectful of that especially since they
> > > have helped a lot with getting the feature in.
> 
> > > I also want it to work well for the sake of its users.
> > 
> > With my release team hat on: we are right now on code freeze.
> 
> The freeze is more or less finished. It is the 3.11 estimate that concerns me.
> This is too long to leave the problem when there are other alternatives but we
> shall see what happens next.

Just an acclaration, when I talk about 3.11 I was talking about all the cycle, not about leaving this unsolved till the end of the 3.11 cycle. Obviously we would like this bug solved as soon as possible. The schedule for 3.11 is not still available, but let's take a look to 3.9 cycle:

https://wiki.gnome.org/ThreePointNine

3.8.0 was released on Mar 27
The first 3.9.x release, 3.9.1, was released on Apr 29. Just one month later. 

3.11 cycle will be similar. 3.11.1 will be in less that a moth. That release will be marked as unstable, so we can be less worried about submitting riskier solutions (obviously, with care), and we will have a 6-month cycle to test that solution.

So, ideally, it would be good to have the proper solution for 3.11.1, that will be scheduled in less than a month.

My rationale for marking the bug fixed is that gnome-control-center no longer freezes for a long time, as stated in the summary, although I realize that it is debatable since it is worked around rather than properly fixed. I just filed bug 708695 for the underlying issue of our needing a way to get needed information via AT-SPI without needing to make synchronous D-Bus calls.

>3.8.0 was released on Mar 27
>The first 3.9.x release, 3.9.1, was released on Apr 29. Just one month later. 

>3.11 cycle will be similar. 3.11.1 will be in less that a moth. That release
>will be marked as unstable, so we can be less worried about submitting riskier
>solutions (obviously, with care), and we will have a 6-month cycle to test that
>solution.

>So, ideally, it would be good to have the proper solution for 3.11.1, that will
>be scheduled in less than a month.

Ok, thanks for clarifying Alejandro. If GNOME Shell are happy with that then I am.

Mike, thanks for filing bug 708695 and for presenting lots of information about the bug in there too.