Created attachment 254853 [details] [review]
Patch proposal

Here comes the patch. I'll try to provide a similar fix for WebKitGTK's jhbuild-based development/testing environment, so we can get rid of the issue while we don't reach a version of GTK+ new enough.

(In reply to comment #0)

> So, after talking to Pinheiro yesterday, I believe we should move that
> initialization call from do_post_parse_initialization() to gtk_main(), both to
> make it consistent with the call to _shutdown() and to fix this issue.

FWIW, just in case someone wonders about that, one of the things we mentioned yesterday, is that due how far we are in the current cycle (hard code freeze is next Monday), and that this bug is really specific, and doesn't impact users right now, we shouldn't make this change for 3.10, but for 3.11/12.

Yeah, that's not urgent in that regard, but still I would love to have some feedback on this because depending on whether it's accepted or not I will ask for review over the related patch in WebKit[1], which will fix the aforementioned issue in the TestWebKit2 test suite.

So, even if it's not urgent to integrate it in GTK+ now, I'd certainly appreciate a review as soon as possible :)

Thanks!

[1] https://bugs.webkit.org/show_bug.cgi?id=121294

I think this is a bug on where the exit is happening. I don't think entering/leaving the main loop has anything to do with initializing/stopping subsystems.

Case in point: A lot of applications do not even use gtk_main() but either create their own main loops or just call gtk_main_iteration() (I think Firefox is an example of that).

So what we'd really need to fix is a call to _gtk_accessibility_shutdown(). Unfortunately GTK applications cannot be shut down. So there's no place to put a call to that function.

After some discussion on IRC, we believe the best way to go forward here is to remove the call to shutdown() since there's no point on shutting down a11y if there's not possibility of loading or unloading the bridge anyway as it happened before, when it was a module.

And it seems that the best way to do that would be to revert the patch for bug 684076, which basically introduced the code needed to cleanup a11y on shutdown.

See below the full log from IRC:

<msanchez> Company: mind reviewing this patch? https://bugzilla.gnome.org/show_bug.cgi?id=708024
<Services> Bug 708024: normal, Normal, ---, gtk-bugs, UNCONFIRMED, Accessibility being initialized in the wrong place
* Company stamps REJECTED on msanchez patch
<Company> it's a really tough question how to do that properly
<Company> does GApplication do shutdown?
<msanchez> as far as I could see, shutdown is called whenever you exit from the topmost main loop from gtk_main()
<mgorse> I remember someone (Bastien probably) saying that the atk-bridge really should be a GObject, so making it one should help work around it at least, since then webkit could ref it and prevent gtk from destroying it since Webkit (or its test app) would still hold a ref to it
<msanchez> API explained to me that probably the reason why it was in the parsing function was due to the need of parsing the GTK_MODULES envvar (no longer true)
<msanchez> mgorse: that would certainly help
<msanchez> but what seems to me wrong now in any case is that we shut it down N times but only start it up once
<msanchez> (assuming you call gtk_main() multiple times without nesting calls)
<API> msanchez, Company well, for me, as "accessibility is always on (tm)" calling _gtk_accessibility_init on do_post_parse_initialization seems somewhat extrange
<API> in fact it is always called
<msanchez> API: indeed
<API> although I agree with Company that probably the shutdown is also bad-placed
<API> the problem with mgorse proposal, is that in order to implement it, gtk should provide an API to expose the atk-bridge in use
<mgorse> or atk-bridge could assume that there would only be one, and provide an API to ref/return it
<API> mgorse, yes true
<API> in fact having that API on atk-bridge probably is more sensible than in gtk
* API saying a lot of stuff without a deep analysis (disclaimer)
<Company> API, mgorse: Why do we need to shutdown a11y?
<Company> can't we just exit() the process?
<msanchez> Company: I was about to ask exactly the same. It seems there's some deregistration happening, though
<mgorse> Yeah, I was thinking that, too. If you'redebugging with valgrind, then you probably really want to shut it down; otherwise I'm not sure you should need to
<msanchez> ...through atk_bridge_cleanup(). Not sure whether that's really needed or not
<Company> msanchez: yeah, but the same deregistration should happen if I lovingly kill -9 the process
<msanchez> yeah
<Company> msanchez: at least in the server side - and the client side is dead anyway
<Company> msanchez: so I think I'd be fine with just removing the gtk_a11_shutdown() call
<Company> msanchez: unless mgorse or API have reasons to keep it
<msanchez> agreed. Also, from the point of view of outsiders to gtk+, it's not clear to me either how those (e.g. WebKit) would be reffing/unreffing the bridge object
<msanchez> that's something internal to gtk
<msanchez> Company: removing that would mean removing not only the call, but probably the whole function as well
<API> Company, client side is dead anyway? fwiw, we usually call client-side ATs (orca etc) and server-side the apps 
<msanchez> I'm not sure then why the atk_bridge_cleanup() function -called from there- is needed then
<Company> API: yeah, I was thinking of the registry deamon as server side when I said that
* Company has no idea what atk_bridge_cleanup() does
<API> Company, ok, yes, it depends on the pov
<API> I'm taking a look right now to atk_bridge_adaptor_cleanup
<msanchez> Company, API: it's also called from gtk_application_shutdown (GApplication *application) it seems
<msanchez> so in the end we probably would just remove that call
<API> surpringlsy, it does some cleanup
<API> but not sure if it is really needed or not to do the explicit call
<msanchez> curiously enough, there's no call to gtk_accessibility_init() in gtk_application_startup(). Shouldn't there one ?
<Company> msanchez: it's done by gtk_init()
<API> probably that is another leftover from the old period of having modules
<API> the still existing gtk+2 module
<Company> msanchez: but gtk_init() only does everything once and then sets inited=TRUE;
<API> calls that method
<API> in that context, calling cleanup made sense
<Company> msanchez: so if you then unload a11y you'll never get it back by calling gtk_init() again
<Company> API: that explanation makes sense
<API> as in theory you could load/unload modules (although probably doing that with gail/atk-bridge would be a bad idea)
<Company> in theory, deregister code makes sense for clean valgrind profiles, but we never bothered with keeping valgrind clean in the whole stack
<API> Company, yes, that also makes sense
<API> shutdown was added just for that
<API> https://bugzilla.gnome.org/show_bug.cgi?id=684076
<Services> Bug 684076: normal, Normal, ---, gtk-bugs, RESOLVED FIXED, clean up a11y on shutdown
<msanchez> so, the the right thing to do now is just to remove the shutdown from gtk_main()?
<API> so that means that cleanup was not called during some time
<API> and everybody were "happy"
<Company> msanchez: and from gtk_application_shutdown(), too. If we want to keep it, we should put it into an atexit() handler
<API> Company, msanchez on gtk_applicaiton_shutdown was also added on that bug
<msanchez> API: I'm seeing it now
<Company> msanchez: so essentially, reopen that bug and revert the patch
<Company> msanchez: and explain why the approach doesn't work

Marking as duplicate of the old bug, where work will continue.

*** This bug has been marked as a duplicate of bug 684076 ***