Image from Multipart/x-mixed-replace response is not automatically refreshed and occasional crash after fixed bug 182532.
<rdar://problem/39144446>
rdar://problem/37825283
Created attachment 337185 [details] Patch
Comment on attachment 337185 [details] Patch Attachment 337185 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7206286 New failing tests: http/tests/multipart/multipart-html-no-injected-policy.php
Created attachment 337198 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 337185 [details] Patch Attachment 337185 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7206314 New failing tests: http/tests/multipart/multipart-html-no-injected-policy.php
Created attachment 337203 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 337185 [details] Patch Attachment 337185 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7206836 New failing tests: http/tests/multipart/multipart-html-no-injected-policy.php
Created attachment 337213 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 337221 [details] Patch
Comment on attachment 337221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337221&action=review r- due to the lack of description in the change log. > Source/WebCore/ChangeLog:8 > + Please add a description as to why the crash was happening, and how you fixed it. > Source/WebCore/ChangeLog:9 > + No new tests. An existing test is modified to not use injected bundle policy so it can test the crash case. You should name the test you've modified. > LayoutTests/http/tests/multipart/multipart-html.php:-24 > - $i++; So we used to only generate 5 results?
Created attachment 337240 [details] Patch
Can you please attach the crash report/stack trace for the crash to this bug?
Comment on attachment 337240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337240&action=review > Source/WebCore/loader/DocumentLoader.cpp:794 > + if (m_doneCheckingMultipartContentPolicy) What does it mean for a boundary to be followed by a different Content-Type than its predecessor? Do we precent that? If not, how did you come to the decision to not allow the embedding client to make a decision on the load?
(In reply to Daniel Bates from comment #14) > Comment on attachment 337240 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=337240&action=review > > > Source/WebCore/loader/DocumentLoader.cpp:794 > > + if (m_doneCheckingMultipartContentPolicy) > > What does it mean for a boundary to be followed by a different Content-Type > than its predecessor? Do we precent that? If not, how did you come to the > decision to not allow the embedding client to make a decision on the load? *prevent
Comment on attachment 337240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337240&action=review > Source/WebCore/ChangeLog:10 > + should check the content policy only once for reponses from same multipart stream. Typo: reponses > Source/WebCore/loader/cache/CachedResourceLoader.cpp:-1261 > - ASSERT(!m_documentResources.contains(resource.url()) || m_documentResources.get(resource.url()).get() == &resource); Please explain in the Changelog why dropping this assertion is OK. > LayoutTests/http/tests/multipart/multipart-html.php:13 > + testRunner.setShouldDecideResponsePolicyAfterDelay(true); This is a no-op nowadays since all policy decisions are made asynchronously now. I will drop this testRunner API soon. > LayoutTests/http/tests/multipart/multipart-html.php:23 > + echo "On pass, The message number should be 10."; Shouldn't this say FAIL ? > LayoutTests/http/tests/multipart/multipart-html.php:25 > + echo "Pass: This is message {$i}."; We usually use PASS in all caps.
Comment on attachment 337240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337240&action=review > Source/WebCore/loader/DocumentLoader.cpp:816 > + m_doneCheckingMultipartContentPolicy = true; This flag seems like a lie, it claims policy check was done although it will be started only a few lines below.
Comment on attachment 337240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337240&action=review >> LayoutTests/http/tests/multipart/multipart-html.php:13 >> + testRunner.setShouldDecideResponsePolicyAfterDelay(true); > > This is a no-op nowadays since all policy decisions are made asynchronously now. I will drop this testRunner API soon. Oh, I see from your changeling that you still need it to bypass the injected bundle policy decision. Never mind my comment then. I guess I'll need to change some injected bundle code before I can drop this testRunner API
(In reply to Daniel Bates from comment #13) > Can you please attach the crash report/stack trace for the crash to this bug? It's not a crash actually. In WebFrameLoaderClient::dispatchDecidePolicyForResponse, we used provisionalDocumentLoader() for policyDocumentLoader and if it's null, the policy is PolicyAction::Ignore. But after the first data of multipart response committed, the provisionalDocumentLoader would be set to null. So when we checked content policy for next responses we would get policy Ignore and the page stop loading subsequent resources. My solution is to simply avoid policy check for subsequent responses, but you make a point we still need to check the MIMEtype. So a substitute I can think of is not using provisionalDocumentLoader() in dispatchDecidePolicyForResponse for multipart response(like adding a new parameter isMultipart).
Created attachment 337316 [details] Patch
Comment on attachment 337316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337316&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:750 > + auto navigationID = policyDocumentLoader ? static_cast<WebDocumentLoader&>(*policyDocumentLoader).navigationID() : 0; This part of the patch looks good to me. However, getting rid of the assertion above seems odd. It would mean we are not removing the right CachedResource from m_documentResources, which could be a bug. > LayoutTests/ChangeLog:3 > + REGRESSION(r227758): Webpage fails to load due to crash in com.apple.WebKit: WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse + 267 I am unclear why this fixes a crash. My understanding is that https://trac.webkit.org/changeset/228852/webkit fixed this crash already. However, your patch does fix the fact that follow-up responses in the multipart case are no longer ignored (which caused Motion JPEG streams (such as http://128.97.43.214/mjpg/video.mjpg?resolution=352x288) to show only the first frame.
Comment on attachment 337316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337316&action=review >> LayoutTests/ChangeLog:3 >> + REGRESSION(r227758): Webpage fails to load due to crash in com.apple.WebKit: WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse + 267 > > I am unclear why this fixes a crash. My understanding is that https://trac.webkit.org/changeset/228852/webkit fixed this crash already. However, your patch does fix the fact that follow-up responses in the multipart case are no longer ignored (which caused Motion JPEG streams (such as http://128.97.43.214/mjpg/video.mjpg?resolution=352x288) to show only the first frame. Well, I see now that you already updated the title of the bug in Bugzilla. Please fix the title in the ChangeLogs as well.
Created attachment 337615 [details] Patch
Comment on attachment 337615 [details] Patch Clearing flags on attachment: 337615 Committed r230489: <https://trac.webkit.org/changeset/230489>
All reviewed patches have been landed. Closing bug.
Comment on attachment 337615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337615&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1260 > + if (m_documentResources.contains(resource.url()) && m_documentResources.get(resource.url()).get() != &resource) I think calling m_documentResources.contains() is redundant here. If the HashMap m_documentResources does not contain the resource url, m_documentResources.get(resource.url()).get() will return nullptr which will not be equal to &resource.
Comment on attachment 337316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337316&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:-1261 > - ASSERT(!m_documentResources.contains(resource.url()) || m_documentResources.get(resource.url()).get() == &resource); This assertion could indirectly have caused another assertion to fire with the following call stack (See bug 184021): #0 0x00000001151bca94 in ::WTFCrash() at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:271 #1 0x0000000107d8504c in WebCore::CachedResource::~CachedResource() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:169 #2 0x0000000107d91a05 in WebCore::CachedResource::~CachedResource() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:165 #3 0x0000000107d91a29 in WebCore::CachedResource::~CachedResource() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:165 #4 0x0000000107d928eb in WebCore::CachedResource::deleteIfPossible() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:607 #5 0x0000000107d94456 in WebCore::CachedResource::unregisterHandle(WebCore::CachedResourceHandleBase*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:786 #6 0x0000000107d94aad in WebCore::CachedResourceHandleBase::~CachedResourceHandleBase() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResourceHandle.cpp:55 #7 0x0000000107648305 in WebCore::CachedResourceHandle<WebCore::CachedResource>::~CachedResourceHandle() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResourceHandle.h:61 #8 0x0000000107645605 in WebCore::CachedResourceHandle<WebCore::CachedResource>::~CachedResourceHandle() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResourceHandle.h:61 #9 0x0000000107d9191b in WebCore::CachedResourceLoader::removeCachedResource(WebCore::CachedResource&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResourceLoader.cpp:1261 #10 0x0000000107d85169 in WebCore::CachedResource::~CachedResource() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:178 #11 0x0000000107d89057 in WebCore::CachedImage::~CachedImage() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedImage.cpp:85 #12 0x0000000107d89265 in WebCore::CachedImage::~CachedImage() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedImage.cpp:83 #13 0x0000000107d89289 in WebCore::CachedImage::~CachedImage() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedImage.cpp:83 #14 0x0000000107d928eb in WebCore::CachedResource::deleteIfPossible() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:607 #15 0x0000000107d94456 in WebCore::CachedResource::unregisterHandle(WebCore::CachedResourceHandleBase*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:786 #16 0x0000000107d94b27 in WebCore::CachedResourceHandleBase::setResource(WebCore::CachedResource*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResourceHandle.cpp:63 Notice the destructor CachedResource::~CachedResource() calls itself via other callas: 1. CachedResource::unregisterHandle() decides to delete the resource because m_handleCount = 0. So it calls deleteIfPossible() which calls the destructor CachedResource::~CachedResource(). 2. CachedResource::~CachedResource() sets m_deleted = true and calls CachedResourceLoader::removeCachedResource(). 3. Because the assertion at beginning of CachedResourceLoader::removeCachedResource() calls m_documentResources.get(resource.url()), a temporary CachedResourceHandleBase is created and then destroyed. 4. When this temporary CachedResourceHandleBase is created, it calls CachedResource::registerHandle() for the same object we are deleting. This will increment m_handleCount from 0 to 1. 5. When this temporary CachedResourceHandleBase is destroyed, it calls CachedResource::unregisterHandle() which decrements m_handleCount from 1 to 0. This will cause the object to be deleted a second time from the destructor of the same object. 6. The assertion ASSERT(!m_deleted) in CachedResource::~CachedResource() will fire because it was set to true.
*** Bug 184021 has been marked as a duplicate of this bug. ***