Bug 194027 – Stop using blobRegistry in NetworkProcess

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Stop using blobRegistry in NetworkProcess

[Alex Christensen]

Created attachment 360593 [details]
Patch

[Alex Christensen]

I'm still getting crashes with this stack trace:

0   com.apple.WebCore             	0x00000001126f44a8 WebCore::appendBlobResolved(WebCore::FormData*, WTF::URL const&) + 24 (FormData.cpp:316)
1   com.apple.WebCore             	0x00000001126f4468 WebCore::FormData::resolveBlobReferences()::$_9::operator()(WebCore::FormDataElement::EncodedBlobData const&) const + 40 (FormData.cpp:365)
2   com.apple.WebCore             	0x00000001126f4351 void WTF::__visitor_table<WTF::Visitor<WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>, WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>::__trampoline_func<WebCore::FormDataElement::EncodedBlobData>(WTF::Visitor<WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>&, WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>&) + 49 (Variant.h:1868)
3   com.apple.WebCore             	0x00000001126f4210 WTF::__visitor_return_type<WTF::Visitor<WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>, WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>::__type WTF::visit<WTF::Visitor<WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>, WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>(WTF::Visitor<WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>&&, WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>&) + 80 (Variant.h:1882)
4   com.apple.WebCore             	0x00000001126e7142 decltype(visit(makeVisitor(std::forward<WebCore::FormData::resolveBlobReferences()::$_7>(fp0), std::forward<WebCore::FormData::resolveBlobReferences()::$_8>(fp0), std::forward<WebCore::FormData::resolveBlobReferences()::$_9>(fp0)), std::forward<WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>&>(fp))) WTF::switchOn<WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>&, WebCore::FormData::resolveBlobReferences()::$_7, WebCore::FormData::resolveBlobReferences()::$_8, WebCore::FormData::resolveBlobReferences()::$_9>(WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::FormDataElement::EncodedFileData, WebCore::FormDataElement::EncodedBlobData>&&&, WebCore::FormData::resolveBlobReferences()::$_7&&, WebCore::FormData::resolveBlobReferences()::$_8&&, WebCore::FormData::resolveBlobReferences()::$_9&&) + 114 (Variant.h:2049)
5   com.apple.WebCore             	0x00000001126e6fb3 WebCore::FormData::resolveBlobReferences() + 387 (FormData.cpp:357)
6   com.apple.WebCore             	0x000000011271a720 WebCore::createHTTPBodyCFReadStream(WebCore::FormData&) + 48 (FormDataStreamCFNet.cpp:378)
7   com.apple.WebCore             	0x000000011271b082 WebCore::setHTTPBody(_CFURLRequest*, WebCore::FormData*) + 242 (FormDataStreamCFNet.cpp:402)
8   com.apple.WebCore             	0x000000010ff9c538 WebCore::setHTTPBody(NSMutableURLRequest*, WebCore::FormData*) + 56 (FormDataStreamMac.mm:40)
9   com.apple.WebCore             	0x000000010ff388c7 WebCore::ResourceRequest::doUpdatePlatformHTTPBody() + 391 (ResourceRequestCocoa.mm:244)
10  com.apple.WebCore             	0x0000000112710184 WebCore::ResourceRequestBase::updatePlatformRequest(WebCore::HTTPBodyUpdatePolicy) const + 308 (ResourceRequestBase.cpp:700)
11  com.apple.WebCore             	0x000000010ff361a9 WebCore::ResourceRequest::nsURLRequest(WebCore::HTTPBodyUpdatePolicy) const + 41 (ResourceRequestCocoa.mm:45)
12  com.apple.WebKit              	0x00000001087497d7 WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, unsigned long long, unsigned long long, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, WebKit::PreconnectOnly, bool, WTF::Optional<WebKit::NetworkActivityTracker>) + 1799 (NetworkDataTaskCocoa.mm:201)
13  com.apple.WebKit              	0x000000010874adf3 WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, unsigned long long, unsigned long long, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, WebKit::PreconnectOnly, bool, WTF::Optional<WebKit::NetworkActivityTracker>) + 243 (NetworkDataTaskCocoa.mm:249)
14  com.apple.WebKit              	0x0000000108765eff WebKit::NetworkDataTaskCocoa::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, unsigned long long, unsigned long long, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, WebKit::PreconnectOnly, bool, WTF::Optional<WebKit::NetworkActivityTracker>) + 463 (NetworkDataTaskCocoa.h:47)
15  com.apple.WebKit              	0x000000010875bd04 WebKit::NetworkDataTask::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebKit::NetworkLoadParameters const&) + 372 (NetworkDataTask.cpp:51)
16  com.apple.WebKit              	0x000000010876a7c4 WebKit::NetworkLoad::initialize(WebKit::NetworkSession&, WebCore::BlobRegistryImpl*) + 244 (NetworkLoad.cpp:68)
17  com.apple.WebKit              	0x000000010876a640 WebKit::NetworkLoad::NetworkLoad(WebKit::NetworkLoadClient&, WebCore::BlobRegistryImpl*, WebKit::NetworkLoadParameters&&, WebKit::NetworkSession&) + 272 (NetworkLoad.cpp:61)
18  com.apple.WebKit              	0x000000010876a865 WebKit::NetworkLoad::NetworkLoad(WebKit::NetworkLoadClient&, WebCore::BlobRegistryImpl*, WebKit::NetworkLoadParameters&&, WebKit::NetworkSession&) + 53 (NetworkLoad.cpp:61)
19  com.apple.WebKit              	0x00000001087d7603 std::__1::__unique_if<WebKit::NetworkLoad>::__unique_single std::__1::make_unique<WebKit::NetworkLoad, WebKit::NetworkResourceLoader&, WebCore::BlobRegistryImpl*, WebKit::NetworkLoadParameters, WebKit::NetworkSession&>(WebKit::NetworkResourceLoader&&&, WebCore::BlobRegistryImpl*&&, WebKit::NetworkLoadParameters&&, WebKit::NetworkSession&&&) + 131 (memory:3114)
20  com.apple.WebKit              	0x00000001087d695a WebKit::NetworkResourceLoader::startNetworkLoad(WebCore::ResourceRequest&&, WebKit::NetworkResourceLoader::FirstLoad) + 1978 (NetworkResourceLoader.cpp:300)

[EWS Watchlist]

Comment on attachment 360593 [details]
Patch

Attachment 360593 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10954742

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360598 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360593 [details]
Patch

Attachment 360593 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10955169

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360603 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Alex Christensen]

Created attachment 360787 [details]
Patch

[Alex Christensen]

Created attachment 360792 [details]
Patch

[EWS Watchlist]

Comment on attachment 360792 [details]
Patch

Attachment 360792 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10981820

New failing tests:
fast/dom/HTMLAnchorElement/anchor-file-blob-download.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-xhr.https.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html
http/tests/local/blob/send-hybrid-blob-using-open-panel.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html
fast/dom/HTMLAnchorElement/anchor-download-synthetic-click.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html
http/tests/security/anchor-download-allow-blob.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html
fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.html
imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-blob-url.https.html
fast/dom/HTMLAnchorElement/anchor-nodownload-set.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event.https.html
fast/dom/HTMLAnchorElement/anchor-download.html

[EWS Watchlist]

Created attachment 360805 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

(In reply to Build Bot from comment #9)
> Comment on attachment 360792 [details]
> Patch
> 
> Attachment 360792 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/10981820
> 
> New failing tests:
> fast/dom/HTMLAnchorElement/anchor-file-blob-download.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-
> allowed.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html
> imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-
> xhr.https.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html
> http/tests/local/blob/send-hybrid-blob-using-open-panel.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.
> html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html
> fast/dom/HTMLAnchorElement/anchor-download-synthetic-click.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html
> http/tests/security/anchor-download-allow-blob.html
> fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-
> not-allowed.html
> fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.
> html
> imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-
> blob-url.https.html
> fast/dom/HTMLAnchorElement/anchor-nodownload-set.html
> imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event.
> https.html
> fast/dom/HTMLAnchorElement/anchor-download.html

Ouch :)

[EWS Watchlist]

Comment on attachment 360792 [details]
Patch

Attachment 360792 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10981952

New failing tests:
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-xhr.https.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html
fast/dom/HTMLAnchorElement/anchor-download-synthetic-click.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html
http/tests/security/anchor-download-allow-blob.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html
imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-blob-url.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event.https.html
fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.html

[EWS Watchlist]

Created attachment 360814 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Geoffrey Garen]

Comment on attachment 360792 [details]
Patch

Approach looks good but tests are failing.

[Alex Christensen]

Created attachment 360893 [details]
Patch

[Alex Christensen]

Created attachment 360899 [details]
Patch

[EWS Watchlist]

Comment on attachment 360899 [details]
Patch

Attachment 360899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10999846

New failing tests:
http/tests/local/blob/send-hybrid-blob-using-open-panel.html

[EWS Watchlist]

Created attachment 360920 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Alex Christensen]

The test failure has something to do with sandbox extensions because giving the NetworkProcess read-only access to the whole filesystem makes the test pass.

[Alex Christensen]

Created attachment 360955 [details]
Patch

[youenn fablet]

Comment on attachment 360955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360955&action=review

> Source/WebCore/platform/network/BlobRegistryImpl.h:59
>      void appendStorageItems(BlobData*, const BlobDataItemList&, long long offset, long long length);

Do we need all the methods below to be public?

> Source/WebCore/platform/network/FormData.cpp:321
> +    BlobData* blobData = static_cast<BlobRegistryImpl&>(blobRegistry).getBlobDataFromURL(url);

auto*?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:326
> +        file->prepareForFileAccess();

for loop in the if block?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:340
> +    auto loader = NetworkResourceLoader::create(WTFMove(loadParameters), *this, WTFMove(blobFiles));

I would move the blob handling in NetworkResourceLoader and PingLoad.
They already have a ref to the connection.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:418
> +    ASSERT_UNUSED(blobFiles, blobFiles.isEmpty());

Maybe just ASSERT(!parameters.request.httpBody())

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:571
> +Vector<RefPtr<WebCore::BlobDataFileReference>> NetworkConnectionToWebProcess::filesInBlob(const URL& url)

Could we make them a Vector<Ref> here or in a follow-up?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:192
> +    ASSERT_UNUSED(count, count == 1);

I am not sure we should use removeAllMatching here since it requires iterating on the whole array, for no good reason since the current code seems pretty good at making sure this double entry case will never happen.
We could use removeFirstMatching() and if we want to ASSERT that we cannot find connection in m_webProcessConnections.
Or we could make m_webProcessConnections a HashSet, not sure the order is actually important.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-917
> -        fileReference->prepareForFileAccess();

It seems a bit odd to no longer prepareForFileAccess in consumeSandboxExtensions but do revokeFileAccess in invalidateSandboxExtensions.
And revokeFileAccess is done conditionally on m_didConsumeSandboxExtensions.
Maybe we should at least do revokeFileAccess if m_fileReferences is not empty.

I think that keeping prepareForFileAccess/revokeFileAccess inside NetworkResourceLoader would also help ensuring to keep this logic in sync.

> Source/WebKit/NetworkProcess/PingLoad.cpp:85
> +            file->revokeFileAccess();

Can we have cases where file is nullptr?
Should we pass a Vector<Ref<>>.
It seems that NetworkResourceLoader is not doing this check.

[Alex Christensen]

Comment on attachment 360955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360955&action=review

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:571
>> +Vector<RefPtr<WebCore::BlobDataFileReference>> NetworkConnectionToWebProcess::filesInBlob(const URL& url)
> 
> Could we make them a Vector<Ref> here or in a follow-up?

Later.  This would add a significant amount of complexity to this already complex patch.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:192
>> +    ASSERT_UNUSED(count, count == 1);
> 
> I am not sure we should use removeAllMatching here since it requires iterating on the whole array, for no good reason since the current code seems pretty good at making sure this double entry case will never happen.
> We could use removeFirstMatching() and if we want to ASSERT that we cannot find connection in m_webProcessConnections.
> Or we could make m_webProcessConnections a HashSet, not sure the order is actually important.

Vector removal already requires complete iteration, reading, and writing.  We could change it to a HashSet in the future, but I don't think this is terribly important given the typical small size of this collection.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-917
>> -        fileReference->prepareForFileAccess();
> 
> It seems a bit odd to no longer prepareForFileAccess in consumeSandboxExtensions but do revokeFileAccess in invalidateSandboxExtensions.
> And revokeFileAccess is done conditionally on m_didConsumeSandboxExtensions.
> Maybe we should at least do revokeFileAccess if m_fileReferences is not empty.
> 
> I think that keeping prepareForFileAccess/revokeFileAccess inside NetworkResourceLoader would also help ensuring to keep this logic in sync.

I'm going to keep the prepareForFileAccess and revokeFileAccess symmetric in the same objects in my patch for landing.

[Alex Christensen]

Created attachment 361199 [details]
Patch

[Alex Christensen]

(In reply to youenn fablet from comment #21)
> Do we need all the methods below to be public?
Yes.  They are used in NetworkBlobRegistry now.

[Alex Christensen]

http://trac.webkit.org/r240984

[Radar WebKit Bug Importer]

<rdar://problem/47823846>

[Shawn Roberts]

It appears this is causing API time outs on Mac testing.

Reproduced with:

run-api-tests --root d240984 TestWebKitAPI.WebKit.ContextMenuDownloadHTMLDownloadAttribute —debug

Log file:

https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/2734/steps/run-api-tests/logs/stdio

Timeout

    TestWebKitAPI.WebKit.ContextMenuDownloadHTMLDownloadAttribute
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.

    TestWebKitAPI.WebKit.ContextMenuDownloadHTMLDownloadAttributeWithSlashes
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.

[Truitt Savell]

Reverted r240984 for reason:

Revision casued two API timeouts

Committed r241005: <https://trac.webkit.org/changeset/241005>

[Alex Christensen]

Created attachment 361255 [details]
patch to fix tests

[Alex Christensen]

Created attachment 361257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361257 [details]
Patch

Clearing flags on attachment: 361257

Committed r241008: <https://trac.webkit.org/changeset/241008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.