Bug 234737

Summary: [GTK][a11y] Web process crashes in some sites having SVG images
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, aperez, apinheiro, bugs-noreply, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white
Priority: P2 Keywords: Gtk
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=234563
https://bugs.webkit.org/show_bug.cgi?id=233804
Bug Depends on:    
Bug Blocks: 230253, 234740    
Attachments:
Description Flags
Patch
none
Patch aperez: review+

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>