After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 727824 - Crash when enabling Zoom a11y feature
Crash when enabling Zoom a11y feature
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal critical
: ---
Assigned To: gjs-maint
gjs-maint
3.12.1
: 727684 728082 (view as bug list)
Depends on: 728004
Blocks:
 
 
Reported: 2014-04-08 12:34 UTC by Rui Matos
Modified: 2014-05-08 11:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
magnifier: Don't explicitly get object interfaces (1.11 KB, patch)
2014-04-08 12:34 UTC, Rui Matos
reviewed Details | Review
gjs: gi: arg: use instance's gtype rather than the introspection data (1.83 KB, patch)
2014-04-08 14:29 UTC, Lionel Landwerlin
none Details | Review
gjs: gi: arg: use instance's gtype rather than the introspection data (2.37 KB, patch)
2014-04-08 15:23 UTC, Lionel Landwerlin
none Details | Review
Simple Gjs script to reproduce (701 bytes, application/javascript)
2014-04-08 15:24 UTC, Lionel Landwerlin
  Details
gobject-introspection: tests: add implementation of an interface (3.95 KB, patch)
2014-04-10 23:41 UTC, Lionel Landwerlin
committed Details | Review
gjs: gi: arg: use instance's gtype rather than the introspection data (2.93 KB, patch)
2014-04-10 23:43 UTC, Lionel Landwerlin
committed Details | Review

Description Rui Matos 2014-04-08 12:34:46 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.
Comment 1 Rui Matos 2014-04-08 12:34:49 UTC
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.
Comment 2 Rui Matos 2014-04-08 12:35:23 UTC
Here's the backtrace:

Program received signal SIGABRT, Aborted.
0x00007f81031dac39 in raise () from /lib64/libc.so.6
(gdb) bt
  • #0 raise
    from /lib64/libc.so.6
  • #1 abort
    from /lib64/libc.so.6
  • #2 g_assertion_message
  • #3 g_assertion_message_expr
  • #4 init_fundamental_instance
    at gi/fundamental.cpp line 167
  • #5 gjs_object_from_g_fundamental
    at gi/fundamental.cpp line 817
  • #6 gjs_value_from_g_argument
    at gi/arg.cpp line 2668
  • #7 gjs_invoke_c_function
    at gi/function.cpp line 973
  • #8 function_call
    at gi/function.cpp line 1234
  • #9 js::CallJSNative
  • #10 js::Invoke
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 474
  • #11 Interpret
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 2298
  • #12 js::RunScript
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 438
  • #13 js::Invoke
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 500
  • #14 js_fun_apply
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/jsfun.cpp line 982
  • #15 js::CallJSNative
  • #16 js::Invoke
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 481
  • #17 js::Invoke
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/vm/Interpreter.cpp line 531
  • #18 js::jit::DoCallFallback
    at /home/rmatos/Source/git.gnome.org/mozjs-24.2.0/js/src/jit/BaselineIC.cpp line 7007
  • #19 ??
  • #20 ??
  • #21 ??
  • #22 ??
  • #23 ??
  • #24 js::jit::CreateThisInfo
    from /home/rmatos/Source/git.gnome.org/install/lib/libmozjs-24.so
  • #25 ??
  • #26 ??
  • #27 ??
  • #28 ??
  • #29 ??
  • #30 ??
  • #31 ??
  • #32 ??
  • #33 ??
  • #34 ??
  • #35 ??
  • #36 ??
  • #37 ??
  • #38 ??
  • #39 ??
  • #40 ??
  • #41 ??
  • #42 ??
  • #43 ??
  • #44 ??
  • #45 ??
  • #46 ??
  • #47 ??
  • #48 ??
  • #49 ??
  • #50 ??
  • #51 ??
  • #52 ??
  • #53 ??
  • #54 ??

Comment 3 Rui Matos 2014-04-08 12:37:37 UTC
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
Comment 4 Rui Matos 2014-04-08 12:44:20 UTC
The assertion message actually makes this look like a gjs bug:

  • #3 g_assertion_message_expr

Comment 5 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-04-08 12:47:58 UTC
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).
Comment 6 Rui Matos 2014-04-08 12:50:44 UTC
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 .
Comment 7 Lionel Landwerlin 2014-04-08 14:29:13 UTC
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.
Comment 8 Lionel Landwerlin 2014-04-08 15:23:12 UTC
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...
Comment 9 Lionel Landwerlin 2014-04-08 15:24:09 UTC
Created attachment 273812 [details]
Simple Gjs script to reproduce
Comment 10 Rui Matos 2014-04-08 15:31:51 UTC
(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.
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-04-08 15:48:15 UTC
(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.
Comment 12 Lionel Landwerlin 2014-04-10 23:41:38 UTC
Created attachment 274054 [details] [review]
gobject-introspection: tests: add implementation of an interface
Comment 13 Lionel Landwerlin 2014-04-10 23:43:01 UTC
Created attachment 274055 [details] [review]
gjs: gi: arg: use instance's gtype rather than the introspection data

Includes regression test.
Comment 14 Magdalen Berns (irc magpie) 2014-04-11 12:19:24 UTC
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
Comment 15 Magdalen Berns (irc magpie) 2014-04-11 12:22:08 UTC
*** Bug 727684 has been marked as a duplicate of this bug. ***
Comment 16 Lionel Landwerlin 2014-04-11 12:51:55 UTC
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.
Comment 17 Giovanni Campagna 2014-04-11 16:58:43 UTC
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.
Comment 18 Giovanni Campagna 2014-04-11 16:59:26 UTC
Review of attachment 274054 [details] [review]:

Should probably add a comment to GIMarshallingTestsInterface saying that we're not putting a prerequisite on purpose.
Comment 19 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-04-11 17:16:51 UTC
(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.
Comment 20 Lionel Landwerlin 2014-04-11 17:27:06 UTC
Pushed to master.
Comment 21 Magdalen Berns (irc magpie) 2014-04-12 01:12:31 UTC
(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.
Comment 22 Magdalen Berns (irc magpie) 2014-04-12 01:12:53 UTC
s/calling/querying.
Comment 23 Magdalen Berns (irc magpie) 2014-04-12 01:26:23 UTC
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.
Comment 24 Jasper St. Pierre (not reading bugmail) 2014-04-12 14:33:21 UTC
*** Bug 728082 has been marked as a duplicate of this bug. ***
Comment 25 Gabriel de Perthuis 2014-05-08 10:53:50 UTC
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
Comment 26 Lionel Landwerlin 2014-05-08 10:56:49 UTC
Looking at packages.ubuntu.com, Trusty doesn't seem to be up to date on the latest version of gjs.
Comment 27 Gabriel de Perthuis 2014-05-08 11:01:01 UTC
Looks like it's going to be fixed:
https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1292357