Bug 184268 - Loading of multipart response was cancelled because of content policy set in WebFrameLoaderClient::dispatchDecidePolicyForResponse
Summary: Loading of multipart response was cancelled because of content policy set in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
: 184021 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-03 10:11 PDT by Sihui Liu
Modified: 2018-04-27 18:48 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2018-04-04 10:32 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.20 MB, application/zip)
2018-04-04 11:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (2.93 MB, application/zip)
2018-04-04 12:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.13 MB, application/zip)
2018-04-04 13:02 PDT, EWS Watchlist
no flags Details
Patch (5.78 KB, patch)
2018-04-04 13:53 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2018-04-04 16:44 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2018-04-05 17:27 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (6.84 KB, patch)
2018-04-10 09:44 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2018-04-03 10:11:29 PDT
Image from Multipart/x-mixed-replace response is not automatically refreshed and occasional crash after fixed bug 182532.
Comment 1 Radar WebKit Bug Importer 2018-04-03 10:13:32 PDT
<rdar://problem/39144446>
Comment 2 Sihui Liu 2018-04-03 10:14:53 PDT
rdar://problem/37825283
Comment 3 Sihui Liu 2018-04-04 10:32:00 PDT
Created attachment 337185 [details]
Patch
Comment 4 EWS Watchlist 2018-04-04 11:38:47 PDT
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
Comment 5 EWS Watchlist 2018-04-04 11:38:49 PDT
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 6 EWS Watchlist 2018-04-04 12:24:54 PDT
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
Comment 7 EWS Watchlist 2018-04-04 12:24:56 PDT
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 8 EWS Watchlist 2018-04-04 13:02:08 PDT
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
Comment 9 EWS Watchlist 2018-04-04 13:02:20 PDT
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
Comment 10 Sihui Liu 2018-04-04 13:53:47 PDT
Created attachment 337221 [details]
Patch
Comment 11 Ryosuke Niwa 2018-04-04 15:20:13 PDT
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?
Comment 12 Sihui Liu 2018-04-04 16:44:06 PDT
Created attachment 337240 [details]
Patch
Comment 13 Daniel Bates 2018-04-04 20:42:19 PDT
Can you please attach the crash report/stack trace for the crash to this bug?
Comment 14 Daniel Bates 2018-04-04 21:02:51 PDT
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?
Comment 15 Daniel Bates 2018-04-04 21:03:16 PDT
(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 16 Chris Dumez 2018-04-05 09:36:52 PDT
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 17 Chris Dumez 2018-04-05 09:43:51 PDT
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 18 Chris Dumez 2018-04-05 09:46:01 PDT
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
Comment 19 Sihui Liu 2018-04-05 10:41:50 PDT
(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).
Comment 20 Sihui Liu 2018-04-05 17:27:48 PDT
Created attachment 337316 [details]
Patch
Comment 21 Chris Dumez 2018-04-09 16:47:43 PDT
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 22 Chris Dumez 2018-04-09 16:48:24 PDT
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.
Comment 23 Sihui Liu 2018-04-10 09:44:28 PDT
Created attachment 337615 [details]
Patch
Comment 24 WebKit Commit Bot 2018-04-10 12:46:45 PDT
Comment on attachment 337615 [details]
Patch

Clearing flags on attachment: 337615

Committed r230489: <https://trac.webkit.org/changeset/230489>
Comment 25 WebKit Commit Bot 2018-04-10 12:46:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Said Abou-Hallawa 2018-04-27 16:23:18 PDT
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 27 Said Abou-Hallawa 2018-04-27 17:44:53 PDT
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.
Comment 28 Said Abou-Hallawa 2018-04-27 18:48:37 PDT
*** Bug 184021 has been marked as a duplicate of this bug. ***