Bug 163939 – NetworkSession: Add NetworkDataTask implementation for blobs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

And get rid of ResourceHandle in NetworkProcess when NetworkSession is enabled.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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

[Keith Rollin]

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

[Carlos Garcia Campos]

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

[Keith Rollin]

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

[Carlos Garcia Campos]

Created attachment 293287 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Carlos Garcia Campos]

Created attachment 293336 [details]
Try to fix mac builds

[Carlos Garcia Campos]

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.

[Alex Christensen]

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.

[Carlos Garcia Campos]

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

[Alex Christensen]

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.

[Alex Christensen]

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

[Alex Christensen]

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.

[Alex Christensen]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 293547 [details]
Updated patch including logs

Added some log messages.

[Alex Christensen]

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.

[Alex Christensen]

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

[Alex Christensen]

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

[Alex Christensen]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 293623 [details]
Patch

[Alex Christensen]

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

[Carlos Garcia Campos]

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

Ah, right :-)

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 293650 [details]
Patch for landing

[Carlos Garcia Campos]

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+/-

[Alex Christensen]

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.

[Carlos Garcia Campos]

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.

[Alex Christensen]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 293876 [details]
Updated patch for landing

[Alex Christensen]

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