Bug 234737 - [GTK][a11y] Web process crashes in some sites having SVG images
Summary: [GTK][a11y] Web process crashes in some sites having SVG images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: ATSPI 234740
  Show dependency treegraph
 
Reported: 2021-12-28 23:37 PST by Carlos Garcia Campos
Modified: 2022-01-11 06:07 PST (History)
11 users (show)

See Also:


Attachments
Patch (49.49 KB, patch)
2021-12-29 00:36 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (52.42 KB, patch)
2022-01-11 02:52 PST, Carlos Garcia Campos
aperez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2021-12-28 23:37:09 PST
Changes in r287388 are not enough, it can still happen that root hasn't been set to the SVGImage page when the wrapper are created. So, we can't actually create the wrappers with a reference to the root object as we did in r286767
Comment 1 Carlos Garcia Campos 2021-12-29 00:36:13 PST
Created attachment 448066 [details]
Patch
Comment 2 Adrian Perez 2022-01-10 07:37:37 PST
Comment on attachment 448066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448066&action=review

> Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:35
> +static AccessibilityAtspi* s_currentAtspi;

This smells like we may want to use “NeverDestroyed<AccessibilityAtspi>“ and provide instead a
“::singleton()“ method. As far as I find from the code, only the WebProcess will have an instance
(and only one) of this, which gets created in “WebProcess::platformInitializeWebProcess()”. Then
we could ditch the “WebProcess::m_accessibility” member and the “WebProcess::accessibilityAtspi()“
accessor — then “WebPageGtk.cpp“ can use “AccessibilityAtspi::singleton()”, and that seems the
only place where the accessor is ever used.

Of course we would still something like an “AccessibilityAtspi::initialize(const String& busAddress)”
static method to configure the single instance from “::platformInitializeWebProcess()”.

Thoughts?
Comment 3 Carlos Garcia Campos 2022-01-11 02:52:18 PST
Created attachment 448835 [details]
Patch
Comment 4 Adrian Perez 2022-01-11 03:38:49 PST
Comment on attachment 448835 [details]
Patch

Thanks for the updated patch! I think it looks clearer now and I like
it that now the fact that AccessibilityAtspi is a single instance is
explicitly stated in the code :)
Comment 5 Carlos Garcia Campos 2022-01-11 06:07:49 PST
Committed r287876 (245920@trunk): <https://commits.webkit.org/245920@trunk>