[webkit-changes] [256347] trunk/Source/WebCore

Tue, 11 Feb 2020 13:35:27 -0800

Title: [256347] trunk/Source/WebCore
Revision
256347
Author
andresg...@apple.com
Date
2020-02-11 13:33:41 -0800 (Tue, 11 Feb 2020)

Log Message

Fix for crashes in WebAccessibilityObjectWrapper after notification updates in IsolatedTree mode.
https://bugs.webkit.org/show_bug.cgi?id=207558

Reviewed by Chris Fleizach.

- Accessibility methods invoked in the secondary thread that Return id
values retrieved from the main thread, need to retain/autorelease the
returned ids.
- When serving a request on the AX thread that requires retrieving a
value from the main thread, the backing obbject on the main thread may
have gone away, so need to check for nullity of the backing object also
on the main thread.

* accessibility/AccessibilityObjectInterface.h:
(WebCore::Accessibility::retrieveAutoreleasedValueFromMainThread):
* accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
(-[WebAccessibilityObjectWrapper attachmentView]):
(-[WebAccessibilityObjectWrapper textMarkerRangeFromRange:]):
(-[WebAccessibilityObjectWrapper renderWidgetChildren]):
(-[WebAccessibilityObjectWrapper associatedPluginParent]):
(-[WebAccessibilityObjectWrapper scrollViewParent]):
(-[WebAccessibilityObjectWrapper windowElement:]):
(-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (256346 => 256347)


--- trunk/Source/WebCore/ChangeLog	2020-02-11 21:14:44 UTC (rev 256346)
+++ trunk/Source/WebCore/ChangeLog	2020-02-11 21:33:41 UTC (rev 256347)
@@ -1,3 +1,29 @@
+2020-02-11  Andres Gonzalez  <andresg...@apple.com>
+
+        Fix for crashes in WebAccessibilityObjectWrapper after notification updates in IsolatedTree mode.
+        https://bugs.webkit.org/show_bug.cgi?id=207558
+
+        Reviewed by Chris Fleizach.
+
+        - Accessibility methods invoked in the secondary thread that Return id
+        values retrieved from the main thread, need to retain/autorelease the
+        returned ids.
+        - When serving a request on the AX thread that requires retrieving a
+        value from the main thread, the backing obbject on the main thread may
+        have gone away, so need to check for nullity of the backing object also
+        on the main thread.
+
+        * accessibility/AccessibilityObjectInterface.h:
+        (WebCore::Accessibility::retrieveAutoreleasedValueFromMainThread):
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
+        (-[WebAccessibilityObjectWrapper attachmentView]):
+        (-[WebAccessibilityObjectWrapper textMarkerRangeFromRange:]):
+        (-[WebAccessibilityObjectWrapper renderWidgetChildren]):
+        (-[WebAccessibilityObjectWrapper associatedPluginParent]):
+        (-[WebAccessibilityObjectWrapper scrollViewParent]):
+        (-[WebAccessibilityObjectWrapper windowElement:]):
+        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]):
+
 2020-02-11  Zalan Bujtas  <za...@apple.com>
 
         [LFC] Introduce Layout::LineBreakBox

Modified: trunk/Source/WebCore/accessibility/AccessibilityObjectInterface.h (256346 => 256347)


--- trunk/Source/WebCore/accessibility/AccessibilityObjectInterface.h	2020-02-11 21:14:44 UTC (rev 256346)
+++ trunk/Source/WebCore/accessibility/AccessibilityObjectInterface.h	2020-02-11 21:33:41 UTC (rev 256347)
@@ -1207,6 +1207,20 @@
     return value;
 }
 
+#if PLATFORM(COCOA)
+template<typename T, typename U> inline T retrieveAutoreleasedValueFromMainThread(U&& lambda)
+{
+    if (isMainThread())
+        return lambda().autorelease();
+
+    RetainPtr<T> value;
+    callOnMainThreadAndWait([&value, &lambda] {
+        value = lambda();
+    });
+    return value.autorelease();
+}
+#endif
+
 } // namespace Accessibility
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm (256346 => 256347)


--- trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm	2020-02-11 21:14:44 UTC (rev 256346)
+++ trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm	2020-02-11 21:33:41 UTC (rev 256347)
@@ -557,7 +557,7 @@
 {
     ASSERT(self.axBackingObject->isAttachment());
 
-    return Accessibility::retrieveValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+    return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
         auto* widget = protectedSelf.get().axBackingObject->widgetForAttachmentView();
         if (!widget)
             return nil;
@@ -799,7 +799,9 @@
 
 - (id)textMarkerRangeFromRange:(const RefPtr<Range>)range
 {
-    return textMarkerRangeFromRange(self.axBackingObject->axObjectCache(), range);
+    if (auto* backingObject = self.axBackingObject)
+        return textMarkerRangeFromRange(backingObject->axObjectCache(), range);
+    return nil;
 }
 
 static id textMarkerRangeFromRange(AXObjectCache* cache, const RefPtr<Range> range)
@@ -1895,7 +1897,11 @@
 - (NSArray*)renderWidgetChildren
 {
     return Accessibility::retrieveValueFromMainThread<NSArray *>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> NSArray * {
-        Widget* widget = protectedSelf.get().axBackingObject->widget();
+        auto* backingObject = protectedSelf.get().axBackingObject;
+        if (!backingObject)
+            return nil;
+
+        Widget* widget = backingObject->widget();
         if (!widget)
             return nil;
         ALLOW_DEPRECATED_DECLARATIONS_BEGIN
@@ -1947,7 +1953,7 @@
 
 - (id)associatedPluginParent
 {
-    return Accessibility::retrieveValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+    return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
         if (!protectedSelf.get().axBackingObject || !protectedSelf.get().axBackingObject->hasApplePDFAnnotationAttribute())
             return nil;
     
@@ -2298,7 +2304,7 @@
 
 - (id)scrollViewParent
 {
-    return Accessibility::retrieveValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+    return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
         if (!is<AccessibilityScrollView>(protectedSelf.get().axBackingObject))
             return nil;
 
@@ -2343,7 +2349,7 @@
 
 - (id)windowElement:(NSString*)attributeName
 {
-    return Accessibility::retrieveValueFromMainThread<id>([attributeName, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+    return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([attributeName, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
         id remoteParent = [protectedSelf remoteAccessibilityParentObject];
         if (remoteParent) {
             ALLOW_DEPRECATED_DECLARATIONS_BEGIN
@@ -2351,7 +2357,11 @@
             ALLOW_DEPRECATED_DECLARATIONS_END
         }
 
-        if (auto* fv = protectedSelf.get().axBackingObject->documentFrameView())
+        auto* backingObject = protectedSelf.get().axBackingObject;
+        if (!backingObject)
+            return nil;
+
+        if (auto* fv = backingObject->documentFrameView())
             return [fv->platformWidget() window];
 
         return nil;
@@ -3957,7 +3967,7 @@
     }
 
     if ([attribute isEqualToString:NSAccessibilityEndTextMarkerForBoundsParameterizedAttribute]) {
-        return Accessibility::retrieveValueFromMainThread<id>([&rect, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&rect, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -3969,7 +3979,7 @@
     }
 
     if ([attribute isEqualToString:NSAccessibilityStartTextMarkerForBoundsParameterizedAttribute]) {
-        return Accessibility::retrieveValueFromMainThread<id>([&rect, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&rect, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4014,7 +4024,7 @@
     }
 
     if ([attribute isEqualToString:@"AXTextMarkerRangeForUIElement"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([&uiElement, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&uiElement, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             RefPtr<Range> range = uiElement.get()->elementRange();
             return [protectedSelf textMarkerRangeFromRange:range];
         });
@@ -4042,7 +4052,7 @@
         if (!pointSet)
             return nil;
 
-        return Accessibility::retrieveValueFromMainThread<id>([&webCorePoint, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&webCorePoint, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4061,17 +4071,21 @@
 
     if ([attribute isEqualToString:NSAccessibilityBoundsForRangeParameterizedAttribute]) {
         NSRect rect = Accessibility::retrieveValueFromMainThread<NSRect>([&range, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> NSRect {
-            auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
+            auto* backingObject = protectedSelf.get().axBackingObject;
+            if (!backingObject)
+                return CGRectZero;
+
+            auto* cache = backingObject->axObjectCache();
             if (!cache)
                 return CGRectZero;
 
-            CharacterOffset start = cache->characterOffsetForIndex(range.location, protectedSelf.get().axBackingObject);
-            CharacterOffset end = cache->characterOffsetForIndex(range.location+range.length, protectedSelf.get().axBackingObject);
+            CharacterOffset start = cache->characterOffsetForIndex(range.location, backingObject);
+            CharacterOffset end = cache->characterOffsetForIndex(range.location+range.length, backingObject);
             if (start.isNull() || end.isNull())
                 return CGRectZero;
 
             RefPtr<Range> range = cache->rangeForUnorderedCharacterOffsets(start, end);
-            auto bounds = FloatRect(protectedSelf.get().axBackingObject->boundsForRange(range));
+            auto bounds = FloatRect(backingObject->boundsForRange(range));
             return [protectedSelf convertRectToSpace:bounds space:AccessibilityConversionSpace::Screen];
         });
         return [NSValue valueWithRect:rect];
@@ -4127,7 +4141,7 @@
         if (!AXObjectIsTextMarker(textMarker1) || !AXObjectIsTextMarker(textMarker2))
             return nil;
 
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker1, textMarker2, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker1, textMarker2, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4150,7 +4164,7 @@
     }
 
     if ([attribute isEqualToString:@"AXLeftWordTextMarkerRangeForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4162,7 +4176,7 @@
     }
 
     if ([attribute isEqualToString:@"AXRightWordTextMarkerRangeForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4186,7 +4200,7 @@
     }
 
     if ([attribute isEqualToString:@"AXSentenceTextMarkerRangeForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4198,7 +4212,7 @@
     }
 
     if ([attribute isEqualToString:@"AXParagraphTextMarkerRangeForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4210,7 +4224,7 @@
     }
 
     if ([attribute isEqualToString:@"AXNextWordEndTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4222,7 +4236,7 @@
     }
 
     if ([attribute isEqualToString:@"AXPreviousWordStartTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4244,7 +4258,7 @@
     }
     
     if ([attribute isEqualToString:@"AXNextSentenceEndTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4256,7 +4270,7 @@
     }
 
     if ([attribute isEqualToString:@"AXPreviousSentenceStartTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4268,7 +4282,7 @@
     }
 
     if ([attribute isEqualToString:@"AXNextParagraphEndTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;
@@ -4280,7 +4294,7 @@
     }
 
     if ([attribute isEqualToString:@"AXPreviousParagraphStartTextMarkerForTextMarker"]) {
-        return Accessibility::retrieveValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> id {
+        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker, protectedSelf = RetainPtr<WebAccessibilityObjectWrapper>(self)] () -> RetainPtr<id> {
             auto* cache = protectedSelf.get().axBackingObject->axObjectCache();
             if (!cache)
                 return nil;

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to