Bug 265470 – AX: accessing WeakPtr from wrong thread with embedded PDF

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

You can repro with a trivially simple HTML page that embeds a PDF, e.g.:

<object data="hello.pdf" type="application/pdf"  width="300" height="300"></object>

Open the web page and enable VoiceOver, then try to navigate to the embedded PDF object - I get this assertion in debug mode:

ASSERTION FAILED: canSafelyBeUsed()
include/wtf/WeakPtr.h(138) : T *WTF::WeakPtr<WebKit::WebFrame>::operator->() const [T = WebKit::WebFrame, WeakPtrImpl = WTF::DefaultWeakPtrImpl]
1    WTFCrash
2    WTFCrashWithInfo(int, char const*, char const*, int)
3    WTF::WeakPtr<WebKit::WebFrame, WTF::DefaultWeakPtrImpl>::operator->() const
4    WebKit::PDFPlugin::axObjectCache() const
5    -[WKPDFPluginAccessibilityObject parent]
6    -[WKPDFPluginAccessibilityObject accessibilityAttributeValue:]
7    NSAccessibilityGetObjectForAttributeUsingLegacyAPI

[Radar WebKit Bug Importer]

<rdar://problem/118892293>

[Dominic Mazzoni]

Created attachment 468788 [details]
Patch

[chris fleizach]

Comment on attachment 468788 [details]
Patch

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

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:194
> +        callOnMainRunLoopAndWait([&] {

if we don't do this is it a crash?

also returning and storing an ivar (_parent) on a different thread might be bad

[Dominic Mazzoni]

Created attachment 468789 [details]
Patch

[Dominic Mazzoni]

(In reply to chris fleizach from comment #3)
> if we don't do this is it a crash?

It's a debug assertion in WeakPtr: canSafelyBeUsed

> also returning and storing an ivar (_parent) on a different thread might be
> bad

Got it, changed to use a local RetainPtr to move between threads

[Dominic Mazzoni]

Created attachment 468790 [details]
Patch

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #6)
> Created attachment 468790 [details]
> Patch

Use the main thread when accessing the document's
 axObjectCache from WKPDFPluginAccessibilityObject

https://bugs.webkit.org/show_bug.cgi?id=265470
rdar://118894435

Reviewed by NOBODY (OOPS!).

AG: Include here a brief explanation of the problem and solution.

* Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:
(-[WKPDFPluginAccessibilityObject parent]):
(WebKit::PDFPlugin::axObjectCache const):
---
 Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm b/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
index 4fc440c1fe89..fb77b90b95f6 100644
--- a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
+++ b/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
@@ -191,10 +191,14 @@ - (id)initWithPDFPlugin:(WebKit::PDFPlugin *)plugin andElement:(WebCore::HTMLPlu
 - (NSObject *)parent
 {
     if (!_parent) {
-        if (auto* axObjectCache = _pdfPlugin->axObjectCache()) {
-            if (RefPtr pluginAxObject = axObjectCache->getOrCreate(_pluginElement.get()))
-                _parent = pluginAxObject->wrapper();
-        }
+        RetainPtr<NSObject> parent;
+        callOnMainRunLoopAndWait([&] {

AG: I think you need to capture a strong reference to self because there is no guaranty that self is alive when the lambda is invoked on the main thread, similar to what we do in WebAccessibilityObjectWrapper. I.e., something like:

        callOnMainRunLoopAndWait([&parent, protectedSelf = retainPtr(self)] {

and then use protectedSelf to access properties and methods in the lambda.

+            if (auto* axObjectCache = _pdfPlugin->axObjectCache()) {
+                if (RefPtr pluginAxObject = axObjectCache->getOrCreate(_pluginElement.get()))
+                    parent = pluginAxObject->wrapper();
+            }
+        });
+        _parent = parent.get();
     }
     return _parent.get().get();
 }

[Dominic Mazzoni]

Created attachment 468810 [details]
Patch

[Dominic Mazzoni]

(In reply to Andres Gonzalez from comment #7)
> AG: I think you need to capture a strong reference to self because there is
> no guaranty that self is alive when the lambda is invoked on the main
> thread, similar to what we do in WebAccessibilityObjectWrapper. I.e.,

Thanks. Notice that the function accesses an ivar at the bottom - so I
think it's actually necessary to retain self throughout the whole function
scope, not just the lambda. Let me know if you disagree.

[EWS]

Committed 271622@main (0cab9f4dd652): <https://commits.webkit.org/271622@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 468810 [details].