Bug 163939

Created attachment 292747 [details] WIP patch This is a WIP patch, it just needs to update XCode file to add the new files, and deal with iOS build failures.

Could you guys help me with the xcode changes so that I can continue working on this, please?

I'll work on it later tonight. I'll try to have something by 9:00 your time.

(In reply to comment #3) > I'll work on it later tonight. I'll try to have something by 9:00 your time. Thanks! note that I'm not asking you to make it build, just to include the files, I'll fight with EWS.

Created attachment 293118 [details] Builds macOS and iOS, passes fast layout tests, loads various pages.

Created attachment 293287 [details] Patch

Comment on attachment 293287 [details] Patch Attachment 293287 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2398618 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus-sync.htm http/tests/xmlhttprequest/access-control-and-redirects.html http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-infinite-sync.htm imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-network-error-sync.htm

Created attachment 293288 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 293336 [details] Try to fix mac builds

Created attachment 293337 [details] Try to fix failing tests I messed it up in the NetworkLoad refactoring, confused because we were using ResourceHandle API (with null handle) in network session code branch.

Comment on attachment 293337 [details] Try to fix failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=293337&action=review ! I'll have to review this from my Sierra machine and make sure things like the new blob downloads still work > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:77 > + , m_blobData(static_cast<BlobRegistryImpl&>(blobRegistry()).getBlobDataFromURL(request.url())) We should move away from using a global blob registry, but not in this patch. > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:206 > + // If the size is -1, it means the file has been moved or changed. Fail now. > + if (size == -1) { It would be nice if we could use Optional instead of having magic negative numbers. I know the blob code does things like this a lot, though. We need to clean that up. > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:48 > +class NetworkDataTaskBlob final : public NetworkDataTask, public WebCore::FileStreamClient { I think we should call this BlobDataTask, except it is used by a NetworkLoad. Blobs are weird.

(In reply to comment #11) > Comment on attachment 293337 [details] > Try to fix failing tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293337&action=review > > ! > I'll have to review this from my Sierra machine and make sure things like > the new blob downloads still work Great, thanks! I ran the anchor download tests that use blobs and they passed. > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:77 > > + , m_blobData(static_cast<BlobRegistryImpl&>(blobRegistry()).getBlobDataFromURL(request.url())) > > We should move away from using a global blob registry, but not in this patch. I don't even know how that works, TBH. > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:206 > > + // If the size is -1, it means the file has been moved or changed. Fail now. > > + if (size == -1) { > > It would be nice if we could use Optional instead of having magic negative > numbers. I know the blob code does things like this a lot, though. We need > to clean that up. That code is indeed copy-pasted from the ResourceHandle implementation, I tried to not change the existing code too much to avoid regressions, but I agree we can take advantage to do some small low risk cleanups. > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:48 > > +class NetworkDataTaskBlob final : public NetworkDataTask, public WebCore::FileStreamClient { > > I think we should call this BlobDataTask, except it is used by a > NetworkLoad. Blobs are weird. I read this is as the NetworkDataTask implementation for Blobs.

Comment on attachment 293337 [details] Try to fix failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=293337&action=review >>> Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:77 >>> + , m_blobData(static_cast<BlobRegistryImpl&>(blobRegistry()).getBlobDataFromURL(request.url())) >> >> We should move away from using a global blob registry, but not in this patch. > > I don't even know how that works, TBH. Right now, blobRegistry() returns the global BlobRegistry, which is returned through PlatformStrategies. We need to remove PlatformStrategies::blobRegistry() and instead access the BlobRegistry through the NetworkSession, because it should be a member variable of the NetworkSession. Unfortunately, we need to keep the ResourceHandle code alive for another few years, and this development involves changing how the ResourceHandle registers a BlobResourceHandle constructor, adding SessionIDs in many places, and making some kind of map of SessionID to BlobRegistry just for ResourceHandles.

This makes http/tests/security/anchor-download-allow-blob.html fail on Sierra as-is. Probably a minor modification.

Comment on attachment 293337 [details] Try to fix failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=293337&action=review I'm sure this is about 95% correct, but blobs and downloads don't work on Sierra and I'm not sure what's wrong. Chris wrote our blob download code. > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:258 > +void NetworkDataTaskBlob::didReceiveResponse(Error errorCode) Please add LOG(NetworkSession, ...) to each of these callbacks so they can actually be debugged. > Source/WebKit2/NetworkProcess/NetworkSession.h:64 > + HashSet<NetworkDataTask*> m_dataTaskSet; This is strange to have NetworkDataTaskBlobs kept in a set here on cocoa but not with Network-NetworkDataTasks. That might be the right thing to do, but I'm not sure.

(In reply to comment #14) > This makes http/tests/security/anchor-download-allow-blob.html fail on > Sierra as-is. Probably a minor modification. It's not even creating a NetworkDataTaskBlob in this test.

What failure are you getting? could you share the diff? It passes here: $ wktr http://127.0.0.1:8000/security/anchor-download-allow-blob.html Content-Type: text/plain Download started. Downloading URL with suggested filename "foo.pdf" Download completed. Tests that a suggested filename on a download attribute is allowed if the link is a blob URL. The suggested filename at the top should be foo.pdf. PASS successfullyParsed is true TEST COMPLETE #EOF #EOF #EOF

(In reply to comment #12) > > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:206 > > > + // If the size is -1, it means the file has been moved or changed. Fail now. > > > + if (size == -1) { > > > > It would be nice if we could use Optional instead of having magic negative > > numbers. I know the blob code does things like this a lot, though. We need > > to clean that up. > > That code is indeed copy-pasted from the ResourceHandle implementation, I > tried to not change the existing code too much to avoid regressions, but I > agree we can take advantage to do some small low risk cleanups. hmm, that -1 comes from FileStream::getSize(), I don't think we want to change that API in this patch.

Created attachment 293547 [details] Updated patch including logs Added some log messages.

Here's the diff. In this case, -999 is kCFURLErrorCancelled, which makes me think we are making a NetworkDataTask where we shouldn't be: --- /Users/alexchristensen/code/OpenSource/WebKitBuild/Debug/layout-test-results/http/tests/security/anchor-download-allow-blob-expected.txt +++ /Users/alexchristensen/code/OpenSource/WebKitBuild/Debug/layout-test-results/http/tests/security/anchor-download-allow-blob-actual.txt @@ -1,6 +1,7 @@ Download started. Downloading URL with suggested filename "foo.pdf" -Download completed. +Download failed. +Failed: NSURLErrorDomain, code=-999, description=cancelled Tests that a suggested filename on a download attribute is allowed if the link is a blob URL. The suggested filename at the top should be foo.pdf.

And here are the logs: Created NetworkSession with cookieAcceptPolicy 2 1 Creating NetworkDataTask with URL http://127.0.0.1:8000/security/anchor-download-allow-blob.html 1 didReceiveResponse 1 didReceiveResponse completionHandler (cancel) 1 didCompleteWithError 2 Creating NetworkDataTask with URL http://127.0.0.1:8000/resources/js-test-pre.js 3 Creating NetworkDataTask with URL http://127.0.0.1:8000/resources/js-test-post.js 2 didReceiveResponse 2 didReceiveResponse completionHandler (cancel) 2 didCompleteWithError 3 didReceiveResponse 3 didReceiveResponse completionHandler (cancel) 3 didCompleteWithError 0x10ebf8840 - Created NetworkDataTaskBlob for blob:http://127.0.0.1:8000/dc810a1b-32ab-494a-a7c6-bdd08c15e3d9 0x10ebf8840 - NetworkDataTaskBlob::didReceiveResponse(0) 0x10ebf8840 - NetworkDataTaskBlob::didReceiveResponse completionHandler (1) 0x10ebf8840 - NetworkDataTaskBlob::download to /var/folders/d_/755k76r526315y2mt8ffpc3m0000gn/T/WebKitTestRunner-GrEfEg/foo.pdf 0x10ebf8840 - NetworkDataTaskBlob::didFailDownload

I had to add #include "Logging.h" to NetworkDataTaskBlob.cpp to get it to compile on Sierra, btw

errno is 1 #define EPERM 1 /* Operation not permitted */

Aha! We're not using the SandboxExtension::Handle given to setPendingDownloadLocation

Created attachment 293623 [details] Patch

Added sandbox extension handling, r=me, please two more reviewers look at this because it touches the disk with openFile, etc.

(In reply to comment #24) > Aha! We're not using the SandboxExtension::Handle given to > setPendingDownloadLocation Ah, right :-)

(In reply to comment #15) > Comment on attachment 293337 [details] > Try to fix failing tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293337&action=review > > I'm sure this is about 95% correct, but blobs and downloads don't work on > Sierra and I'm not sure what's wrong. Chris wrote our blob download code. > > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:258 > > +void NetworkDataTaskBlob::didReceiveResponse(Error errorCode) > > Please add LOG(NetworkSession, ...) to each of these callbacks so they can > actually be debugged. > > > Source/WebKit2/NetworkProcess/NetworkSession.h:64 > > + HashSet<NetworkDataTask*> m_dataTaskSet; > > This is strange to have NetworkDataTaskBlobs kept in a set here on cocoa but > not with Network-NetworkDataTasks. That might be the right thing to do, but > I'm not sure. Actually, NetworkSessionCocoa::invalidateAndCancel() should call NetworkSession::invalidateAndCancel() to invalidate the blobs. I'll update the patch.

Created attachment 293650 [details] Patch for landing

Alex, I didn't ask for r? because you already said r=me and the patch already has the Reviewed by lines filled. So, other reviewers could just comment or even set cq+/-

Comment on attachment 293650 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=293650&action=review https://trac.webkit.org/changeset/208298 > Source/WebKit2/NetworkProcess/Downloads/Download.cpp:173 > String Download::decideDestinationWithSuggestedFilename(const String& filename, bool& allowOverwrite) We're trying to get rid of this in favor of the async version, but that can be done in a followup. > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:566 > + ASSERT(!m_sandboxExtension); I had to move this assertion to after the check for downloads.

Comment on attachment 293650 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=293650&action=review >> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:173 >> String Download::decideDestinationWithSuggestedFilename(const String& filename, bool& allowOverwrite) > > We're trying to get rid of this in favor of the async version, but that can be done in a followup. This is still not used when NETWORK_SESSION is enabled, I just moved the ifdefs to include more things that are now only built when NETWORK_SESSION si disabled.

Rolled out in http://trac.webkit.org/changeset/208344 This caused guardmalloc and asan crashes. Something about the change to NetworkLoad::continueWillSendRequest Tests like http/tests/xmlhttprequest/redirect-cross-origin-sync.html were crashing

(In reply to comment #33) > Rolled out in http://trac.webkit.org/changeset/208344 > This caused guardmalloc and asan crashes. Something about the change to > NetworkLoad::continueWillSendRequest > Tests like http/tests/xmlhttprequest/redirect-cross-origin-sync.html were > crashing Yes, I screwed it up trying to make it easier to read. So, the thing is that when the new request is null we call didCompleteWithError, that notifies the NetworkResourceLoader that cleans it up deleting the network load. Before the patch we called the completion handler with { }, but with the patch we pass m_currentRequest because it was already null anyway, but at this point it has been freed. So, I'll bring back the previous approach duplicating the call to the complation handler to only use m_currentRequest when it's not null, and ensuring we return after didCompleteWithError because we will be freed at that point.

Created attachment 293876 [details] Updated patch for landing

Asan agrees! http://trac.webkit.org/changeset/208388