Bug 265470

<rdar://problem/118892293>

Created attachment 468788 [details]
Patch

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

Created attachment 468789 [details]
Patch

(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

Created attachment 468790 [details]
Patch

(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();
 }

Created attachment 468810 [details]
Patch

(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.

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].