[GTK][a11y] Web process crashes in some sites having SVG images

https://bugs.webkit.org/show_bug.cgi?id=234737

Reviewed by Adrian Perez de Castro.

Source/WebCore:

Unfortunately the changes in r287388 are not enough, it can still happen that root hasn't been set to the
SVGImage page when the wrappers are created. So, we can't actually create the wrappers with a reference to the
root object as we did in r286767. In most of the cases wrappers use the root just to get AccessibilityAtspi,
which is not a singleton, but it's created and owned by the web process singleton at startup, so it can be
accessed globaly. This patch makes AccessibilityAtspi a singleton to get the global instance without having to
keep a reference in the root object.

* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::remoteSVGRootElement const): Move the code to set the root in SVGImage page
before the remote svg wrapper is created.
* accessibility/atspi/AXObjectCacheAtspi.cpp:
(WebCore::AXObjectCache::attachWrapper): Just pass the page root object to the wrapper constructor, that now
receives a pointer that might be nullptr.
* accessibility/atspi/AccessibilityAtspi.cpp:
(WebCore::AccessibilityAtspi::AccessibilityAtspi): Just create the WorkQueue.
(WebCore::AccessibilityAtspi::singleton): Return a reference to the global instance.
(WebCore::AccessibilityAtspi::connect): Connect to the given dbus address.
(WebCore::AccessibilityAtspi::applicationReference const): This allows wrappers to get the application reference
without having to check if m_root is nullptr or not.
(WebCore::AccessibilityAtspi::parentChanged): Use AccessibilityObjectAtspi::isTreeRegistered()
(WebCore::AccessibilityAtspi::childrenChanged): Ditto.
* accessibility/atspi/AccessibilityAtspi.h:
* accessibility/atspi/AccessibilityObjectAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::create): Receive a AccessibilityRootAtspi pointer instead of reference.
(WebCore::AccessibilityObjectAtspi::AccessibilityObjectAtspi): Ditto.
(WebCore::AccessibilityObjectAtspi::cacheDestroyed): Only call AccessibilityRootAtspi::childRemoved if parent is
the root object.
(WebCore::AccessibilityObjectAtspi::elementDestroyed): Null check m_root and use AccessibilityAtspi::singleton().
(WebCore::AccessibilityObjectAtspi::root): Get the root if already set or try to get it from the page.
(WebCore::AccessibilityObjectAtspi::isTreeRegistered const): Return try if root has been set and its tree has
been registered.
(WebCore::AccessibilityObjectAtspi::registerObject): Use AccessibilityObjectAtspi::root().
(WebCore::AccessibilityObjectAtspi::reference): Use AccessibilityAtspi::singleton().
(WebCore::AccessibilityObjectAtspi::hyperlinkReference): Ditto.
(WebCore::AccessibilityObjectAtspi::setParent): Return early also if root hasn't been set yet.
(WebCore::AccessibilityObjectAtspi::parentReference const): Use AccessibilityAtspi::singleton().
(WebCore::AccessibilityObjectAtspi::serialize const): Ditto.
(WebCore::AccessibilityObjectAtspi::childAdded): Ditto.
(WebCore::AccessibilityObjectAtspi::childRemoved): Ditto.
(WebCore::AccessibilityObjectAtspi::stateChanged): Ditto.
(WebCore::AccessibilityObjectAtspi::loadEvent): Ditto.
* accessibility/atspi/AccessibilityObjectAtspi.h:
* accessibility/atspi/AccessibilityObjectComponentAtspi.cpp:
* accessibility/atspi/AccessibilityObjectHyperlinkAtspi.cpp:
* accessibility/atspi/AccessibilityObjectHypertextAtspi.cpp:
* accessibility/atspi/AccessibilityObjectSelectionAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::selectionChanged): Ditto.
* accessibility/atspi/AccessibilityObjectTableAtspi.cpp:
* accessibility/atspi/AccessibilityObjectTableCellAtspi.cpp:
* accessibility/atspi/AccessibilityObjectTextAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::textInserted): Ditto.
(WebCore::AccessibilityObjectAtspi::textDeleted): Ditto.
(WebCore::AccessibilityObjectAtspi::selectionChanged): Ditto.
(WebCore::AccessibilityObjectAtspi::textAttributesChanged): Ditto.
* accessibility/atspi/AccessibilityObjectValueAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::valueChanged): Ditto.
* accessibility/atspi/AccessibilityRootAtspi.cpp:
(WebCore::AccessibilityRootAtspi::create): Remove AccessibilityAtspi parameter.
(WebCore::AccessibilityRootAtspi::AccessibilityRootAtspi): Ditto.
(WebCore::AccessibilityRootAtspi::registerObject): Use AccessibilityAtspi::singleton().
(WebCore::AccessibilityRootAtspi::unregisterObject): Ditto
(WebCore::AccessibilityRootAtspi::embedded): Ditto.
(WebCore::AccessibilityRootAtspi::applicationReference const): Ditto.
(WebCore::AccessibilityRootAtspi::reference const): Ditto.
(WebCore::AccessibilityRootAtspi::childAdded): Ditto.
(WebCore::AccessibilityRootAtspi::childRemoved): Ditto.
(WebCore::AccessibilityRootAtspi::serialize const): Ditto.
* accessibility/atspi/AccessibilityRootAtspi.h:

Source/WebKit:

Use AccessibilityAtspi::singleton().

* WebProcess/WebPage/gtk/WebPageGtk.cpp:
(WebKit::WebPage::platformInitialize):
* WebProcess/WebProcess.h:
(WebKit::WebProcess::accessibilityAtspi const): Deleted.
* WebProcess/glib/WebProcessGLib.cpp:
(WebKit::WebProcess::platformInitializeWebProcess):

Tools:

Use AccessibilityAtspi::singleton().

* WebKitTestRunner/InjectedBundle/atspi/AccessibilityControllerAtspi.cpp:
(WTR::AccessibilityController::axRunLoop):
* WebKitTestRunner/InjectedBundle/atspi/AccessibilityNotificationHandler.cpp:
(WTR::AccessibilityNotificationHandler::AccessibilityNotificationHandler):
(WTR::AccessibilityNotificationHandler::~AccessibilityNotificationHandler):


Canonical link: https://commits.webkit.org/245920@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@287876 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,79 @@
+2022-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK][a11y] Web process crashes in some sites having SVG images
+        https://bugs.webkit.org/show_bug.cgi?id=234737
+
+        Reviewed by Adrian Perez de Castro.
+
+        Unfortunately the changes in r287388 are not enough, it can still happen that root hasn't been set to the
+        SVGImage page when the wrappers are created. So, we can't actually create the wrappers with a reference to the
+        root object as we did in r286767. In most of the cases wrappers use the root just to get AccessibilityAtspi,
+        which is not a singleton, but it's created and owned by the web process singleton at startup, so it can be
+        accessed globaly. This patch makes AccessibilityAtspi a singleton to get the global instance without having to
+        keep a reference in the root object.
+
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::remoteSVGRootElement const): Move the code to set the root in SVGImage page
+        before the remote svg wrapper is created.
+        * accessibility/atspi/AXObjectCacheAtspi.cpp:
+        (WebCore::AXObjectCache::attachWrapper): Just pass the page root object to the wrapper constructor, that now
+        receives a pointer that might be nullptr.
+        * accessibility/atspi/AccessibilityAtspi.cpp:
+        (WebCore::AccessibilityAtspi::AccessibilityAtspi): Just create the WorkQueue.
+        (WebCore::AccessibilityAtspi::singleton): Return a reference to the global instance.
+        (WebCore::AccessibilityAtspi::connect): Connect to the given dbus address.
+        (WebCore::AccessibilityAtspi::applicationReference const): This allows wrappers to get the application reference
+        without having to check if m_root is nullptr or not.
+        (WebCore::AccessibilityAtspi::parentChanged): Use AccessibilityObjectAtspi::isTreeRegistered()
+        (WebCore::AccessibilityAtspi::childrenChanged): Ditto.
+        * accessibility/atspi/AccessibilityAtspi.h:
+        * accessibility/atspi/AccessibilityObjectAtspi.cpp:
+        (WebCore::AccessibilityObjectAtspi::create): Receive a AccessibilityRootAtspi pointer instead of reference.
+        (WebCore::AccessibilityObjectAtspi::AccessibilityObjectAtspi): Ditto.
+        (WebCore::AccessibilityObjectAtspi::cacheDestroyed): Only call AccessibilityRootAtspi::childRemoved if parent is
+        the root object.
+        (WebCore::AccessibilityObjectAtspi::elementDestroyed): Null check m_root and use AccessibilityAtspi::singleton().
+        (WebCore::AccessibilityObjectAtspi::root): Get the root if already set or try to get it from the page.
+        (WebCore::AccessibilityObjectAtspi::isTreeRegistered const): Return try if root has been set and its tree has
+        been registered.
+        (WebCore::AccessibilityObjectAtspi::registerObject): Use AccessibilityObjectAtspi::root().
+        (WebCore::AccessibilityObjectAtspi::reference): Use AccessibilityAtspi::singleton().
+        (WebCore::AccessibilityObjectAtspi::hyperlinkReference): Ditto.
+        (WebCore::AccessibilityObjectAtspi::setParent): Return early also if root hasn't been set yet.
+        (WebCore::AccessibilityObjectAtspi::parentReference const): Use AccessibilityAtspi::singleton().
+        (WebCore::AccessibilityObjectAtspi::serialize const): Ditto.
+        (WebCore::AccessibilityObjectAtspi::childAdded): Ditto.
+        (WebCore::AccessibilityObjectAtspi::childRemoved): Ditto.
+        (WebCore::AccessibilityObjectAtspi::stateChanged): Ditto.
+        (WebCore::AccessibilityObjectAtspi::loadEvent): Ditto.
+        * accessibility/atspi/AccessibilityObjectAtspi.h:
+        * accessibility/atspi/AccessibilityObjectComponentAtspi.cpp:
+        * accessibility/atspi/AccessibilityObjectHyperlinkAtspi.cpp:
+        * accessibility/atspi/AccessibilityObjectHypertextAtspi.cpp:
+        * accessibility/atspi/AccessibilityObjectSelectionAtspi.cpp:
+        (WebCore::AccessibilityObjectAtspi::selectionChanged): Ditto.
+        * accessibility/atspi/AccessibilityObjectTableAtspi.cpp:
+        * accessibility/atspi/AccessibilityObjectTableCellAtspi.cpp:
+        * accessibility/atspi/AccessibilityObjectTextAtspi.cpp:
+        (WebCore::AccessibilityObjectAtspi::textInserted): Ditto.
+        (WebCore::AccessibilityObjectAtspi::textDeleted): Ditto.
+        (WebCore::AccessibilityObjectAtspi::selectionChanged): Ditto.
+        (WebCore::AccessibilityObjectAtspi::textAttributesChanged): Ditto.
+        * accessibility/atspi/AccessibilityObjectValueAtspi.cpp:
+        (WebCore::AccessibilityObjectAtspi::valueChanged): Ditto.
+        * accessibility/atspi/AccessibilityRootAtspi.cpp:
+        (WebCore::AccessibilityRootAtspi::create): Remove AccessibilityAtspi parameter.
+        (WebCore::AccessibilityRootAtspi::AccessibilityRootAtspi): Ditto.
+        (WebCore::AccessibilityRootAtspi::registerObject): Use AccessibilityAtspi::singleton().
+        (WebCore::AccessibilityRootAtspi::unregisterObject): Ditto
+        (WebCore::AccessibilityRootAtspi::embedded): Ditto.
+        (WebCore::AccessibilityRootAtspi::applicationReference const): Ditto.
+        (WebCore::AccessibilityRootAtspi::reference const): Ditto.
+        (WebCore::AccessibilityRootAtspi::childAdded): Ditto.
+        (WebCore::AccessibilityRootAtspi::childRemoved): Ditto.
+        (WebCore::AccessibilityRootAtspi::serialize const): Ditto.
+        * accessibility/atspi/AccessibilityRootAtspi.h:
+
 2022-01-11  Martin Robinson  <mrobinson@webkit.org>
 
         Some css-transforms tests assert in debug

Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -3402,17 +3402,18 @@ AccessibilitySVGRoot* AccessibilityRenderObject::remoteSVGRootElement(CreationCh
     AXObjectCache* cache = frame.document()->axObjectCache();
     if (!cache)
         return nullptr;
-    AccessibilityObject* rootSVGObject = createIfNecessary == Create ? cache->getOrCreate(rendererRoot) : cache->get(rendererRoot);
-
-    ASSERT(!createIfNecessary || rootSVGObject);
-    if (!is<AccessibilitySVGRoot>(rootSVGObject))
-        return nullptr;
 
 #if USE(ATSPI)
     if (auto* page = document->page())
         page->setAccessibilityRootObject(createIfNecessary == Create ? axObjectCache()->document().page()->accessibilityRootObject() : nullptr);
 #endif
 
+    AccessibilityObject* rootSVGObject = createIfNecessary == Create ? cache->getOrCreate(rendererRoot) : cache->get(rendererRoot);
+
+    ASSERT(!createIfNecessary || rootSVGObject);
+    if (!is<AccessibilitySVGRoot>(rootSVGObject))
+        return nullptr;
+
     return downcast<AccessibilitySVGRoot>(rootSVGObject);
 }
 

Source/WebCore/accessibility/atspi/AXObjectCacheAtspi.cpp

@@ -35,11 +35,7 @@ namespace WebCore {
 
 void AXObjectCache::attachWrapper(AXCoreObject* axObject)
 {
-    auto* rootWrapper = document().page()->accessibilityRootObject();
-    if (!rootWrapper)
-        return;
-
-    auto wrapper = AccessibilityObjectAtspi::create(axObject, *rootWrapper);
+    auto wrapper = AccessibilityObjectAtspi::create(axObject, document().page()->accessibilityRootObject());
     axObject->setWrapper(wrapper.ptr());
 
     m_deferredParentChangedList.add(axObject);

Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp

@@ -26,18 +26,31 @@
 #include <gio/gio.h>
 #include <glib/gi18n-lib.h>
 #include <wtf/MainThread.h>
+#include <wtf/NeverDestroyed.h>
 #include <wtf/SetForScope.h>
 #include <wtf/SortedArrayMap.h>
 #include <wtf/UUID.h>
 
 namespace WebCore {
 
-AccessibilityAtspi::AccessibilityAtspi(const String& busAddress)
+AccessibilityAtspi& AccessibilityAtspi::singleton()
+{
+    static NeverDestroyed<AccessibilityAtspi> atspi;
+    return atspi;
+}
+
+AccessibilityAtspi::AccessibilityAtspi()
     : m_queue(WorkQueue::create("org.webkit.a11y"))
+{
+    RELEASE_ASSERT(isMainThread());
+}
+
+void AccessibilityAtspi::connect(const String& busAddress)
 {
     RELEASE_ASSERT(isMainThread());
     if (busAddress.isEmpty())
         return;
+
     m_queue->dispatch([this, busAddress = busAddress.isolatedCopy()] {
         GUniqueOutPtr<GError> error;
         m_connection = adoptGRef(g_dbus_connection_new_for_address_sync(busAddress.utf8().data(),
@@ -51,12 +64,6 @@ AccessibilityAtspi::AccessibilityAtspi(const String& busAddress)
     });
 }
 
-AccessibilityAtspi::~AccessibilityAtspi()
-{
-    if (m_registry)
-        g_signal_handlers_disconnect_by_data(m_registry.get(), this);
-}
-
 void AccessibilityAtspi::registerTrees() const
 {
     RELEASE_ASSERT(!isMainThread());
@@ -218,6 +225,19 @@ GVariant* AccessibilityAtspi::nullReference() const
     return g_variant_new("(so)", uniqueName(), "/org/a11y/atspi/null");
 }
 
+GVariant* AccessibilityAtspi::applicationReference() const
+{
+    RELEASE_ASSERT(!isMainThread());
+
+    // The application is the same for all root objects, so just use the first root object that is already embedded.
+    for (auto* rootObject : m_rootObjects.keys()) {
+        if (!rootObject->path().isNull())
+            return rootObject->applicationReference();
+    }
+
+    return nullReference();
+}
+
 void AccessibilityAtspi::registerRoot(AccessibilityRootAtspi& rootObject, Vector<std::pair<GDBusInterfaceInfo*, GDBusInterfaceVTable*>>&& interfaces, CompletionHandler<void(const String&)>&& completionHandler)
 {
     RELEASE_ASSERT(isMainThread());
@@ -329,7 +349,7 @@ void AccessibilityAtspi::parentChanged(AccessibilityObjectAtspi& atspiObject)
             return;
 
         // Always emit parentChanged when the tree is registered because the atspi cache always consumes it.
-        if (!atspiObject->root().isTreeRegistered())
+        if (!atspiObject->isTreeRegistered())
             return;
 
         // Emit parentChanged only if the object is already registered, otherwise register the object,
@@ -365,7 +385,7 @@ void AccessibilityAtspi::childrenChanged(AccessibilityObjectAtspi& atspiObject,
             return;
 
         // Always emit ChildrenChanged when the tree is registered because the atspi cache always consumes it.
-        if (!atspiObject->root().isTreeRegistered())
+        if (!atspiObject->isTreeRegistered())
             return;
 
         g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject->path().utf8().data(), "org.a11y.atspi.Event.Object", "ChildrenChanged",

Source/WebCore/accessibility/atspi/AccessibilityAtspi.h

@@ -40,15 +40,18 @@ class AccessibilityRootAtspi;
 enum class AccessibilityRole;
 
 class AccessibilityAtspi {
-    WTF_MAKE_FAST_ALLOCATED;
+    WTF_MAKE_NONCOPYABLE(AccessibilityAtspi); WTF_MAKE_FAST_ALLOCATED;
+    friend NeverDestroyed<AccessibilityAtspi>;
 public:
-    AccessibilityAtspi(const String&);
-    ~AccessibilityAtspi();
+    WEBCORE_EXPORT static AccessibilityAtspi& singleton();
+
+    void connect(const String&);
 
     WEBCORE_EXPORT RunLoop& runLoop() const;
 
     const char* uniqueName() const;
     GVariant* nullReference() const;
+    GVariant* applicationReference() const;
     bool hasEventListeners() const { return !m_eventListeners.isEmpty(); }
 
     void registerRoot(AccessibilityRootAtspi&, Vector<std::pair<GDBusInterfaceInfo*, GDBusInterfaceVTable*>>&&, CompletionHandler<void(const String&)>&&);
@@ -88,6 +91,8 @@ class AccessibilityAtspi {
 #endif
 
 private:
+    AccessibilityAtspi();
+
     void registerTrees() const;
     void initializeRegistry();
     void addEventListener(const char* dbusName, const char* eventName);