Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content downloaded with fetch() API when Content-Encoding: gzip is set is not decompressed #6408

Conversation

rreno
Copy link
Member

@rreno rreno commented Nov 11, 2022

ec8ff55

Content downloaded with fetch() API when Content-Encoding: gzip is set is not decompressed
https://bugs.webkit.org/show_bug.cgi?id=247421
rdar://101935292

Reviewed by Brent Fulgham.

In 195247@main we adopted CFNetwork SPI to opt-out of content encoding sniffing for XMLHttpRequest.
Content encoding sniffing would cause responses with `Content-Encoding: gzip` and
`Content-Type: application/octet-stream` to be treated as `Content-Type: application/gzip` and
`Content-Encoding: identity` on macOS. The user and developer visible behavior is that the gzipped data
is not decompressed.

This brings that change forward to Fetch requests by setting the ContentEncodingSniffingPolicy flag
in the FetchLoader. As an additional step this also renames the ContentEncodingSniffingPolicy flags to
Default and Disable and replaces raw bools representing those flags with the enum itself.

On iOS the flag has no effect since the default behavior is to opt-out of sniffing.

* LayoutTests/platform/mac-wk1/TestExpectations:
    Skip new worker tests since workers are unsupported in WK1.

* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.js: Added.
(string_appeared_here.forEach.contentType.promise_test.async t):
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.serviceworker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.serviceworker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.sharedworker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.sharedworker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.worker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.worker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.octetstream.gz: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.octetstream.gz.headers: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.text.gz: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.text.gz.headers: Added.

* Source/WebCore/Modules/fetch/FetchLoader.cpp:
(WebCore::FetchLoader::start):
* Source/WebCore/loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::start):
* Source/WebCore/loader/ResourceLoader.h:
(WebCore::ResourceLoader::contentEncodingSniffingPolicy const):
(WebCore::ResourceLoader::shouldSniffContentEncoding const): Deleted.
* Source/WebCore/loader/ResourceLoaderOptions.h:
(WebCore::ResourceLoaderOptions::ResourceLoaderOptions):
* Source/WebCore/loader/ThreadableLoader.cpp:
(WebCore::ThreadableLoaderOptions::isolatedCopy const):
* Source/WebCore/platform/network/BlobResourceHandle.cpp:
(WebCore::BlobResourceHandle::BlobResourceHandle):
* Source/WebCore/platform/network/ResourceHandle.cpp:
(WebCore::ResourceHandle::ResourceHandle):
(WebCore::ResourceHandle::create):
(WebCore::ResourceHandle::contentEncodingSniffingPolicy const):
(WebCore::ResourceHandle::shouldContentEncodingSniff const): Deleted.
* Source/WebCore/platform/network/ResourceHandle.h:
* Source/WebCore/platform/network/ResourceHandleInternal.h:
(WebCore::ResourceHandleInternal::ResourceHandleInternal):
* Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:
(WebCore::ResourceHandle::createCFURLConnection):
(WebCore::ResourceHandle::start):
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/platform/network/mac/ResourceHandleMac.mm:
(WebCore::ResourceHandle::applySniffingPoliciesIfNeeded):
(WebCore::ResourceHandle::createNSURLConnection):
(WebCore::ResourceHandle::start):
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::createRequest):
* Source/WebKit/NetworkProcess/NetworkLoadParameters.h:
* Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.cpp:
(WebKit::ServiceWorkerSoftUpdateLoader::loadFromNetwork):
* Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
(WebKit::NetworkCache::SpeculativeLoad::SpeculativeLoad):
* Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::SpeculativeLoadManager::preconnectForSubresource):
* Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:
* Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
(WebKit::NetworkDataTaskCocoa::applySniffingPoliciesAndBindRequestToInferfaceIfNeeded):
(WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa):
* Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:
(WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
(WebKit::WebLoaderStrategy::loadResourceSynchronously):
* Source/WebKitLegacy/WebCoreSupport/PingHandle.h:

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

2827e85

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@rreno rreno requested a review from cdumez as a code owner November 11, 2022 20:34
@rreno rreno self-assigned this Nov 11, 2022
@rreno rreno added the Page Loading For bugs in page loading, including handling of network callbacks. label Nov 11, 2022
@rreno
Copy link
Member Author

rreno commented Nov 11, 2022

Not quite ready to land yet. I need to incorporate test cases and verify this actually builds on ports other than macOS.

@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from 98b0095 to a4be646 Compare November 11, 2022 20:44
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 11, 2022
@achristensen07
Copy link
Contributor

Seems reasonable though

@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label Nov 11, 2022
@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from a4be646 to 9bbc8fa Compare November 11, 2022 22:56
@@ -365,7 +365,7 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
}

ContentSniffingPolicy contentSniffingPolicy = resourceLoader.shouldSniffContent() ? ContentSniffingPolicy::SniffContent : ContentSniffingPolicy::DoNotSniffContent;
ContentEncodingSniffingPolicy contentEncodingSniffingPolicy = resourceLoader.shouldSniffContentEncoding() ? ContentEncodingSniffingPolicy::Sniff : ContentEncodingSniffingPolicy::DoNotSniff;
ContentEncodingSniffingPolicy contentEncodingSniffingPolicy = resourceLoader.contentEncodingSniffingPolicy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should be auto.

@@ -50,8 +51,7 @@ class PingHandle final : private WebCore::ResourceHandleClient {
{
bool defersLoading = false;
bool shouldContentSniff = false;
bool shouldContentEncodingSniff = true;
m_handle = WebCore::ResourceHandle::create(networkingContext, request, this, defersLoading, shouldContentSniff, shouldContentEncodingSniff, nullptr, false);
m_handle = WebCore::ResourceHandle::create(networkingContext, request, this, defersLoading, shouldContentSniff, WebCore::ContentEncodingSniffingPolicy::Default, nullptr, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum really is much more readable than a bool!

#else
void ResourceHandle::createNSURLConnection(id delegate, bool shouldUseCredentialStorage, bool shouldContentSniff, bool shouldContentEncodingSniff, SchedulingBehavior schedulingBehavior, NSDictionary *connectionProperties)
void ResourceHandle::createNSURLConnection(id delegate, bool shouldUseCredentialStorage, bool shouldContentSniff, ContentEncodingSniffingPolicy contentEncodingSniffingPolicy, SchedulingBehavior schedulingBehavior, NSDictionary *connectionProperties)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading through this, it seems silly to have two different method signatures because we need a connection properties object for IOS_FAMILY. Would probably be cleaner to just pass nullptr on macOS for this. (Not relevant to this review -- just an observation).

@@ -107,7 +107,7 @@ class ResourceHandleInternal {

bool m_defersLoading;
bool m_shouldContentSniff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should get a similar treatment to the ContentEncodingSniffingPolicy member.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that and I plan to do that in a follow-up.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from 9bbc8fa to b454255 Compare November 12, 2022 02:36
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from b454255 to 7b0524c Compare November 12, 2022 17:14
@rreno
Copy link
Member Author

rreno commented Nov 15, 2022

While writing tests for this I discovered that this solution does not seem to work for Dedicated and Shared workers. Converting this to draft for now while I work that out.

@rreno rreno marked this pull request as draft November 15, 2022 22:18
@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch 2 times, most recently from e93d908 to d40db4a Compare November 15, 2022 22:28
@rreno rreno changed the title Content downloaded with fetch() API when Concent-Encoding: gzip is set is not decompressed Content downloaded with fetch() API when Content-Encoding: gzip is set is not decompressed Nov 15, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 16, 2022
@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label Nov 16, 2022
@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from d40db4a to 6b032d2 Compare November 16, 2022 01:15
@rreno rreno marked this pull request as ready for review November 16, 2022 01:15
@rreno
Copy link
Member Author

rreno commented Nov 16, 2022

Should be good to go now pending EWS results.

I wasn't copying the ResourceLoaderOptions::contentEncodingSniffingPolicy in an isolatedCopy call so the copy from worker thread to main thread was setting the flag to Default when we wanted Disable

@rreno rreno force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from 6b032d2 to 2827e85 Compare November 16, 2022 01:50
@rreno rreno added the merge-queue Applied to send a pull request to merge-queue label Nov 16, 2022
…t is not decompressed

https://bugs.webkit.org/show_bug.cgi?id=247421
rdar://101935292

Reviewed by Brent Fulgham.

In 195247@main we adopted CFNetwork SPI to opt-out of content encoding sniffing for XMLHttpRequest.
Content encoding sniffing would cause responses with `Content-Encoding: gzip` and
`Content-Type: application/octet-stream` to be treated as `Content-Type: application/gzip` and
`Content-Encoding: identity` on macOS. The user and developer visible behavior is that the gzipped data
is not decompressed.

This brings that change forward to Fetch requests by setting the ContentEncodingSniffingPolicy flag
in the FetchLoader. As an additional step this also renames the ContentEncodingSniffingPolicy flags to
Default and Disable and replaces raw bools representing those flags with the enum itself.

On iOS the flag has no effect since the default behavior is to opt-out of sniffing.

* LayoutTests/platform/mac-wk1/TestExpectations:
    Skip new worker tests since workers are unsupported in WK1.

* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.js: Added.
(string_appeared_here.forEach.contentType.promise_test.async t):
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.serviceworker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.serviceworker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.sharedworker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.sharedworker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.worker-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/gzip-body.any.worker.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.octetstream.gz: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.octetstream.gz.headers: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.text.gz: Added.
* LayoutTests/imported/w3c/web-platform-tests/fetch/content-encoding/resources/foo.text.gz.headers: Added.

* Source/WebCore/Modules/fetch/FetchLoader.cpp:
(WebCore::FetchLoader::start):
* Source/WebCore/loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::start):
* Source/WebCore/loader/ResourceLoader.h:
(WebCore::ResourceLoader::contentEncodingSniffingPolicy const):
(WebCore::ResourceLoader::shouldSniffContentEncoding const): Deleted.
* Source/WebCore/loader/ResourceLoaderOptions.h:
(WebCore::ResourceLoaderOptions::ResourceLoaderOptions):
* Source/WebCore/loader/ThreadableLoader.cpp:
(WebCore::ThreadableLoaderOptions::isolatedCopy const):
* Source/WebCore/platform/network/BlobResourceHandle.cpp:
(WebCore::BlobResourceHandle::BlobResourceHandle):
* Source/WebCore/platform/network/ResourceHandle.cpp:
(WebCore::ResourceHandle::ResourceHandle):
(WebCore::ResourceHandle::create):
(WebCore::ResourceHandle::contentEncodingSniffingPolicy const):
(WebCore::ResourceHandle::shouldContentEncodingSniff const): Deleted.
* Source/WebCore/platform/network/ResourceHandle.h:
* Source/WebCore/platform/network/ResourceHandleInternal.h:
(WebCore::ResourceHandleInternal::ResourceHandleInternal):
* Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:
(WebCore::ResourceHandle::createCFURLConnection):
(WebCore::ResourceHandle::start):
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/platform/network/mac/ResourceHandleMac.mm:
(WebCore::ResourceHandle::applySniffingPoliciesIfNeeded):
(WebCore::ResourceHandle::createNSURLConnection):
(WebCore::ResourceHandle::start):
(WebCore::ResourceHandle::platformLoadResourceSynchronously):
* Source/WebCore/xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::createRequest):
* Source/WebKit/NetworkProcess/NetworkLoadParameters.h:
* Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.cpp:
(WebKit::ServiceWorkerSoftUpdateLoader::loadFromNetwork):
* Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
(WebKit::NetworkCache::SpeculativeLoad::SpeculativeLoad):
* Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::SpeculativeLoadManager::preconnectForSubresource):
* Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:
* Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
(WebKit::NetworkDataTaskCocoa::applySniffingPoliciesAndBindRequestToInferfaceIfNeeded):
(WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa):
* Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:
(WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
(WebKit::WebLoaderStrategy::loadResourceSynchronously):
* Source/WebKitLegacy/WebCoreSupport/PingHandle.h:

Canonical link: https://commits.webkit.org/256755@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Content-downloaded-with-fetch-API-when-Concent-Encoding-gzip-is-set-is-not-decompressed branch from 2827e85 to ec8ff55 Compare November 16, 2022 23:07
@webkit-commit-queue
Copy link
Collaborator

Committed 256755@main (ec8ff55): https://commits.webkit.org/256755@main

Reviewed commits have been landed. Closing PR #6408 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit ec8ff55 into WebKit:main Nov 16, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Page Loading For bugs in page loading, including handling of network callbacks.
Projects
None yet
6 participants