Bug 184268 – Loading of multipart response was cancelled because of content policy set in WebFrameLoaderClient::dispatchDecidePolicyForResponse

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Sihui Liu]

Image from Multipart/x-mixed-replace response is not automatically refreshed and occasional crash after fixed bug 182532.

[Radar WebKit Bug Importer]

<rdar://problem/39144446>

[Sihui Liu]

rdar://problem/37825283

[Sihui Liu]

Created attachment 337185 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sihui Liu]

Created attachment 337221 [details]
Patch

[Ryosuke Niwa]

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?

[Sihui Liu]

Created attachment 337240 [details]
Patch

[Daniel Bates]

Can you please attach the crash report/stack trace for the crash to this bug?

[Daniel Bates]

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?

[Daniel Bates]

(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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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

[Sihui Liu]

(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).

[Sihui Liu]

Created attachment 337316 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 337615 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337615 [details]
Patch

Clearing flags on attachment: 337615

Committed r230489: <https://trac.webkit.org/changeset/230489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

*** Bug 184021 has been marked as a duplicate of this bug. ***