Title: [258477] trunk/Source/WebKit
Revision
258477
Author
beid...@apple.com
Date
2020-03-14 20:40:43 -0700 (Sat, 14 Mar 2020)

Log Message

Fix the "deliver cached ranges" logic in PDFPlugin (and other small cleanups)
https://bugs.webkit.org/show_bug.cgi?id=209097

Reviewed by Tim Hatcher.

Streaming in data always appended to the buffer instead of first growing the buffer.
This wasn't noticed earlier because we often did not grow the buffer for successful range request completion.
But now we often do!

So this cleans that all up.

At the same time it revealed other interactions with PDFKit that force us to handle data requests on the main
thread after the document load is complete - Which is fine!

* WebProcess/Plugins/PDF/PDFPlugin.h:
* WebProcess/Plugins/PDF/PDFPlugin.mm:
(WebKit::dataProviderGetBytesAtPositionCallback): If on the main thread (and the document load is complete)
  handle the request directly!
(WebKit::PDFPlugin::getResourceBytesAtPositionMainThread):
(WebKit::PDFPlugin::ByteRangeRequest::completeWithAccumulatedData):
(WebKit::PDFPlugin::ensureDataBufferLength):
(WebKit::PDFPlugin::didFail):
(WebKit::PDFPlugin::maybeClearHighLatencyDataProviderFlag):
(WebKit::PDFPlugin::documentDataDidFinishLoading):
(WebKit::PDFPlugin::installPDFDocument):
(WebKit::PDFPlugin::manualStreamDidReceiveData): Grow the buffer instead of append.

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (258476 => 258477)


--- trunk/Source/WebKit/ChangeLog	2020-03-15 00:14:33 UTC (rev 258476)
+++ trunk/Source/WebKit/ChangeLog	2020-03-15 03:40:43 UTC (rev 258477)
@@ -1,3 +1,33 @@
+2020-03-14  Brady Eidson  <beid...@apple.com>
+
+        Fix the "deliver cached ranges" logic in PDFPlugin (and other small cleanups)
+        https://bugs.webkit.org/show_bug.cgi?id=209097
+
+        Reviewed by Tim Hatcher.
+
+        Streaming in data always appended to the buffer instead of first growing the buffer.
+        This wasn't noticed earlier because we often did not grow the buffer for successful range request completion.
+        But now we often do!
+        
+        So this cleans that all up.
+        
+        At the same time it revealed other interactions with PDFKit that force us to handle data requests on the main
+        thread after the document load is complete - Which is fine!
+
+        * WebProcess/Plugins/PDF/PDFPlugin.h:
+        * WebProcess/Plugins/PDF/PDFPlugin.mm:
+        (WebKit::dataProviderGetBytesAtPositionCallback): If on the main thread (and the document load is complete)
+          handle the request directly!
+        (WebKit::PDFPlugin::getResourceBytesAtPositionMainThread):
+        (WebKit::PDFPlugin::ByteRangeRequest::completeWithAccumulatedData):
+        (WebKit::PDFPlugin::ensureDataBufferLength):
+        (WebKit::PDFPlugin::didFail):
+        (WebKit::PDFPlugin::maybeClearHighLatencyDataProviderFlag):
+        (WebKit::PDFPlugin::documentDataDidFinishLoading):
+        (WebKit::PDFPlugin::installPDFDocument):
+        (WebKit::PDFPlugin::manualStreamDidReceiveData): Grow the buffer instead of append.
+
+
 2020-03-14  Brent Fulgham  <bfulg...@apple.com>
 
         Add missing checks needed for AppBound Quirk

Modified: trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h (258476 => 258477)


--- trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h	2020-03-15 00:14:33 UTC (rev 258476)
+++ trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h	2020-03-15 03:40:43 UTC (rev 258477)
@@ -130,8 +130,11 @@
     PDFPluginAnnotation* activeAnnotation() const { return m_activeAnnotation.get(); }
     WebCore::AXObjectCache* axObjectCache() const;
 
+    void ensureDataBufferLength(uint64_t length);
+
 #if HAVE(INCREMENTAL_PDF_APIS)
     void getResourceBytesAtPosition(size_t count, off_t position, CompletionHandler<void(const uint8_t*, size_t count)>&&);
+    size_t getResourceBytesAtPositionMainThread(void* buffer, off_t position, size_t count);
 #ifndef NDEBUG
     void pdfLog(const String& event);
     size_t incrementThreadsWaitingOnCallback() { return ++m_threadsWaitingOnCallback; }
@@ -377,6 +380,7 @@
     ByteRangeRequest* byteRangeRequestForLoader(WebCore::NetscapePlugInStreamLoader&);
     void forgetLoader(WebCore::NetscapePlugInStreamLoader&);
     void cancelAndForgetLoader(WebCore::NetscapePlugInStreamLoader&);
+    void maybeClearHighLatencyDataProviderFlag();
 
     RetainPtr<PDFDocument> m_backgroundThreadDocument;
     RefPtr<Thread> m_pdfThread;

Modified: trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm (258476 => 258477)


--- trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm	2020-03-15 00:14:33 UTC (rev 258476)
+++ trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm	2020-03-15 03:40:43 UTC (rev 258477)
@@ -98,6 +98,7 @@
 @interface PDFDocument ()
 -(instancetype)initWithProvider:(CGDataProviderRef)dataProvider;
 -(void)preloadDataOfPagesInRange:(NSRange)range onQueue:(dispatch_queue_t)queue completion:(void (^)(NSIndexSet* loadedPageIndexes))completionBlock;
+@property (readwrite, nonatomic) BOOL hasHighLatencyDataProvider;
 @end
 #endif
 
@@ -693,7 +694,12 @@
 
 static size_t dataProviderGetBytesAtPositionCallback(void* info, void* buffer, off_t position, size_t count)
 {
-    ASSERT(!isMainThread());
+    if (isMainThread()) {
+#if !LOG_DISABLED
+        ((PDFPlugin*)info)->pdfLog(makeString("Handling request for ", count, " bytes at position ", position, " synchronously on the main thread"));
+#endif
+        return ((PDFPlugin*)info)->getResourceBytesAtPositionMainThread(buffer, position, count);
+    }
 
 #if !LOG_DISABLED
     Ref<PDFPlugin> debugPluginRef = *((PDFPlugin*)info);
@@ -777,6 +783,15 @@
     adoptRef((PDFPlugin*)info);
 }
 
+void PDFPlugin::maybeClearHighLatencyDataProviderFlag()
+{
+    if (!m_pdfDocument || !m_documentFinishedLoading)
+        return;
+
+    if ([m_pdfDocument.get() respondsToSelector:@selector(setHasHighLatencyDataProvider:)])
+        [m_pdfDocument.get() setHasHighLatencyDataProvider:NO];
+}
+
 void PDFPlugin::threadEntry(Ref<PDFPlugin>&& protectedPlugin)
 {
     CGDataProviderDirectAccessRangesCallbacks dataProviderCallbacks {
@@ -821,6 +836,25 @@
     m_outstandingByteRangeRequests.clear();
 }
 
+size_t PDFPlugin::getResourceBytesAtPositionMainThread(void* buffer, off_t position, size_t count)
+{
+    ASSERT(isMainThread());
+    ASSERT(m_documentFinishedLoading);
+    ASSERT(position >= 0);
+
+    auto cfLength = CFDataGetLength(m_data.get());
+    ASSERT(cfLength >= 0);
+
+    if ((unsigned)position + count > (unsigned)cfLength) {
+        // We could return partial data, but this method should only be called
+        // once the entire buffer is known, and therefore PDFKit should only
+        // be asking for valid ranges.
+        return 0;
+    }
+    memcpy(buffer, CFDataGetBytePtr(m_data.get()) + position, count);
+    return count;
+}
+
 void PDFPlugin::getResourceBytesAtPosition(size_t count, off_t position, CompletionHandler<void(const uint8_t*, size_t)>&& completionHandler)
 {
     ASSERT(isMainThread()); 
@@ -924,22 +958,16 @@
 
     m_completionHandler(m_accumulatedData.data(), m_accumulatedData.size());
 
-    if (m_streamLoader)
-        plugin.forgetLoader(*m_streamLoader);
-
     // Fold this data into the main data buffer so that if something in its range is requested again (which happens quite often)
     // we do not need to hit the network layer again.
-
-    auto length = CFDataGetLength(plugin.m_data.get());
-    CFIndex targetSize = m_position + m_accumulatedData.size();
-    auto delta = targetSize - length;
-    if (delta > 0)
-        CFDataIncreaseLength(plugin.m_data.get(), delta);
-
+    plugin.ensureDataBufferLength(m_position + m_accumulatedData.size());
     if (m_accumulatedData.size()) {
         memcpy(CFDataGetMutableBytePtr(plugin.m_data.get()) + m_position, m_accumulatedData.data(), m_accumulatedData.size());
         plugin.m_completedRanges.add({ m_position, m_position + m_accumulatedData.size() - 1});
     }
+
+    if (m_streamLoader)
+        plugin.forgetLoader(*m_streamLoader);
 }
 
 bool PDFPlugin::ByteRangeRequest::maybeComplete(PDFPlugin& plugin)
@@ -1022,7 +1050,7 @@
             m_outstandingByteRangeRequests.remove(identifier);
     }
 
-    cancelAndForgetLoader(*loader);
+    forgetLoader(*loader);
 }
 
 void PDFPlugin::didFinishLoading(NetscapePlugInStreamLoader* loader)
@@ -1415,11 +1443,12 @@
 
 #if HAVE(INCREMENTAL_PDF_APIS)
 #if !LOG_DISABLED
-    pdfLog(makeString("PDF document finished loading with a total of %llu bytes", m_streamedBytes));
+    pdfLog(makeString("PDF document finished loading with a total of ", m_streamedBytes, " bytes"));
 #endif
     if (m_incrementalPDFLoadingEnabled) {
         // At this point we know all data for the document, and therefore we know how to answer any outstanding range requests.
         unconditionalCompleteOutstandingRangeRequests();
+        maybeClearHighLatencyDataProviderFlag();
     } else
 #endif
     {
@@ -1435,6 +1464,10 @@
     ASSERT(m_pdfDocument);
     ASSERT(isMainThread());
 
+#if HAVE(INCREMENTAL_PDF_APIS)
+    maybeClearHighLatencyDataProviderFlag();
+#endif
+
     updatePageAndDeviceScaleFactors();
 
     [m_pdfLayerController setFrameSize:size()];
@@ -1514,12 +1547,23 @@
         m_isPostScript = true;
 }
 
+void PDFPlugin::ensureDataBufferLength(uint64_t targetLength)
+{
+    ASSERT(m_data);
+
+    auto currentLength = CFDataGetLength(m_data.get());
+    ASSERT(currentLength >= 0);
+    if (targetLength > (uint64_t)currentLength)
+        CFDataIncreaseLength(m_data.get(), targetLength - currentLength);
+}
+
 void PDFPlugin::manualStreamDidReceiveData(const char* bytes, int length)
 {
     if (!m_data)
         m_data = adoptCF(CFDataCreateMutable(0, 0));
 
-    CFDataAppendBytes(m_data.get(), reinterpret_cast<const UInt8*>(bytes), length);
+    ensureDataBufferLength(m_streamedBytes + length);
+    memcpy(CFDataGetMutableBytePtr(m_data.get()) + m_streamedBytes, bytes, length);
     m_streamedBytes += length;
 
 #if HAVE(INCREMENTAL_PDF_APIS)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to