SubresourceLoader should keep a ResourceResponse of the previous part…

… of multipart/x-mixed-replace https://bugs.webkit.org/show_bug.cgi?id=263423 Reviewed by Chris Dumez. multipart/x-mixed-replace resources don't load progressively because it keeps showing the previous part content while loading the next part. Thus, SubresourceLoader finishes loading the current part content when the next resource responce comes in. SubresourceLoader::didReceiveResponse does the trick. It synthetically calls CachedResource::finishLoading for the previous part content. However, when CachedResource::finishLoading is called with the previous part content, CachedResource::responseReceived is already called with the next part resource responce. So, for example, if a multipart/x-mixed-replace resource contains two parts, CachedResource methods were called in the following sequence: CachedResource::responseReceived with multipart/x-mixed-replace CachedResource::responseReceived with the first part response CachedResource::responseReceived with the second part response CachedResource::finishLoading with the first part content CachedResource::finishLoading with the second part content As the result, the last part doesn't show as expected. <https://webkit.org/b/36536> SubresourceLoader has to keep the previous ResourceResponse and synthetically call CachedResource::responseReceived with it just before synthetically calling CachedResource::finishLoading. Also, this fixes another problem that debug build of Windows port was failing an assertion for http/tests/multipart/invalid-image-data.html test. The Image object wasn't recreated for the second part. CachedImage::responseReceived clears the previous Image object. But, it wasn't called for the second part just before finishLoading of the second part. * LayoutTests/platform/wincairo/TestExpectations: * LayoutTests/platform/wincairo/http/tests/multipart/invalid-image-data-expected.txt: * Source/WebCore/loader/SubresourceLoader.cpp: * Source/WebCore/loader/SubresourceLoader.h: * LayoutTests/http/tests/multipart/images-expected.html: Added. * LayoutTests/http/tests/multipart/images.html: Added. * LayoutTests/platform/glib/TestExpectations: Canonical link: https://commits.webkit.org/270023@main
WebKit · Oct 31, 2023 · 3a699a9 · 3a699a9
1 parent 385f5d9
commit 3a699a9
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 35 deletions.
diff --git a/LayoutTests/http/tests/multipart/images-expected.html b/LayoutTests/http/tests/multipart/images-expected.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<img src="resources/abe.png">
diff --git a/LayoutTests/http/tests/multipart/images.html b/LayoutTests/http/tests/multipart/images.html
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<script>
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    setTimeout(() => { testRunner.notifyDone() }, 1000);
+}
+</script>
+<img src="resources/multipart.py?interval=0&img1=green-100x100.png&img2=abe.png">
diff --git a/LayoutTests/platform/glib/TestExpectations b/LayoutTests/platform/glib/TestExpectations
@@ -1524,6 +1524,8 @@ webkit.org/b/258547 imported/w3c/web-platform-tests/html/browsers/browsing-the-w
 
 webkit.org/b/263629 http/tests/multipart/win-boundary-crash.html [ Skip ] # Crash
 
+webkit.org/b/263867 http/tests/multipart/images.html [ ImageOnlyFailure ]
+
 #////////////////////////////////////////////////////////////////////////////////////////
 # End of SOUP and Networking-related bugs
 #////////////////////////////////////////////////////////////////////////////////////////

diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
@@ -973,9 +973,6 @@ http/tests/ssl/curl/certificate-and-authentication.html [ Failure ]
 # Needs site isolation drawing implementation
 http/tests/site-isolation [ Skip ]
 
-webkit.org/b/263423 http/tests/multipart/invalid-image-data.html [ Skip ]
-webkit.org/b/263423 http/tests/multipart/multipart-async-image.html [ Skip ]
-
 #///////////////////////////////////////////////////////////////////////////////
 # Issue categories below are shared with other platforms (primarily AppleWin)
 #///////////////////////////////////////////////////////////////////////////////

diff --git a/LayoutTests/platform/wincairo/http/tests/multipart/invalid-image-data-expected.txt b/LayoutTests/platform/wincairo/http/tests/multipart/invalid-image-data-expected.txt
@@ -3,22 +3,22 @@ layer at (0,0) size 800x600
 layer at (0,0) size 800x600
   RenderBlock {HTML} at (0,0) size 800x600
     RenderBody {BODY} at (8,8) size 784x584
-      RenderBlock {P} at (0,0) size 784x40
-        RenderText {#text} at (0,0) size 51x19
-          text run at (0,0) width 51: "Test for "
-        RenderInline {I} at (0,0) size 779x39
-          RenderInline {A} at (0,0) size 305x19 [color=#0000EE]
-            RenderText {#text} at (51,0) size 305x19
-              text run at (51,0) width 305: "http://bugs.webkit.org/show_bug.cgi?id=13759"
-          RenderText {#text} at (356,0) size 779x39
-            text run at (356,0) width 4: " "
-            text run at (360,0) width 419: "REGRESSION (r20182-r20184): Incorrect rendering of multipart"
-            text run at (0,20) width 45: "images"
-        RenderText {#text} at (45,20) size 4x19
-          text run at (45,20) width 4: "."
-      RenderBlock {P} at (0,56) size 784x20
-        RenderText {#text} at (0,0) size 272x19
-          text run at (0,0) width 272: "There should be a bright green square below."
-      RenderBlock (anonymous) at (0,92) size 784x100
+      RenderBlock {P} at (0,0) size 784x36
+        RenderText {#text} at (0,0) size 52x17
+          text run at (0,0) width 52: "Test for "
+        RenderInline {I} at (0,0) size 775x35
+          RenderInline {A} at (0,0) size 301x17 [color=#0000EE]
+            RenderText {#text} at (52,0) size 301x17
+              text run at (52,0) width 301: "http://bugs.webkit.org/show_bug.cgi?id=13759"
+          RenderText {#text} at (353,0) size 775x35
+            text run at (353,0) width 4: " "
+            text run at (357,0) width 418: "REGRESSION (r20182-r20184): Incorrect rendering of multipart"
+            text run at (0,18) width 45: "images"
+        RenderText {#text} at (45,18) size 4x17
+          text run at (45,18) width 4: "."
+      RenderBlock {P} at (0,52) size 784x18
+        RenderText {#text} at (0,0) size 284x17
+          text run at (0,0) width 284: "There should be a bright green square below."
+      RenderBlock (anonymous) at (0,86) size 784x100
         RenderImage {IMG} at (0,0) size 100x100
         RenderText {#text} at (0,0) size 0x0
diff --git a/Source/WebCore/loader/SubresourceLoader.cpp b/Source/WebCore/loader/SubresourceLoader.cpp
@@ -475,8 +475,25 @@ void SubresourceLoader::didReceiveResponse(const ResourceResponse& response, Com
         }
     }
 
-    if (m_resource)
-        m_resource->responseReceived(response);
+    if (m_loadingMultipartContent) {
+        if (!m_previousPartResponse.isNull()) {
+            if (m_resource) {
+                m_resource->responseReceived(m_previousPartResponse);
+                // The resource data will change as the next part is loaded, so we need to make a copy.
+                m_resource->finishLoading(resourceData()->copy().ptr(), { });
+            }
+        }
+        clearResourceData();
+        m_previousPartResponse = response;
+        // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.
+        // After the first multipart section is complete, signal to delegates that this load is "finished"
+        NetworkLoadMetrics emptyMetrics;
+        m_documentLoader->subresourceLoaderFinishedLoadingOnePart(*this);
+        didFinishLoadingOnePart(emptyMetrics);
+    } else {
+        if (m_resource)
+            m_resource->responseReceived(response);
+    }
     if (reachedTerminalState())
         return;
 
@@ -501,19 +518,6 @@ void SubresourceLoader::didReceiveResponse(const ResourceResponse& response, Com
             }
         }
 
-        auto* buffer = resourceData();
-        if (m_loadingMultipartContent && buffer && buffer->size()) {
-            // The resource data will change as the next part is loaded, so we need to make a copy.
-            if (m_resource)
-                m_resource->finishLoading(buffer->copy().ptr(), { });
-            clearResourceData();
-            // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.
-            // After the first multipart section is complete, signal to delegates that this load is "finished"
-            NetworkLoadMetrics emptyMetrics;
-            m_documentLoader->subresourceLoaderFinishedLoadingOnePart(*this);
-            didFinishLoadingOnePart(emptyMetrics);
-        }
-
         if (responseHasHTTPStatusCodeError()) {
             m_loadTiming.markEndTime();
             auto* metrics = this->response().deprecatedNetworkLoadMetricsOrNull();
@@ -750,6 +754,8 @@ void SubresourceLoader::didFinishLoading(const NetworkLoadMetrics& networkLoadMe
         tracePoint(SubresourceLoadDidEnd, identifier().toUInt64());
 
     m_state = Finishing;
+    if (m_loadingMultipartContent && !m_previousPartResponse.isNull())
+        m_resource->responseReceived(m_previousPartResponse);
     m_resource->finishLoading(resourceData(), networkLoadMetrics);
 
     if (wasCancelled()) {

diff --git a/Source/WebCore/loader/SubresourceLoader.h b/Source/WebCore/loader/SubresourceLoader.h
@@ -139,6 +139,7 @@ class SubresourceLoader final : public ResourceLoader {
     std::optional<RequestCountTracker> m_requestCountTracker;
     RefPtr<SecurityOrigin> m_origin;
     CompletionHandler<void()> m_policyForResponseCompletionHandler;
+    ResourceResponse m_previousPartResponse;
     unsigned m_redirectCount { 0 };
     bool m_loadingMultipartContent { false };
     bool m_inAsyncResponsePolicyCheck { false };