PDFPlugin hangs in deallocation when data load contends with waiting …

…for thread completion

https://bugs.webkit.org/show_bug.cgi?id=246454
rdar://101147811

Reviewed by Tim Horton.

When `PDFPlugin::teardown()` would get called (on the main thread), it would
wait for the PDF thread to complete. However, if loading is still going on,
the PDF thread might be blocked on `dataSemaphore.wait()` in
PDFIncrementalLoader::dataProviderGetBytesAtPosition(). The `dataSemaphore`
is supposed to get signaled on a task dispatched to the main thread but this
task won't run if the main thread is blocked on waiting for the thread to
exit.

To address the issue, PDFIncrementalLoader::clear() now signals the
pending dataSemaphores before waiting for the thread to exit and sets a
flag indicating we want the thread to exit. This flag is then checked
on the PDF thread to make sure we don't queue additional work / semaphores.

* LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction-expected.txt: Added.
* LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction.html: Added.
I took the test from Erica Li's earlier PR that got reverted.

* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h:
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::create):
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::wait):
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::signal):
(WebKit::PDFIncrementalLoader::WTF_GUARDED_BY_LOCK):
* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm:
(WebKit::PDFIncrementalLoader::clear):
(WebKit::PDFIncrementalLoader::dataProviderGetBytesAtPosition):
(WebKit::PDFIncrementalLoader::dataProviderGetByteRanges):
(WebKit::PDFIncrementalLoader::threadEntry):

Canonical link: https://commits.webkit.org/275707@main

LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction-expected.txt

@@ -0,0 +1,3 @@
+This test passes if it does not timeout.
+
+

LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction.html

@@ -0,0 +1,17 @@
+<script>
+  if (window.testRunner)
+    testRunner.dumpAsText();
+  onload = async () => {
+    let iframe0 = document.createElement('iframe');
+    iframe0.src = 'data:application/pdf,x';
+    document.body.append(iframe0);
+
+    await caches.has('a');
+    await caches.has('a');
+
+    $vm.print('before');
+    iframe0.remove();
+    $vm.print('after');
+  };
+</script>
+<p>This test passes if it does not timeout.</p>

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h

@@ -29,8 +29,10 @@
 
 #include <wtf/Ref.h>
 #include <wtf/ThreadSafeRefCounted.h>
+#include <wtf/ThreadSafeWeakHashSet.h>
 #include <wtf/ThreadSafeWeakPtr.h>
 #include <wtf/Threading.h>
+#include <wtf/threads/BinarySemaphore.h>
 
 OBJC_CLASS PDFDocument;
 
@@ -114,6 +116,27 @@ class PDFIncrementalLoader : public ThreadSafeRefCountedAndCanMakeThreadSafeWeak
     void logStreamLoader(WTF::TextStream&, WebCore::NetscapePlugInStreamLoader&);
 #endif
 
+    class SemaphoreWrapper : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<SemaphoreWrapper> {
+    public:
+        static Ref<SemaphoreWrapper> create() { return adoptRef(*new SemaphoreWrapper); }
+
+        void wait() { m_semaphore.wait(); }
+        void signal()
+        {
+            m_wasSignaled = true;
+            m_semaphore.signal();
+        }
+        bool wasSignaled() const { return m_wasSignaled; }
+
+    private:
+        SemaphoreWrapper() = default;
+
+        BinarySemaphore m_semaphore;
+        std::atomic<bool> m_wasSignaled { false };
+    };
+
+    RefPtr<SemaphoreWrapper> createDataSemaphore();
+
     ThreadSafeWeakPtr<PDFPluginBase> m_plugin;
 
     RetainPtr<PDFDocument> m_backgroundThreadDocument;
@@ -124,6 +147,11 @@ class PDFIncrementalLoader : public ThreadSafeRefCountedAndCanMakeThreadSafeWeak
     struct RequestData;
     std::unique_ptr<RequestData> m_requestData;
 
+    ThreadSafeWeakHashSet<SemaphoreWrapper> m_dataSemaphores WTF_GUARDED_BY_LOCK(m_wasPDFThreadTerminationRequestedLock);
+
+    Lock m_wasPDFThreadTerminationRequestedLock;
+    bool m_wasPDFThreadTerminationRequested WTF_GUARDED_BY_LOCK(m_wasPDFThreadTerminationRequestedLock) { false };
+
 #if !LOG_DISABLED
     std::atomic<size_t> m_threadsWaitingOnCallback { 0 };
     std::atomic<size_t> m_completedRangeRequests { 0 };

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm

@@ -34,10 +34,10 @@
 #import <WebCore/HTTPStatusCodes.h>
 #import <WebCore/NetscapePlugInStreamLoader.h>
 #import <pal/spi/cg/CoreGraphicsSPI.h>
+#import <wtf/CallbackAggregator.h>
 #import <wtf/Identified.h>
 #import <wtf/Range.h>
 #import <wtf/RangeSet.h>
-#import <wtf/WTFSemaphore.h>
 
 #import "PDFKitSoftLink.h"
 
@@ -294,6 +294,13 @@
     // we can force the PDFThread to complete quickly
     if (m_pdfThread) {
         unconditionalCompleteOutstandingRangeRequests();
+        {
+            Locker locker { m_wasPDFThreadTerminationRequestedLock };
+            m_wasPDFThreadTerminationRequested = true;
+            m_dataSemaphores.forEach([](auto& dataSemaphore) {
+                dataSemaphore.signal();
+            });
+        }
         m_pdfThread->waitForCompletion();
     }
 }
@@ -649,19 +656,26 @@ static void dataProviderReleaseInfoCallback(void* info)
         return 0;
     }
 
-    WTF::Semaphore dataSemaphore { 0 };
+    RefPtr dataSemaphore = createDataSemaphore();
+    if (!dataSemaphore)
+        return 0;
+
     size_t bytesProvided = 0;
 
-    RunLoop::main().dispatch([protectedLoader = Ref { *this }, position, count, buffer, &dataSemaphore, &bytesProvided] {
-        protectedLoader->getResourceBytesAtPosition(count, position, [count, buffer, &dataSemaphore, &bytesProvided](const uint8_t* bytes, size_t bytesCount) {
+    RunLoop::main().dispatch([protectedLoader = Ref { *this }, dataSemaphore, position, count, buffer, &bytesProvided] {
+        if (dataSemaphore->wasSignaled())
+            return;
+        protectedLoader->getResourceBytesAtPosition(count, position, [count, buffer, dataSemaphore, &bytesProvided](const uint8_t* bytes, size_t bytesCount) {
+            if (dataSemaphore->wasSignaled())
+                return;
             RELEASE_ASSERT(bytesCount <= count);
             memcpy(buffer, bytes, bytesCount);
             bytesProvided = bytesCount;
-            dataSemaphore.signal();
+            dataSemaphore->signal();
         });
     });
 
-    dataSemaphore.wait();
+    dataSemaphore->wait();
 
 #if !LOG_DISABLED
     decrementThreadsWaitingOnCallback();
@@ -671,6 +685,19 @@ static void dataProviderReleaseInfoCallback(void* info)
     return bytesProvided;
 }
 
+auto PDFIncrementalLoader::createDataSemaphore() -> RefPtr<SemaphoreWrapper>
+{
+    Ref dataSemaphore = SemaphoreWrapper::create();
+    {
+        Locker locker { m_wasPDFThreadTerminationRequestedLock };
+        if (m_wasPDFThreadTerminationRequested)
+            return nullptr;
+
+        m_dataSemaphores.add(dataSemaphore.get());
+    }
+    return dataSemaphore;
+}
+
 void PDFIncrementalLoader::dataProviderGetByteRanges(CFMutableArrayRef buffers, const CFRange* ranges, size_t count)
 {
     ASSERT(!isMainRunLoop());
@@ -692,21 +719,29 @@ static void dataProviderReleaseInfoCallback(void* info)
     incrementalLoaderLog(stream.release());
 #endif
 
-    WTF::Semaphore dataSemaphore { 0 };
+    RefPtr dataSemaphore = createDataSemaphore();
+    if (!dataSemaphore)
+        return;
+
     Vector<RetainPtr<CFDataRef>> dataResults(count);
 
     // FIXME: Once we support multi-range requests, make a single request for all ranges instead of <count> individual requests.
-    RunLoop::main().dispatch([protectedLoader = Ref { *this }, &dataResults, ranges, count, &dataSemaphore] {
+    RunLoop::main().dispatch([protectedLoader = Ref { *this }, &dataResults, ranges, count, dataSemaphore]() mutable {
+        if (dataSemaphore->wasSignaled())
+            return;
+        Ref callbackAggregator = CallbackAggregator::create([dataSemaphore] {
+            dataSemaphore->signal();
+        });
         for (size_t i = 0; i < count; ++i) {
-            protectedLoader->getResourceBytesAtPosition(ranges[i].length, ranges[i].location, [i, &dataResults, &dataSemaphore](const uint8_t* bytes, size_t bytesCount) {
+            protectedLoader->getResourceBytesAtPosition(ranges[i].length, ranges[i].location, [i, &dataResults, dataSemaphore, callbackAggregator](const uint8_t* bytes, size_t bytesCount) {
+                if (dataSemaphore->wasSignaled())
+                    return;
                 dataResults[i] = adoptCF(CFDataCreate(kCFAllocatorDefault, bytes, bytesCount));
-                dataSemaphore.signal();
             });
         }
     });
 
-    for (size_t i = 0; i < count; ++i)
-        dataSemaphore.wait();
+    dataSemaphore->wait();
 
 #if !LOG_DISABLED
     decrementThreadsWaitingOnCallback();
@@ -777,7 +812,7 @@ static void dataProviderReleaseInfoCallback(void* info)
         return;
     }
 
-    WTF::Semaphore firstPageSemaphore { 0 };
+    BinarySemaphore firstPageSemaphore;
     auto firstPageQueue = WorkQueue::create("PDF first page work queue");
 
     [m_backgroundThreadDocument preloadDataOfPagesInRange:NSMakeRange(0, 1) onQueue:firstPageQueue->dispatchQueue() completion:[&firstPageSemaphore, protectedThis = Ref { *this }] (NSIndexSet *) mutable {