GNOME Bugzilla – Bug 727824
Crash when enabling Zoom a11y feature
Last modified: 2014-05-08 11:01:01 UTC
The attached patch fixes the crash and seems to work correctly. This seems like it used to work in the past though and since I'm not very familiar with atspi's API I'm not sure this is the right fix.
Created attachment 273796 [details] [review] magnifier: Don't explicitly get object interfaces Introspection takes care of this for us. Trying to do it explicitly just crashes gjs.
Here's the backtrace: Program received signal SIGABRT, Aborted. 0x00007f81031dac39 in raise () from /lib64/libc.so.6 (gdb) bt
+ Trace 233446
And the JS backtrace: == Stack trace for context 0x2ad65e0 == ZoomRegion<._updateCaret@resource:///org/gnome/shell/ui/magnifier.js:737 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _emit@resource:///org/gnome/gjs/modules/signals.js:124 FocusCaretTracker<._onChanged@resource:///org/gnome/shell/ui/focusCaretTracker.js:48 wrapper@resource:///org/gnome/gjs/modules/lang.js:169
The assertion message actually makes this look like a gjs bug:
+ Trace 233447
Review of attachment 273796 [details] [review]: ::: js/ui/magnifier.js @@ -719,2 @@ _updateFocus: function(caller, event) { - let component = event.source.get_component_iface(); As mentioned on IRC, get_component_iface is a convenient utility API that checks if the objects implements component interface, and returns a cast to that interface if that is the case. But is just a cast, so it is in fact the same object. Replacing this call for just event.source would work because all the accessible objects (or at least most of them) implements this interface. @@ -736,2 @@ _updateCaret: function(caller, event) { - let text = event.source.get_text_iface(); But doing the same here would not work, as not all the objects implement text interface. If we need a workaround (although as I mentioned yesterday, this regression seems to be on gobject-introspection or gjs, because .get_text_iface are not doing strange things), one option would be call atspi_accessible_get_interfaces (so event.source.get_interfaces), and if the list of implemented interfaces doesn't include text (component on the previous case), returns early. Assuming that that would work (because get_text_iface should also work).
This seems like it's fallout from the fundamental types support that landed in gjs. See https://bugzilla.gnome.org/show_bug.cgi?id=621716 .
Created attachment 273805 [details] [review] gjs: gi: arg: use instance's gtype rather than the introspection data Here is a patch for Gjs to improve the situation a bit. Let me know if it helps.
Created attachment 273811 [details] [review] gjs: gi: arg: use instance's gtype rather than the introspection data Better version of that previous patch. I need to write a test case for gjs, not much time right now...
Created attachment 273812 [details] Simple Gjs script to reproduce
(In reply to comment #8) > Created an attachment (id=273811) [details] [review] > gjs: gi: arg: use instance's gtype rather than the introspection data > > Better version of that previous patch. > I need to write a test case for gjs, not much time right now... This one works fine. I don't feel like I can review this though, moving to gjs.
(In reply to comment #8) > Created an attachment (id=273811) [details] [review] > gjs: gi: arg: use instance's gtype rather than the introspection data > > Better version of that previous patch. > I need to write a test case for gjs, not much time right now... Just want to confirm that this patch also works for me.
Created attachment 274054 [details] [review] gobject-introspection: tests: add implementation of an interface
Created attachment 274055 [details] [review] gjs: gi: arg: use instance's gtype rather than the introspection data Includes regression test.
Review of attachment 273796 [details] [review]: I think the updated patch should be pushed. The words "with great power, comes great responsibility are relevant here. You know, it would be just spiffing if, gnome shell maintainers would just bother to check the changes they make to the magnifier component before going ahead and pushing them... It is not rocket science and would really save bugs like this from ending up in master. Thanks for your work in finding the cause of this problem Rui. ::: js/ui/magnifier.js @@ -719,2 @@ _updateFocus: function(caller, event) { - let component = event.source.get_component_iface(); echo this
*** Bug 727684 has been marked as a duplicate of this bug. ***
I might not understand correctly what you're saying here, but this bug is more of a gjs problem. It was introduced when I added support for fundamental types. Even though I made sure the test suite passed at the time, we were missing a test case to detect that particular problem.
Review of attachment 274055 [details] [review]: The commit message is slightly wrong: interfaces can only be attached to instantiatable types (so no GBoxed or basic). Also, g_type_is_a() takes care of prerequisite - which is why this usually works - and which is why we should fix the libraries to prerequire GObject for all interfaces (the G_DEFINE_INTERFACE macro make it easy, libraries should use that). Other than that, looks good.
Review of attachment 274054 [details] [review]: Should probably add a comment to GIMarshallingTestsInterface saying that we're not putting a prerequisite on purpose.
(In reply to comment #16) > I might not understand correctly what you're saying here, but this bug is more > of a gjs problem. It was introduced when I added support for fundamental types. Yes, you are right. Nobody changed the magnifier code at all. Is not a magnifier bug. And that is the reason the bug was moved from gnome-shell to gjs. > Even though I made sure the test suite passed at the time, we were missing a > test case to detect that particular problem. Test suites would help to prevent regressions, but would not catch all of them. This kind of things happens. And thank you for your quick patches, even when you mentioned that you haven't too much time.
Pushed to master.
(In reply to comment #19) > (In reply to comment #16) > > I might not understand correctly what you're saying here, but this bug is more > > of a gjs problem. It was introduced when I added support for fundamental types. > > Yes, you are right. Nobody changed the magnifier code at all. Is not a > magnifier bug. And that is the reason the bug was moved from gnome-shell to > gjs. Then why was the caret tracker calling the component interface? Caret tracking simply would not work with that call so it sure as hell was not a leftover I can take responsibility for because the tracking would never have worked with that line of code. It must have been changed.
s/calling/querying.
Ok i must have been looking at the wrong patch. Apologies, I have now had a moment to double check the magnifier log and I see that no change was made to the text interface query which is good. It would have broken stuff. Still. Be careful with my baby. I don't have time to pay attention to it anymore so it's yours now.
*** Bug 728082 has been marked as a duplicate of this bug. ***
This crash still exists in Ubuntu 14.04 (magnification was enabled somehow, gnome crashed on login), here is the workaround I used. Launch a gnome session, it will freeze. Switch to a tty using Ctrl-Alt-F6. Run: DISPLAY=:0 gsettings set org.gnome.desktop.a11y.applications \ screen-magnifier-enabled false
Looking at packages.ubuntu.com, Trusty doesn't seem to be up to date on the latest version of gjs.
Looks like it's going to be fixed: https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1292357