327530 - gobject assertion failures when running yelp with a11y enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

Note that this is similar to bug 312092, but this needs to be really fixed for all embedders, not just worked around in some mozilla applications.
--

When running yelp (a gtkmozembed embedder) with a11y enabled, some people see warnings on console [i.e. http://bugzilla.gnome.org/show_bug.cgi?id=324190#c10]:

(yelp:29666): GLib-GObject-WARNING **: gsignal.c:1028: unable to lookup signal
"activate" of unloaded type `MaiAtkObject'

(yelp:29666): GLib-GObject-CRITICAL **: g_signal_emit_valist: assertion
`signal_id > 0' failed

Trace:
#0  IA__g_log (log_domain=0xb71c17bc "GLib-GObject",
    log_level=G_LOG_LEVEL_WARNING,
    format=0xb71c3fd4 "gsignal.c:1028: unable to lookup signal \"%s\" of
unloaded type `%s'") at gmessages.c:516
#1  0xb71ae010 in IA__g_signal_lookup (name=0xb27eacc6 "activate", itype=0)
    at gsignal.c:1027
#2  0xb27be9c9 in nsDocAccessibleWrap::FireToolkitEvent (this=0x879e3e0,
    aEvent=280, aAccessible=0x879e3fc, aEventData=0x0)
    at nsDocAccessibleWrap.cpp:419
#3  0xb5c5a760 in nsWindow::DispatchActivateEvent (this=0x84f0f98)
    at nsWindow.cpp:4303
#4  0xb5c5d8cf in nsWindow::OnButtonPressEvent (this=0x84f0f98,
    aWidget=0x80e2f40, aEvent=0x80bbd50) at nsWindow.cpp:1533
#5  0xb5c5da1f in button_press_event_cb (widget=0x80e2f40, event=0x80bbd50)
    at nsWindow.cpp:3706

The code in question is line 419 from http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsDocAccessibleWrap.cpp#416 
416     case nsIAccessibleEvent::EVENT_ATK_WINDOW_ACTIVATE:
417         MAI_LOG_DEBUG(("\n\nReceived: EVENT_ATK_WINDOW_ACTIVATED\n"));
418         g_signal_emit(accWrap->GetAtkObject(),
419                       g_signal_lookup ("activate", MAI_TYPE_ATK_OBJECT), 0);

With the help of Tim Janik on #gtk+, I've prepared this analysis of the bug:

The "unable to lookup signal "activate" of unloaded type `MaiAtkObject'" warning indicates that while the MaiAtkObject type is already known (i.e. the type registered), the class has not yet been initialised, since no object of this type has been instantiated, nor has the class been initalised in another way (e.g. g_type_class_ref). accWrap->GetAtkObject() creates an object of a type derived from MaiAtkObject, so at first this code seems to guarantee that the class is created by the time we enter g_signal_lookup. But that depends on the arguments of g_signal_emit being evaluated in order. An experimental patch that does not rely on this assumption has been tested by one person that could reproduce the warnings without the patch, and the patch appears to fix the problem.

This code was introduced in bug 235719, so I'm CC:ing some people from that bug.
Question: why does this code use g_signal_lookup + g_signal_emit in the first place, when all the other signal emissions in nsDocAccessibleWrap::FireToolkitEvent use g_signal_emit_by_name ?

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: possible fix

Comment 2

[Ginn Chen]

chpe, can you explain your patch?

Comment 3

[Crispin Flowerday (not receiving bug mail)]

The patch appears to be missing at least the g_signal_emit() in the 'deactivate' case ....

Comment 4

[Ginn Chen]

I built latest yelp with latest firefox trunk, also export GTK_MODULES=:gail:atk-bridge
But I can not reproduce this problem on ubuntu 5.10.
At-poke works fine with yelp.

Comment 5

[Aaron Leventhal]

Comment on attachment 212147 [details] [diff] [review]
possible fix

I don't understand this code well but it looks like you're missing a line after this:
+        AtkObject *accessible = accWrap->GetAtkObject();
+        guint id = g_signal_lookup ("deactivate", MAI_TYPE_ATK_OBJECT);

Shoudln't it then have:
g_signal_emit(accessible, id, 0);

?

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #5)
> (From update of attachment 212147 [details] [diff] [review] [edit])
> I don't understand this code well but it looks like you're missing a line after
> this:
> +        AtkObject *accessible = accWrap->GetAtkObject();
> +        guint id = g_signal_lookup ("deactivate", MAI_TYPE_ATK_OBJECT);
> 
> Shoudln't it then have:
> g_signal_emit(accessible, id, 0);

Yes, sorry. I accidentally dropped that line; will upload a new patch tomorrow.

What I'm wondering is why this code looks up the signal explicitly at all instead of just using g_signal_emit_with_name as with all the other signals in this function.
> ?
> 

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: fixed patch

Comment 8

[Ginn Chen]

Comment on attachment 214080 [details] [diff] [review]
fixed patch

According to http://www.mozilla.org/hacking/mozilla-style-guide.html

We should use
case 1:
    {
      // When you need to declare a variable in a switch, put the block in braces
      int var;
    } break;

Comment 9

[Ginn Chen]

Checking in nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsDocAccessibleWrap.cpp,v  <--  nsDocAccessibleWrap.cpp
new revision: 1.22; previous revision: 1.21
done

Comment 10

[Ginn Chen]

Checking in nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsDocAccessibleWrap.cpp,v  <--  nsDocAccessibleWrap.cpp
new revision: 1.20.4.1; previous revision: 1.20
done

Comment 11

[Ginn Chen]

*** Bug 337429 has been marked as a duplicate of this bug. ***