1802297 - GLib assertions in accessibility cache

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jan Alexander Steffens [:heftig]]

Firefox fails multiple GLib assertions very frequently, at least when starting, opening new tabs and navigating.

[Parent 714131, Main Thread] WARNING: g_signal_emit_by_name: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed: 'glib warning', file /builds/worker/checkouts/gecko/toolkit/xre/nsSigHandlers.cpp:167
(firefox-nightly:714131): GLib-GObject-CRITICAL **: 11:13:03.574: g_signal_emit_by_name: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed

[Parent 714131, Main Thread] WARNING: invalid (NULL) pointer instance: 'glib warning', file /builds/worker/checkouts/gecko/toolkit/xre/nsSigHandlers.cpp:167
(firefox-nightly:714131): GLib-GObject-WARNING **: 11:13:03.672: invalid (NULL) pointer instance

Setting G_DEBUG=fatal-warnings to make the assertions crash the process produced bp-33db1d00-119a-423b-a0cf-9c0e90221124.

Disabling accessibility.cache.enabled removes the warnings.

GNOME 43 (Wayland), Arch Linux.
GTK 3.24.35
GLib 2.74.1

Last good revision: 29c37fecb4ae63a949035f37debcb9e847977ee9
First bad revision: 0c66c3cc26c0fd5b8f3d59060d6c1ad3317cf2fd
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29c37fecb4ae63a949035f37debcb9e847977ee9&tochange=0c66c3cc26c0fd5b8f3d59060d6c1ad3317cf2fd

Regressed by bug 1798621.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1798621

:eeejay, since you are the author of the regressor, bug 1798621, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[James Teh [:Jamie]]

I think this is due to the change in bug 1779156. Before that, we never fired a show event for any accessible in the initial tree push. Now, we fire a show event for any initial cache push. However, the very first initial cache push is for the document itself. Since top level documents have no remote parent, we pass a null parent, resulting in this problem.

The fix is probably to return early if the target is a document just above this.

I can write the patch, but I'm not currently set up to test it.

Comment 3

[James Teh [:Jamie]]

Actually, before bug 1779156, we never fired show events for anything pushed in DoInitialUpdate. Now we fire one for the document plus every direct child. The original patch didn't - it used an explicit argument to suppress it - but it got changed later so that it just uses CacheUpdateType::Initial, which can't distinguish between a DoInitialUpdate and a subsequent insertion.

The question is whether this really matters. On one hand, they really are individual insertions into the tree. On the other hand, this would cause a lot of event spam for documents with a lot of direct children, which could slow down the time it takes for a document to be usable. I'm inclined to think it does matter.

Comment 4

[James Teh [:Jamie]]

We were previously doing something similar and bug 1402951 comment 7 suggests that avoiding firing these events helped performance. Granted, that was on Windows without CTW, but it's still an interesting data point.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1802297: Don't fire show events for the initial a11y tree push.

After bug 1779156, show events were fired from RecvCache, rather than from RecvShowEvent.
This was done to ensure that cache data was available when the event was fired.
However, because RecvCache fired a show event for every initial cache push, this meant that it also fired one for the document itself, plus all the document's initial direct children.
Firing an event for the document caused problems for ATK, since the parent was null for all top level documents.
This also meant we were firing a lot of unnecessary show events, which could be a performance problem for documents with a lot of initial direct children.
To fix this, provide an explicit argument to PDocAccessible::Cache specifying whether to dispatch a show event or not.
This replaces the existing aFinal argument, which was never used.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c350896c890
Don't fire show events for the initial a11y tree push. r=eeejay

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8c350896c890