| Summary: | Stop using blobRegistry in NetworkProcess | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, ews-watchlist, galpeter, ggaren, rniwa, sroberts, tsavell, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Alex Christensen
2019-01-30 10:31:21 PST
Created attachment 360593 [details]
Patch
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) 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. 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
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. 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
Created attachment 360787 [details]
Patch
Created attachment 360792 [details]
Patch
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 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
(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 :) 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 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
Comment on attachment 360792 [details]
Patch
Approach looks good but tests are failing.
Created attachment 360893 [details]
Patch
Created attachment 360899 [details]
Patch
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 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
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. Created attachment 360955 [details]
Patch
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. 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. Created attachment 361199 [details]
Patch
(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. 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. Reverted r240984 for reason: Revision casued two API timeouts Committed r241005: <https://trac.webkit.org/changeset/241005> Created attachment 361255 [details]
patch to fix tests
Created attachment 361257 [details]
Patch
Comment on attachment 361257 [details] Patch Clearing flags on attachment: 361257 Committed r241008: <https://trac.webkit.org/changeset/241008> All reviewed patches have been landed. Closing bug. |