[UnifiedPDF] Form control repaints are incorrect when PDF content cha…

…nges across multiple tiles

https://bugs.webkit.org/show_bug.cgi?id=274237
rdar://128072857

Reviewed by Simon Fraser.

When PDF content changes across multiple tiles, for example if we have
multiple checkboxes toggled and we reset them with the reset action, we
enqueue multiple tile paints that chase each other, ultimately for them
all to be dropped on the floor because they are either not the most
recent tile render or because the most recent tile
renders contents version identifier does not match that of the current
render info. The former reason is valid while the latter is not. In
fact, local experimentation shows that our checking against the contents
version identifiers is redundant because the render identifier state
always reflects what we learn from the contents version check.

In this PR, we remove the notion of contents version from the async
renderer, since this information is redudnant and our aggressive
checking of this information incorrectly drops valid tile renders on the
floor.

* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.mm:
(WebKit::AsyncPDFRenderer::AsyncPDFRenderer):
(WebKit::AsyncPDFRenderer::renderInfoIsValidForTile const):
(WebKit::AsyncPDFRenderer::willRepaintTile):
(WebKit::AsyncPDFRenderer::renderInfoForTile const):
(WebKit::AsyncPDFRenderer::didCompleteTileRender):
(WebKit::AsyncPDFRenderer::pdfContentChangedInRect):

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

Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.h

@@ -91,9 +91,6 @@ class UnifiedPDFPlugin;
 struct PDFTileRenderType;
 using PDFTileRenderIdentifier = ObjectIdentifier<PDFTileRenderType>;
 
-struct PDFContentsVersionType;
-using PDFContentsVersionIdentifier = ObjectIdentifier<PDFContentsVersionType>;
-
 class AsyncPDFRenderer final : public WebCore::TiledBackingClient,
     public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AsyncPDFRenderer> {
     WTF_MAKE_FAST_ALLOCATED;
@@ -128,23 +125,16 @@ class AsyncPDFRenderer final : public WebCore::TiledBackingClient,
         WebCore::FloatRect tileRect;
         std::optional<WebCore::FloatRect> clipRect; // If set, represents the portion of the tile that needs repaint (in the same coordinate system as tileRect).
         PDFPageCoverageAndScales pageCoverage;
-        PDFContentsVersionIdentifier contentsVersion;
 
         bool equivalentForPainting(const TileRenderInfo& other) const
-        {
-            return tileRect == other.tileRect && pageCoverage == other.pageCoverage && contentsVersion == other.contentsVersion;
-        }
-
-        bool equivalentForPaintingIgnoringContentVersion(const TileRenderInfo& other) const
         {
             return tileRect == other.tileRect && pageCoverage == other.pageCoverage;
         }
     };
 
     TileRenderInfo renderInfoForTile(const TileForGrid& tileInfo, const WebCore::FloatRect& tileRect, const std::optional<WebCore::FloatRect>& clipRect = { }) const;
 
-    enum class CheckContentVersion : bool { No, Yes };
-    bool renderInfoIsValidForTile(const TileForGrid&, const TileRenderInfo&, CheckContentVersion = CheckContentVersion::Yes) const;
+    bool renderInfoIsValidForTile(const TileForGrid&, const TileRenderInfo&) const;
 
     // TiledBackingClient
     void willRepaintTile(WebCore::TiledBacking&, WebCore::TileGridIdentifier, WebCore::TileIndex, const WebCore::FloatRect& tileRect, const WebCore::FloatRect& tileDirtyRect) final;
@@ -184,8 +174,6 @@ class AsyncPDFRenderer final : public WebCore::TiledBackingClient,
     RefPtr<WebCore::GraphicsLayer> m_pdfContentsLayer;
     Ref<ConcurrentWorkQueue> m_paintingWorkQueue;
 
-    PDFContentsVersionIdentifier m_contentsVersion;
-
     struct TileRenderData {
         PDFTileRenderIdentifier renderIdentifier;
         TileRenderInfo renderInfo;

Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.mm

@@ -50,7 +50,6 @@
 AsyncPDFRenderer::AsyncPDFRenderer(UnifiedPDFPlugin& plugin)
     : m_plugin(plugin)
     , m_paintingWorkQueue(ConcurrentWorkQueue::create("WebKit: PDF Painting Work Queue"_s, WorkQueue::QOS::UserInteractive)) // Maybe make this concurrent?
-    , m_contentsVersion(PDFContentsVersionIdentifier::generate())
     , m_maxConcurrentTileRenders(std::clamp(WTF::numberOfProcessorCores() - 2, 4, 16))
 {
 }
@@ -148,7 +147,7 @@
     return m_pagePreviews.get(pageIndex);
 }
 
-bool AsyncPDFRenderer::renderInfoIsValidForTile(const TileForGrid& tileInfo, const TileRenderInfo& renderInfo, CheckContentVersion checkContentVersion) const
+bool AsyncPDFRenderer::renderInfoIsValidForTile(const TileForGrid& tileInfo, const TileRenderInfo& renderInfo) const
 {
     ASSERT(isMainRunLoop());
     if (!m_pdfContentsLayer)
@@ -161,10 +160,7 @@
     auto currentTileRect = tiledBacking->rectForTile(tileInfo.tileIndex);
     auto currentRenderInfo = renderInfoForTile(tileInfo, currentTileRect);
 
-    if (checkContentVersion == CheckContentVersion::Yes)
-        return renderInfo.equivalentForPainting(currentRenderInfo);
-
-    return renderInfo.equivalentForPaintingIgnoringContentVersion(currentRenderInfo);
+    return renderInfo.equivalentForPainting(currentRenderInfo);
 }
 
 void AsyncPDFRenderer::willRepaintTile(TiledBacking&, TileGridIdentifier gridIdentifier, TileIndex tileIndex, const FloatRect& tileRect, const FloatRect& tileDirtyRect)
@@ -180,8 +176,7 @@
         if (renderInfo.tileRect != tileRect)
             return false;
 
-        // It's OK to paint tiles with a stale content version (e.g. old state of a checkbox). We'll paint the new tile whne we get it.
-        return renderInfoIsValidForTile(tileInfo, renderInfo, CheckContentVersion::No);
+        return renderInfoIsValidForTile(tileInfo, renderInfo);
     };
 
     LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::willRepaintTile " << tileInfo << " rect " << tileRect << " (dirty rect " << tileDirtyRect << ") - already queued "
@@ -342,7 +337,7 @@
     auto paintingClipRect = convertTileRectToPaintingCoords(tileRect, tilingScaleFactor);
     auto pageCoverage = plugin->pageCoverageAndScalesForRect(paintingClipRect);
 
-    return TileRenderInfo { tileRect, clipRect, pageCoverage, m_contentsVersion };
+    return TileRenderInfo { tileRect, clipRect, pageCoverage };
 }
 
 void AsyncPDFRenderer::enqueuePaintWithClip(const TileForGrid& tileInfo, const TileRenderInfo& renderInfo)
@@ -535,7 +530,7 @@
     --m_numConcurrentTileRenders;
     serviceRequestQueue();
 
-    LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteNewTileRender - got results for tile at " << tileInfo << " clip " << renderInfo.clipRect << " ident " << renderIdentifier
+    LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteTileRender - got results for tile at " << tileInfo << " clip " << renderInfo.clipRect << " ident " << renderIdentifier
         << " (" << m_rendereredTiles.size() << " tiles in cache). Request revoked " << !requestWasValid);
 
     if (!requestWasValid)
@@ -551,11 +546,11 @@
     if (renderInfo.clipRect) {
         auto renderedTilesIt = m_rendereredTiles.find(tileInfo);
         if (renderedTilesIt == m_rendereredTiles.end()) {
-            LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteTileUpdateRender - tile to be updated " << tileInfo << " has been removed");
+            LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteTileRender - tile to be updated " << tileInfo << " has been removed");
             return;
         }
 
-        LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteTileUpdateRender - updating tile " << tileInfo << " in rect " << *renderInfo.clipRect);
+        LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::didCompleteTileRender - updating tile " << tileInfo << " in rect " << *renderInfo.clipRect);
 
         RefPtr existingBuffer = renderedTilesIt->value.buffer;
         auto& context = existingBuffer->context();
@@ -652,8 +647,6 @@
     if (!pdfDocument)
         return;
 
-    m_contentsVersion = PDFContentsVersionIdentifier::generate();
-
     auto toTileTransform = paintingToTileTransform(pageScaleFactor);
     auto paintingRectInTileCoordinates = toTileTransform.mapRect(paintingRect);