Bug 164458 – [WK2][NETWORK_SESSION] Add support for downloading file backed blobs

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Add support for downloading file backed blobs.

[Chris Dumez]

<rdar://problem/28905514>

[Chris Dumez]

Created attachment 294012 [details]
WIP patch

This patch works but I have not written a test for it yet. I think this would be testable using a file input, URL.createObjectURL() and a download anchor. However, we do not support eventSender.beginDragWithFile() in WebKit2 so I'll need another way.

[Alexey Proskuryakov]

Adding a way for a test to create a file object without beginDragWithFile would let us make lots of tests work with WebKit2. That would be a huge improvement.

[Chris Dumez]

(In reply to comment #3)
> Adding a way for a test to create a file object without beginDragWithFile
> would let us make lots of tests work with WebKit2. That would be a huge
> improvement.

It looks like there is a internals.createFile() operation. I'll try with this.

[Alexey Proskuryakov]

Interesting, I didn't know about that one. It is implemented fully in WebContent, so it doesn't get a sandbox extension of its own, but most things would work anyway because WebContent already has access to the whole file system.

[Chris Dumez]

(In reply to comment #5)
> Interesting, I didn't know about that one. It is implemented fully in
> WebContent, so it doesn't get a sandbox extension of its own, but most
> things would work anyway because WebContent already has access to the whole
> file system.

I managed to write a test using internals.createFile().

[Chris Dumez]

Created attachment 294014 [details]
Patch

[Carlos Garcia Campos]

It's a bit weird that NetworkDataTaskBlob is in charge of the file references only for downloads started with DownloadStart, but not for normal loads or loads converted to download. So, I wonder if we could unify that, by moving the handling to NetworkLoad and always transferring the file references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass its file references to the NetworkLoad constructor and PendingDownload too, we could add file references to NetworkLoadParameters to avoid adding more parameters to the NetworkLoad constructor. The NetworkLoad constructor could create the NetworkDataTask passing the file references. If we add file references to NetworkLoadParameters we could change the NetworkDataTask::create() to receive the NetworkLoadParameters instead and only pass the file references to the NetworkDataTaskBlob constructor. Note that NetworkResourceLoader can revoke the file references too early because in case of load converted to donwload, NetworkResourceLoader::cleanup() is called before the download has started the IO. See bug #164220 or more details about this.

[Chris Dumez]

(In reply to comment #8)
> It's a bit weird that NetworkDataTaskBlob is in charge of the file
> references only for downloads started with DownloadStart, but not for normal
> loads or loads converted to download. So, I wonder if we could unify that,
> by moving the handling to NetworkLoad and always transferring the file
> references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass
> its file references to the NetworkLoad constructor and PendingDownload too,
> we could add file references to NetworkLoadParameters to avoid adding more
> parameters to the NetworkLoad constructor. The NetworkLoad constructor could
> create the NetworkDataTask passing the file references. If we add file
> references to NetworkLoadParameters we could change the
> NetworkDataTask::create() to receive the NetworkLoadParameters instead and
> only pass the file references to the NetworkDataTaskBlob constructor. Note
> that NetworkResourceLoader can revoke the file references too early because
> in case of load converted to donwload, NetworkResourceLoader::cleanup() is
> called before the download has started the IO. See bug #164220 or more
> details about this.

Converting a blob load into a download is currently broken for other reasons than sandboxing. This is why I did not touch that code path yet. Otherwise, I think it would be as easy as calling transferBlobFileReferences() in NetworkReaourceLoader on the NetworkLoad object after constructing it. I could do it in this patch but I could not test it because it still does not download the blob for some reason. So I figured I will deal with pure downloads first.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > It's a bit weird that NetworkDataTaskBlob is in charge of the file
> > references only for downloads started with DownloadStart, but not for normal
> > loads or loads converted to download. So, I wonder if we could unify that,
> > by moving the handling to NetworkLoad and always transferring the file
> > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass
> > its file references to the NetworkLoad constructor and PendingDownload too,
> > we could add file references to NetworkLoadParameters to avoid adding more
> > parameters to the NetworkLoad constructor. The NetworkLoad constructor could
> > create the NetworkDataTask passing the file references. If we add file
> > references to NetworkLoadParameters we could change the
> > NetworkDataTask::create() to receive the NetworkLoadParameters instead and
> > only pass the file references to the NetworkDataTaskBlob constructor. Note
> > that NetworkResourceLoader can revoke the file references too early because
> > in case of load converted to donwload, NetworkResourceLoader::cleanup() is
> > called before the download has started the IO. See bug #164220 or more
> > details about this.
> 
> Converting a blob load into a download is currently broken for other reasons
> than sandboxing. This is why I did not touch that code path yet. Otherwise,
> I think it would be as easy as calling transferBlobFileReferences() in
> NetworkReaourceLoader on the NetworkLoad object after constructing it. I
> could do it in this patch but I could not test it because it still does not
> download the blob for some reason. So I figured I will deal with pure
> downloads first.

I will try to figure out why converting blob loads into downloads is broken. If it fixing them is not too much work, I will fix them in the same patch.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > It's a bit weird that NetworkDataTaskBlob is in charge of the file
> > > references only for downloads started with DownloadStart, but not for normal
> > > loads or loads converted to download. So, I wonder if we could unify that,
> > > by moving the handling to NetworkLoad and always transferring the file
> > > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass
> > > its file references to the NetworkLoad constructor and PendingDownload too,
> > > we could add file references to NetworkLoadParameters to avoid adding more
> > > parameters to the NetworkLoad constructor. The NetworkLoad constructor could
> > > create the NetworkDataTask passing the file references. If we add file
> > > references to NetworkLoadParameters we could change the
> > > NetworkDataTask::create() to receive the NetworkLoadParameters instead and
> > > only pass the file references to the NetworkDataTaskBlob constructor. Note
> > > that NetworkResourceLoader can revoke the file references too early because
> > > in case of load converted to donwload, NetworkResourceLoader::cleanup() is
> > > called before the download has started the IO. See bug #164220 or more
> > > details about this.
> > 
> > Converting a blob load into a download is currently broken for other reasons
> > than sandboxing. This is why I did not touch that code path yet. Otherwise,
> > I think it would be as easy as calling transferBlobFileReferences() in
> > NetworkReaourceLoader on the NetworkLoad object after constructing it. I
> > could do it in this patch but I could not test it because it still does not
> > download the blob for some reason. So I figured I will deal with pure
> > downloads first.
> 
> I will try to figure out why converting blob loads into downloads is broken.
> If it fixing them is not too much work, I will fix them in the same patch.

FYI, the reason converting blob loads into downloads fails is because it crashes the network process:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x00000001043768b0 WebKit::NetworkDataTaskBlob::download() + 528 (NetworkDataTaskBlob.cpp:477)
1   com.apple.WebKit              	0x000000010437767f WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1::operator()(WebCore::PolicyAction) const + 237 (NetworkDataTaskBlob.cpp:318)
2   com.apple.WebKit              	0x00000001042cccfd WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 175 (DownloadManager.cpp:125)
3   com.apple.WebKit              	0x000000010432b17b WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 161 (utility:753)
4   com.apple.WebKit              	0x00000001043331a0 void IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 60 (utility:753)
5   com.apple.WebKit              	0x0000000104332b85 void IPC::handleMessage<Messages::NetworkProcess::ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 86 (tuple:178)
6   com.apple.WebKit              	0x00000001042abbd3 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119 (memory:2702)
7   com.apple.WebKit              	0x00000001042ae805 IPC::Connection::dispatchOneMessage() + 175 (memory:2722)
8   com.apple.JavaScriptCore      	0x0000000105cfe5f9 WTF::RunLoop::performWork() + 169 (memory:2525)
9   com.apple.JavaScriptCore      	0x0000000105cfe8c2 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
10  com.apple.CoreFoundation      	0x00007fff84e0a991 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
11  com.apple.CoreFoundation      	0x00007fff84debafd __CFRunLoopDoSources0 + 557
12  com.apple.CoreFoundation      	0x00007fff84deaff6 __CFRunLoopRun + 934
13  com.apple.CoreFoundation      	0x00007fff84dea9f4 CFRunLoopRunSpecific + 420
14  com.apple.Foundation          	0x00007fff8684bcd2 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
15  com.apple.Foundation          	0x00007fff8684bbaa -[NSRunLoop(NSRunLoop) run] + 76
16  libxpc.dylib                  	0x00007fff9a5b289b _xpc_objc_main + 731
17  libxpc.dylib                  	0x00007fff9a5b12e4 xpc_main + 494
18  com.apple.WebKit.Networking   	0x00000001042687a2 main + 380 (XPCServiceMain.mm:130)
19  libdyld.dylib                 	0x00007fff9a34e255 start + 1

m_client is apparently null.

[Chris Dumez]

Created attachment 294028 [details]
Patch

[Chris Dumez]

Created attachment 294033 [details]
Patch

[Chris Dumez]

Created attachment 294034 [details]
Patch

[Darin Adler]

Comment on attachment 294034 [details]
Patch

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

> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60
> +        parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url());

Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>?

> Source/WebKit2/NetworkProcess/NetworkDataTask.h:77
> +    static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&);

Can this be NetworkLoadParameters&& so we can take ownership and possibly cut down on reference count churn? I guess maybe not, but at least the vector of file references seems like it could be moved in rather than copied. Maybe this can be NetworkLoadParameters& and we can specifically document that it will take ownership of the file references?

> Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:51
> +    static Ref<NetworkDataTask> create(NetworkSession& session, NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, WebCore::ContentSniffingPolicy shouldContentSniff, const Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences)

Can this be Vector&& instead of const Vector& so we can take ownership and avoid the reference count churn?

[Chris Dumez]

Comment on attachment 294034 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60
>> +        parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url());
> 
> Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>?

No, this can be updated. I'll do it in this patch if it is a small refactoring.

>> Source/WebKit2/NetworkProcess/NetworkDataTask.h:77
>> +    static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&);
> 
> Can this be NetworkLoadParameters&& so we can take ownership and possibly cut down on reference count churn? I guess maybe not, but at least the vector of file references seems like it could be moved in rather than copied. Maybe this can be NetworkLoadParameters& and we can specifically document that it will take ownership of the file references?

NetworkLoad owns the NetworkLoadParameters and keeps it as a data member so this cannot be a NetworkLoadParameters&&. This could in theory me a NetworkLoadParameters& if we removed the const from the NetworkLoad::m_parameters data member. However, I feel it would be a bit confusing for NetworkLoad to change NetworkLoad::m_parameters. Also, this is not really hot code so I would prioritize readability / simplicity over performance. ref counting churn is unlikely to be an issue here.

[Chris Dumez]

Comment on attachment 294034 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60
>>> +        parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url());
>> 
>> Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>?
> 
> No, this can be updated. I'll do it in this patch if it is a small refactoring.

This is actually inconvenient for 2 reasons in this case:
1. I copy the vector at the moment and the vector is no longer copyable. So to copy I would have to manually iterate over the vector and use copyRef().
2. Vector::appendVector(const Vector&) is used in several places with this vector and this does not work for the same reason. I think we would need a Vector::appendVector(Vector&&) overload which calls WTFMove() on each vector item.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > It's a bit weird that NetworkDataTaskBlob is in charge of the file
> > > > references only for downloads started with DownloadStart, but not for normal
> > > > loads or loads converted to download. So, I wonder if we could unify that,
> > > > by moving the handling to NetworkLoad and always transferring the file
> > > > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass
> > > > its file references to the NetworkLoad constructor and PendingDownload too,
> > > > we could add file references to NetworkLoadParameters to avoid adding more
> > > > parameters to the NetworkLoad constructor. The NetworkLoad constructor could
> > > > create the NetworkDataTask passing the file references. If we add file
> > > > references to NetworkLoadParameters we could change the
> > > > NetworkDataTask::create() to receive the NetworkLoadParameters instead and
> > > > only pass the file references to the NetworkDataTaskBlob constructor. Note
> > > > that NetworkResourceLoader can revoke the file references too early because
> > > > in case of load converted to donwload, NetworkResourceLoader::cleanup() is
> > > > called before the download has started the IO. See bug #164220 or more
> > > > details about this.
> > > 
> > > Converting a blob load into a download is currently broken for other reasons
> > > than sandboxing. This is why I did not touch that code path yet. Otherwise,
> > > I think it would be as easy as calling transferBlobFileReferences() in
> > > NetworkReaourceLoader on the NetworkLoad object after constructing it. I
> > > could do it in this patch but I could not test it because it still does not
> > > download the blob for some reason. So I figured I will deal with pure
> > > downloads first.
> > 
> > I will try to figure out why converting blob loads into downloads is broken.
> > If it fixing them is not too much work, I will fix them in the same patch.
> 
> FYI, the reason converting blob loads into downloads fails is because it
> crashes the network process:
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebKit              	0x00000001043768b0
> WebKit::NetworkDataTaskBlob::download() + 528 (NetworkDataTaskBlob.cpp:477)
> 1   com.apple.WebKit              	0x000000010437767f
> WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::
> Error)::$_1::operator()(WebCore::PolicyAction) const + 237
> (NetworkDataTaskBlob.cpp:318)
> 2   com.apple.WebKit              	0x00000001042cccfd
> WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit::
> DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) +
> 175 (DownloadManager.cpp:125)
> 3   com.apple.WebKit              	0x000000010432b17b
> WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit::
> DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) +
> 161 (utility:753)
> 4   com.apple.WebKit              	0x00000001043331a0 void
> IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void
> (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle const&, bool),
> std::__1::tuple<WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul,
> 3ul>(WebKit::NetworkProcess*, void
> (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle const&, bool),
> std::__1::tuple<WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle, bool>&&,
> std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 60
> (utility:753)
> 5   com.apple.WebKit              	0x0000000104332b85 void
> IPC::handleMessage<Messages::NetworkProcess::
> ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void
> (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&,
> WebKit::NetworkProcess*, void
> (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String,
> WebKit::SandboxExtension::Handle const&, bool)) + 86 (tuple:178)
> 6   com.apple.WebKit              	0x00000001042abbd3
> IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder,
> std::__1::default_delete<IPC::Decoder> >) + 119 (memory:2702)
> 7   com.apple.WebKit              	0x00000001042ae805
> IPC::Connection::dispatchOneMessage() + 175 (memory:2722)
> 8   com.apple.JavaScriptCore      	0x0000000105cfe5f9
> WTF::RunLoop::performWork() + 169 (memory:2525)
> 9   com.apple.JavaScriptCore      	0x0000000105cfe8c2
> WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
> 10  com.apple.CoreFoundation      	0x00007fff84e0a991
> __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
> 11  com.apple.CoreFoundation      	0x00007fff84debafd __CFRunLoopDoSources0
> + 557
> 12  com.apple.CoreFoundation      	0x00007fff84deaff6 __CFRunLoopRun + 934
> 13  com.apple.CoreFoundation      	0x00007fff84dea9f4 CFRunLoopRunSpecific +
> 420
> 14  com.apple.Foundation          	0x00007fff8684bcd2 -[NSRunLoop(NSRunLoop)
> runMode:beforeDate:] + 277
> 15  com.apple.Foundation          	0x00007fff8684bbaa -[NSRunLoop(NSRunLoop)
> run] + 76
> 16  libxpc.dylib                  	0x00007fff9a5b289b _xpc_objc_main + 731
> 17  libxpc.dylib                  	0x00007fff9a5b12e4 xpc_main + 494
> 18  com.apple.WebKit.Networking   	0x00000001042687a2 main + 380
> (XPCServiceMain.mm:130)
> 19  libdyld.dylib                 	0x00007fff9a34e255 start + 1
> 
> m_client is apparently null.

Yes, this is not specific to blobs, it's bug #164220

[Carlos Garcia Campos]

(In reply to comment #15)
> Comment on attachment 294034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294034&action=review
> 
> > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60
> > +        parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url());
> 
> Does filesInBlob really need to return a Vector<RefPtr> instead of a
> Vector<Ref>?
> 
> > Source/WebKit2/NetworkProcess/NetworkDataTask.h:77
> > +    static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&);
> 
> Can this be NetworkLoadParameters&& so we can take ownership and possibly
> cut down on reference count churn? I guess maybe not, but at least the
> vector of file references seems like it could be moved in rather than
> copied. Maybe this can be NetworkLoadParameters& and we can specifically
> document that it will take ownership of the file references?

This was my idea, but I didn't notice that m_parameters is const member of NetworkLoad.

> > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:51
> > +    static Ref<NetworkDataTask> create(NetworkSession& session, NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, WebCore::ContentSniffingPolicy shouldContentSniff, const Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences)
> 
> Can this be Vector&& instead of const Vector& so we can take ownership and
> avoid the reference count churn?

[Carlos Garcia Campos]

Comment on attachment 294034 [details]
Patch

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

> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:48
> +void DownloadManager::startDownload(NetworkConnectionToWebProcess* connection, SessionID sessionID, DownloadID downloadID, const ResourceRequest& request, const String& suggestedName)

Can we pass a reference instead of a pointer?

[Chris Dumez]

(In reply to comment #20)
> Comment on attachment 294034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294034&action=review
> 
> > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:48
> > +void DownloadManager::startDownload(NetworkConnectionToWebProcess* connection, SessionID sessionID, DownloadID downloadID, const ResourceRequest& request, const String& suggestedName)
> 
> Can we pass a reference instead of a pointer?

One of the call sites does not have it, this is why it is a pointer. There is a RequestDownload IPC coming from the UI process, which ends up calling startDownload which therefore does not have a corresponding connection to a WebProcess.

[Build Bot]

Comment on attachment 294034 [details]
Patch

Attachment 294034 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2469276

New failing tests:
fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html

[Build Bot]

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

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

[Chris Dumez]

Created attachment 294087 [details]
Patch

[Build Bot]

Comment on attachment 294087 [details]
Patch

Attachment 294087 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2470244

New failing tests:
fast/dom/HTMLAnchorElement/anchor-file-blob-download.html
fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html

[Build Bot]

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

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

[Chris Dumez]

(In reply to comment #26)
> Created attachment 294103 [details]
> Archive of layout-test-results from ews107 for mac-yosemite-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

Hmm, the bot is using Yosemite, which likely means it is not using NETWORK_SESSION but the old code path instead. This is why my new tests are failing. I think I'll skip the new tests on non-Sierra and file a bug for myself to fix the !NETWORK_SESSION code path as well.

[Chris Dumez]

Created attachment 294173 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 294173 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:484
> -    ASSERT(m_client);
> -    m_client->didBecomeDownload();
> +    if (m_client)
> +        m_client->didBecomeDownload();

Note that this is not correct, we should have a client always here. But it's properly fixed in bug #164220, this will need to be rebased if that one land first, or the other if this lands first, so it's not a problem.

[Chris Dumez]

Comment on attachment 294173 [details]
Patch

Clearing flags on attachment: 294173

Committed r208438: <http://trac.webkit.org/changeset/208438>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

These two tests crash on Sierra.

0   com.apple.JavaScriptCore      	0x000000010bf32d44 WTFCrash + 36 (Assertions.cpp:323)
1   com.apple.WebKit              	0x000000010723b721 WebKit::NetworkDataTaskBlob::readFile(WebCore::BlobDataItem const&) + 81 (NetworkDataTaskBlob.cpp:363)
2   com.apple.WebKit              	0x000000010723b47a WebKit::NetworkDataTaskBlob::read() + 282 (NetworkDataTaskBlob.cpp:345)
3   com.apple.WebKit              	0x000000010723c509 WebKit::NetworkDataTaskBlob::download() + 1145 (NetworkDataTaskBlob.cpp:487)
4   com.apple.WebKit              	0x0000000107240cff WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1::operator()(WebCore::PolicyAction) const + 255 (NetworkDataTaskBlob.cpp:328)
5   com.apple.WebKit              	0x0000000107240b5a WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1>::call(WebCore::PolicyAction) + 42 (Function.h:89)
6   com.apple.WebKit              	0x0000000106f4ec94 WTF::Function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const + 132 (Function.h:50)
7   com.apple.WebKit              	0x0000000106f4e29f WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 639 (DownloadManager.cpp:129)
8   com.apple.WebKit              	0x00000001070e8b22 WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 162 (NetworkProcess.cpp:530)
9   com.apple.WebKit              	0x0000000107116edc void IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 364 (HandleMessage.h:15)
10  com.apple.WebKit              	0x0000000107116b28 void IPC::callMemberFunction<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 88 (HandleMessage.h:22)
11  com.apple.WebKit              	0x0000000107113602 void IPC::handleMessage<Messages::NetworkProcess::ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 338 (HandleMessage.h:102)
12  com.apple.WebKit              	0x0000000107111a78 WebKit::NetworkProcess::didReceiveNetworkProcessMessage(IPC::Connection&, IPC::Decoder&) + 1976 (NetworkProcessMessageReceiver.cpp:129)
13  com.apple.WebKit              	0x00000001070e409b WebKit::NetworkProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 171 (NetworkProcess.cpp:153)
14  com.apple.WebKit              	0x0000000106ec3f53 IPC::Connection::dispatchMessage(IPC::Decoder&) + 51 (Connection.cpp:894)
15  com.apple.WebKit              	0x0000000106eb97f8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 712 (Connection.cpp:922)
16  com.apple.WebKit              	0x0000000106ec4550 IPC::Connection::dispatchOneMessage() + 1520 (Connection.cpp:951)
17  com.apple.WebKit              	0x0000000106edd15d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() + 29 (Connection.cpp:888)
18  com.apple.WebKit              	0x0000000106edd0b9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() + 25 (Function.h:89)
19  com.apple.JavaScriptCore      	0x000000010bf5b42e WTF::Function<void ()>::operator()() const + 94 (Function.h:50)
20  com.apple.JavaScriptCore      	0x000000010bf76a53 WTF::RunLoop::performWork() + 211 (RunLoop.cpp:106)
21  com.apple.JavaScriptCore      	0x000000010bf77244 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
22  com.apple.CoreFoundation      	0x00007fff93602581 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
23  com.apple.CoreFoundation      	0x00007fff935e398c __CFRunLoopDoSources0 + 556
24  com.apple.CoreFoundation      	0x00007fff935e2e76 __CFRunLoopRun + 934
25  com.apple.CoreFoundation      	0x00007fff935e2874 CFRunLoopRunSpecific + 420
26  com.apple.Foundation          	0x00007fff94ffecb2 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
27  com.apple.Foundation          	0x00007fff94ffeb8a -[NSRunLoop(NSRunLoop) run] + 76
28  libxpc.dylib                  	0x00007fffa8917887 _xpc_objc_main + 731
29  libxpc.dylib                  	0x00007fffa89162d0 xpc_main + 494
30  com.apple.WebKit.Networking   	0x0000000106df618d main + 797
31  libdyld.dylib                 	0x00007fffa86b4255 start + 1

[Alex Christensen]

rolled out in http://trac.webkit.org/changeset/208513

[Chris Dumez]

(In reply to comment #33)
> rolled out in http://trac.webkit.org/changeset/208513

ASSERT(m_client); is hit in NetworkDataTaskBlob::readFile(const BlobDataItem& item). Why is this assertion correct? My understanding is that the NetworkDataTaskBlob no longer has a client after the load gets converted into a download.

Will Carlos' patch at https://bugs.webkit.org/show_bug.cgi?id=164220 help? My understand is that it keeps a client alive a bit longer but I do not know if that's long enough to actually read the file.

[Chris Dumez]

Created attachment 294330 [details]
Patch

[Chris Dumez]

Created attachment 294334 [details]
Patch

[Chris Dumez]

Comment on attachment 294334 [details]
Patch

Clearing flags on attachment: 294334

Committed r208522: <http://trac.webkit.org/changeset/208522>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.